Speedup hot code in actionDiscardTags

(re: #4611)
2026-02-13 01:02:58 +00:00 · 2018-02-04 14:57:26 -05:00
parent 42dd36addb
commit dac753c4ea
5 changed files with 135 additions and 131 deletions
--- a/data/discarded.json
+++ b/data/discarded.json
@@ -1,50 +1,51 @@
 {
-    "dataDiscarded": [
-        "created_by",
-        "odbl",
-        "odbl:note",
+    "dataDiscarded": {
+        "created_by": true,
+        "odbl": true,
+        "odbl:note": true,

-        "tiger:upload_uuid",
-        "tiger:tlid",
-        "tiger:source",
-        "tiger:separated",
+        "tiger:upload_uuid": true,
+        "tiger:tlid": true,
+        "tiger:source": true,
+        "tiger:separated": true,

-        "geobase:datasetName",
-        "geobase:uuid",
-        "sub_sea:type",
+        "geobase:datasetName": true,
+        "geobase:uuid": true,
+        "sub_sea:type": true,

-        "KSJ2:ADS",
-        "KSJ2:ARE",
-        "KSJ2:AdminArea",
-        "KSJ2:COP_label",
-        "KSJ2:DFD",
-        "KSJ2:INT",
-        "KSJ2:INT_label",
-        "KSJ2:LOC",
-        "KSJ2:LPN",
-        "KSJ2:OPC",
-        "KSJ2:PubFacAdmin",
-        "KSJ2:RAC",
-        "KSJ2:RAC_label",
-        "KSJ2:RIC",
-        "KSJ2:RIN",
-        "KSJ2:WSC",
-        "KSJ2:coordinate",
-        "KSJ2:curve_id",
-        "KSJ2:curve_type",
-        "KSJ2:filename",
-        "KSJ2:lake_id",
-        "KSJ2:lat",
-        "KSJ2:long",
-        "KSJ2:river_id",
-        "yh:LINE_NAME",
-        "yh:LINE_NUM",
-        "yh:STRUCTURE",
-        "yh:TOTYUMONO",
-        "yh:TYPE",
-        "yh:WIDTH",
-        "yh:WIDTH_RANK",
+        "KSJ2:ADS": true,
+        "KSJ2:ARE": true,
+        "KSJ2:AdminArea": true,
+        "KSJ2:COP_label": true,
+        "KSJ2:DFD": true,
+        "KSJ2:INT": true,
+        "KSJ2:INT_label": true,
+        "KSJ2:LOC": true,
+        "KSJ2:LPN": true,
+        "KSJ2:OPC": true,
+        "KSJ2:PubFacAdmin": true,
+        "KSJ2:RAC": true,
+        "KSJ2:RAC_label": true,
+        "KSJ2:RIC": true,
+        "KSJ2:RIN": true,
+        "KSJ2:WSC": true,
+        "KSJ2:coordinate": true,
+        "KSJ2:curve_id": true,
+        "KSJ2:curve_type": true,
+        "KSJ2:filename": true,
+        "KSJ2:lake_id": true,
+        "KSJ2:lat": true,
+        "KSJ2:long": true,
+        "KSJ2:river_id": true,

-        "SK53_bulk:load"
-    ]
+        "yh:LINE_NAME": true,
+        "yh:LINE_NUM": true,
+        "yh:STRUCTURE": true,
+        "yh:TOTYUMONO": true,
+        "yh:TYPE": true,
+        "yh:WIDTH": true,
+        "yh:WIDTH_RANK": true,
+
+        "SK53_bulk:load": true
+    }
 }
--- a/modules/actions/discard_tags.js
+++ b/modules/actions/discard_tags.js
@@ -9,15 +9,21 @@ export function actionDiscardTags(difference) {

    return function(graph) {
        function discardTags(entity) {
-            if (!_isEmpty(entity.tags)) {
-                var tags = {};
-                _each(entity.tags, function(v, k) {
-                    if (v) tags[k] = v;
-                });
+            var tags = {};
+            var keys = Object.keys(entity.tags);
+            var discarded = false;

-                graph = graph.replace(entity.update({
-                    tags: _omit(tags, dataDiscarded)
-                }));
+            for (var i = 0; i < keys.length; i++) {
+                var k = keys[i];
+                if (dataDiscarded[k] || !entity.tags[k]) {
+                    discarded = true;
+                } else {
+                    tags[k] = entity.tags[k];
+                }
+            }
+
+            if (discarded) {
+                graph = graph.replace(entity.update({ tags: tags }));
            }
        }

--- a/modules/actions/merge_remote_changes.js
+++ b/modules/actions/merge_remote_changes.js
@@ -1,5 +1,4 @@
 import _clone from 'lodash-es/clone';
-import _includes from 'lodash-es/includes';
 import _isEqual from 'lodash-es/isEqual';
 import _isFunction from 'lodash-es/isFunction';
 import _keys from 'lodash-es/keys';
@@ -17,8 +16,8 @@ import { dataDiscarded } from '../../data';


 export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser) {
-    var option = 'safe',  // 'safe', 'force_local', 'force_remote'
-        conflicts = [];
+    var _option = 'safe';  // 'safe', 'force_local', 'force_remote'
+    var _conflicts = [];


    function user(d) {
@@ -32,32 +31,32 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser
            return (Math.abs(a[0] - b[0]) < epsilon) && (Math.abs(a[1] - b[1]) < epsilon);
        }

-        if (option === 'force_local' || pointEqual(target.loc, remote.loc)) {
+        if (_option === 'force_local' || pointEqual(target.loc, remote.loc)) {
            return target;
        }
-        if (option === 'force_remote') {
+        if (_option === 'force_remote') {
            return target.update({loc: remote.loc});
        }

-        conflicts.push(t('merge_remote_changes.conflict.location', { user: user(remote.user) }));
+        _conflicts.push(t('merge_remote_changes.conflict.location', { user: user(remote.user) }));
        return target;
    }


    function mergeNodes(base, remote, target) {
-        if (option === 'force_local' || _isEqual(target.nodes, remote.nodes)) {
+        if (_option === 'force_local' || _isEqual(target.nodes, remote.nodes)) {
            return target;
        }
-        if (option === 'force_remote') {
+        if (_option === 'force_remote') {
            return target.update({nodes: remote.nodes});
        }

-        var ccount = conflicts.length,
-            o = base.nodes || [],
-            a = target.nodes || [],
-            b = remote.nodes || [],
-            nodes = [],
-            hunks = diff3Merge(a, o, b, true);
+        var ccount = _conflicts.length;
+        var o = base.nodes || [];
+        var a = target.nodes || [];
+        var b = remote.nodes || [];
+        var nodes = [];
+        var hunks = diff3Merge(a, o, b, true);

        for (var i = 0; i < hunks.length; i++) {
            var hunk = hunks[i];
@@ -72,13 +71,13 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser
                } else if (_isEqual(c.o, c.b)) {  // only changed locally
                    nodes.push.apply(nodes, c.a);
                } else {       // changed both locally and remotely
-                    conflicts.push(t('merge_remote_changes.conflict.nodelist', { user: user(remote.user) }));
+                    _conflicts.push(t('merge_remote_changes.conflict.nodelist', { user: user(remote.user) }));
                    break;
                }
            }
        }

-        return (conflicts.length === ccount) ? target.update({nodes: nodes}) : target;
+        return (_conflicts.length === ccount) ? target.update({nodes: nodes}) : target;
    }


@@ -90,11 +89,11 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser
                graph.parentRelations(node).length > 0;
        }

-        var ccount = conflicts.length;
+        var ccount = _conflicts.length;

        for (var i = 0; i < children.length; i++) {
-            var id = children[i],
-                node = graph.hasEntity(id);
+            var id = children[i];
+            var node = graph.hasEntity(id);

            // remove unused childNodes..
            if (targetWay.nodes.indexOf(id) === -1) {
@@ -105,29 +104,29 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser
            }

            // restore used childNodes..
-            var local = localGraph.hasEntity(id),
-                remote = remoteGraph.hasEntity(id),
-                target;
+            var local = localGraph.hasEntity(id);
+            var remote = remoteGraph.hasEntity(id);
+            var target;

-            if (option === 'force_remote' && remote && remote.visible) {
+            if (_option === 'force_remote' && remote && remote.visible) {
                updates.replacements.push(remote);

-            } else if (option === 'force_local' && local) {
+            } else if (_option === 'force_local' && local) {
                target = osmEntity(local);
                if (remote) {
                    target = target.update({ version: remote.version });
                }
                updates.replacements.push(target);

-            } else if (option === 'safe' && local && remote && local.version !== remote.version) {
+            } else if (_option === 'safe' && local && remote && local.version !== remote.version) {
                target = osmEntity(local, { version: remote.version });
                if (remote.visible) {
                    target = mergeLocation(remote, target);
                } else {
-                    conflicts.push(t('merge_remote_changes.conflict.deleted', { user: user(remote.user) }));
+                    _conflicts.push(t('merge_remote_changes.conflict.deleted', { user: user(remote.user) }));
                }

-                if (conflicts.length !== ccount) break;
+                if (_conflicts.length !== ccount) break;
                updates.replacements.push(target);
            }
        }
@@ -148,44 +147,44 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser


    function mergeMembers(remote, target) {
-        if (option === 'force_local' || _isEqual(target.members, remote.members)) {
+        if (_option === 'force_local' || _isEqual(target.members, remote.members)) {
            return target;
        }
-        if (option === 'force_remote') {
+        if (_option === 'force_remote') {
            return target.update({members: remote.members});
        }

-        conflicts.push(t('merge_remote_changes.conflict.memberlist', { user: user(remote.user) }));
+        _conflicts.push(t('merge_remote_changes.conflict.memberlist', { user: user(remote.user) }));
        return target;
    }


    function mergeTags(base, remote, target) {
        function ignoreKey(k) {
-            return _includes(dataDiscarded, k);
+            return dataDiscarded[k];
        }

-        if (option === 'force_local' || _isEqual(target.tags, remote.tags)) {
+        if (_option === 'force_local' || _isEqual(target.tags, remote.tags)) {
            return target;
        }
-        if (option === 'force_remote') {
+        if (_option === 'force_remote') {
            return target.update({tags: remote.tags});
        }

-        var ccount = conflicts.length,
-            o = base.tags || {},
-            a = target.tags || {},
-            b = remote.tags || {},
-            keys = _reject(_union(_keys(o), _keys(a), _keys(b)), ignoreKey),
-            tags = _clone(a),
-            changed = false;
+        var ccount = _conflicts.length;
+        var o = base.tags || {};
+        var a = target.tags || {};
+        var b = remote.tags || {};
+        var keys = _reject(_union(_keys(o), _keys(a), _keys(b)), ignoreKey);
+        var tags = _clone(a);
+        var changed = false;

        for (var i = 0; i < keys.length; i++) {
            var k = keys[i];

            if (o[k] !== b[k] && a[k] !== b[k]) {    // changed remotely..
                if (o[k] !== a[k]) {      // changed locally..
-                    conflicts.push(t('merge_remote_changes.conflict.tags',
+                    _conflicts.push(t('merge_remote_changes.conflict.tags',
                        { tag: k, local: a[k], remote: b[k], user: user(remote.user) }));

                } else {                  // unchanged locally, accept remote change..
@@ -199,7 +198,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser
            }
        }

-        return (changed && conflicts.length === ccount) ? target.update({tags: tags}) : target;
+        return (changed && _conflicts.length === ccount) ? target.update({tags: tags}) : target;
    }


@@ -214,18 +213,18 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser
    //  `graph.base()` --- ... --- `remoteGraph`
    //
    var action = function(graph) {
-        var updates = { replacements: [], removeIds: [] },
-            base = graph.base().entities[id],
-            local = localGraph.entity(id),
-            remote = remoteGraph.entity(id),
-            target = osmEntity(local, { version: remote.version });
+        var updates = { replacements: [], removeIds: [] };
+        var base = graph.base().entities[id];
+        var local = localGraph.entity(id);
+        var remote = remoteGraph.entity(id);
+        var target = osmEntity(local, { version: remote.version });

        // delete/undelete
        if (!remote.visible) {
-            if (option === 'force_remote') {
+            if (_option === 'force_remote') {
                return actionDeleteMultiple([id])(graph);

-            } else if (option === 'force_local') {
+            } else if (_option === 'force_local') {
                if (target.type === 'way') {
                    target = mergeChildren(target, _uniq(local.nodes), updates, graph);
                    graph = updateChildren(updates, graph);
@@ -233,7 +232,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser
                return graph.replace(target);

            } else {
-                conflicts.push(t('merge_remote_changes.conflict.deleted', { user: user(remote.user) }));
+                _conflicts.push(t('merge_remote_changes.conflict.deleted', { user: user(remote.user) }));
                return graph;  // do nothing
            }
        }
@@ -254,7 +253,7 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser

        target = mergeTags(base, remote, target);

-        if (!conflicts.length) {
+        if (!_conflicts.length) {
            graph = updateChildren(updates, graph).replace(target);
        }

@@ -263,13 +262,13 @@ export function actionMergeRemoteChanges(id, localGraph, remoteGraph, formatUser


    action.withOption = function(opt) {
-        option = opt;
+        _option = opt;
        return action;
    };


    action.conflicts = function() {
-        return conflicts;
+        return _conflicts;
    };


--- a/modules/ui/commit_changes.js
+++ b/modules/ui/commit_changes.js
@@ -15,14 +15,13 @@ import {


 export function uiCommitChanges(context) {
-    var _entityID;
    var detected = utilDetect();
+    var _entityID;


    function commitChanges(selection) {
-
-        var history = context.history(),
-            summary = history.difference().summary();
+        var history = context.history();
+        var summary = history.difference().summary();

        var container = selection.selectAll('.modal-section.commit-section')
            .data([0]);
@@ -96,14 +95,14 @@ export function uiCommitChanges(context) {


        // Download changeset link
-        var changeset = new osmChangeset().update({ id: undefined }),
-            changes = history.changes(actionDiscardTags(history.difference()));
+        var changeset = new osmChangeset().update({ id: undefined });
+        var changes = history.changes(actionDiscardTags(history.difference()));

        delete changeset.id;  // Export without chnageset_id

-        var data = JXON.stringify(changeset.osmChangeJXON(changes)),
-            blob = new Blob([data], {type: 'text/xml;charset=utf-8;'}),
-            fileName = 'changes.osc';
+        var data = JXON.stringify(changeset.osmChangeJXON(changes));
+        var blob = new Blob([data], {type: 'text/xml;charset=utf-8;'});
+        var fileName = 'changes.osc';

        var linkEnter = container.selectAll('.download-changes')
            .data([0])
@@ -166,6 +165,5 @@ export function uiCommitChanges(context) {
    };


-
    return commitChanges;
 }
--- a/test/spec/actions/discard_tags.js
+++ b/test/spec/actions/discard_tags.js
@@ -1,33 +1,33 @@
 describe('iD.actionDiscardTags', function() {
    it('discards obsolete tags from modified entities', function() {
-        var way    = iD.Way({id: 'w1', tags: {created_by: 'Potlatch'}}),
-            base   = iD.Graph([way]),
-            head   = base.replace(way.update({tags: {created_by: 'Potlatch', foo: 'bar'}})),
-            action = iD.actionDiscardTags(iD.Difference(base, head));
+        var way = iD.osmWay({ id: 'w1', tags: { created_by: 'Potlatch' } });
+        var base = iD.coreGraph([way]);
+        var head = base.replace(way.update({ tags: { created_by: 'Potlatch', foo: 'bar' } }));
+        var action = iD.actionDiscardTags(iD.coreDifference(base, head));
        expect(action(head).entity(way.id).tags).to.eql({foo: 'bar'});
    });

    it('discards obsolete tags from created entities', function() {
-        var way    = iD.Way({tags: {created_by: 'Potlatch'}}),
-            base   = iD.Graph(),
-            head   = base.replace(way),
-            action = iD.actionDiscardTags(iD.Difference(base, head));
+        var way = iD.osmWay({ tags: { created_by: 'Potlatch' } });
+        var base = iD.coreGraph();
+        var head = base.replace(way);
+        var action = iD.actionDiscardTags(iD.coreDifference(base, head));
        expect(action(head).entity(way.id).tags).to.eql({});
    });

    it('doesn\'t modify entities without obsolete tags', function() {
-        var way    = iD.Way(),
-            base   = iD.Graph(),
-            head   = base.replace(way),
-            action = iD.actionDiscardTags(iD.Difference(base, head));
+        var way = iD.osmWay();
+        var base = iD.coreGraph();
+        var head = base.replace(way);
+        var action = iD.actionDiscardTags(iD.coreDifference(base, head));
        expect(action(head).entity(way.id)).to.equal(way);
    });

    it('discards tags with empty values', function() {
-        var way    = iD.Way({tags: {lmnop: ''}}),
-            base   = iD.Graph(),
-            head   = base.replace(way),
-            action = iD.actionDiscardTags(iD.Difference(base, head));
+        var way = iD.osmWay({ tags: { lmnop: '' } });
+        var base = iD.coreGraph();
+        var head = base.replace(way);
+        var action = iD.actionDiscardTags(iD.coreDifference(base, head));
        expect(action(head).entity(way.id).tags).to.eql({});
    });
 });