From dac753c4ea3d3433c3201148bb885262186c92fe Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Sun, 4 Feb 2018 14:57:26 -0500 Subject: [PATCH] Speedup hot code in actionDiscardTags (re: #4611) --- data/discarded.json | 89 ++++++++++---------- modules/actions/discard_tags.js | 22 +++-- modules/actions/merge_remote_changes.js | 105 ++++++++++++------------ modules/ui/commit_changes.js | 18 ++-- test/spec/actions/discard_tags.js | 32 ++++---- 5 files changed, 135 insertions(+), 131 deletions(-) diff --git a/data/discarded.json b/data/discarded.json index 58d998a91..61dc16815 100644 --- 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 + } } diff --git a/modules/actions/discard_tags.js b/modules/actions/discard_tags.js index 1120e7172..a2e9b65e4 100644 --- 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 })); } } diff --git a/modules/actions/merge_remote_changes.js b/modules/actions/merge_remote_changes.js index 71412bf00..7503617ea 100644 --- 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; }; diff --git a/modules/ui/commit_changes.js b/modules/ui/commit_changes.js index 195a7fd6c..9d4f3effe 100644 --- 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; } diff --git a/test/spec/actions/discard_tags.js b/test/spec/actions/discard_tags.js index 863c6d18d..9ebd9ef1a 100644 --- 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({}); }); });