Changeset 11627 in josm

Timestamp:

2017-02-26T00:59:32+01:00 (7 years ago)

Author:

Don-vip

Message:

fix #3346 - improve drastically the performance of fixing duplicate nodes by:

• caching data sources area computation
• moving layer invalidation from UndoRedoHandler.addNoRedraw to UndoRedoHandler.add
• avoiding any EDT call when building tag conflict dialog if it's not meant to be displayed

Location:

trunk/src/org/openstreetmap/josm

Files:

• actions/UpdateDataAction.java (modified) (1 diff)
• actions/downloadtasks/DownloadOsmTask.java (modified) (1 diff)
• data/UndoRedoHandler.java (modified) (2 diffs)
• data/osm/DataSet.java (modified) (6 diffs)
• data/validation/tests/DuplicateNode.java (modified) (1 diff)
• data/validation/tests/DuplicateRelation.java (modified) (2 diffs)
• data/validation/tests/DuplicateWay.java (modified) (4 diffs)
• gui/conflict/tags/CombinePrimitiveResolverDialog.java (modified) (6 diffs)
• gui/conflict/tags/RelationMemberConflictResolverModel.java (modified) (6 diffs)
• gui/conflict/tags/TagConflictResolverModel.java (modified) (6 diffs)
• gui/dialogs/ValidatorDialog.java (modified) (2 diffs)
• gui/dialogs/relation/ParentRelationLoadingTask.java (modified) (1 diff)
• gui/layer/OsmDataLayer.java (modified) (3 diffs)
• io/OsmReader.java (modified) (1 diff)
• io/OsmWriter.java (modified) (1 diff)
• io/OverpassDownloadReader.java (modified) (1 diff)

trunk/src/org/openstreetmap/josm/actions/UpdateDataAction.java

@@ -51 +51 @@
 
         List<Area> areas = new ArrayList<>();
-        for (DataSource ds : editLayer.data.dataSources) {
+        for (DataSource ds : editLayer.data.getDataSources()) {
             areas.add(new Area(ds.bounds.asRect()));
         }

trunk/src/org/openstreetmap/josm/actions/downloadtasks/DownloadOsmTask.java

@@ -343 +343 @@
                 }
                 // need to synthesize a download bounds lest the visual indication of downloaded area doesn't work
-                dataSet.dataSources.add(new DataSource(currentBounds != null ? currentBounds :
+                dataSet.addDataSource(new DataSource(currentBounds != null ? currentBounds :
                     new Bounds(LatLon.ZERO), "OpenStreetMap server"));
             }

trunk/src/org/openstreetmap/josm/data/UndoRedoHandler.java

@@ -52 +52 @@
         CheckParameterUtil.ensureParameterNotNull(c, "c");
         c.executeCommand();
-        c.invalidateAffectedLayers();
         commands.add(c);
         // Limit the number of commands in the undo list.
@@ -81 +80 @@
         }
         addNoRedraw(c);
+        c.invalidateAffectedLayers();
         afterAdd();
 

trunk/src/org/openstreetmap/josm/data/osm/DataSet.java

@@ -4 +4 @@
 import static org.openstreetmap.josm.tools.I18n.tr;
 
+import java.awt.geom.Area;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -25 +26 @@
 
 import org.openstreetmap.josm.Main;
+import org.openstreetmap.josm.data.Bounds;
 import org.openstreetmap.josm.data.Data;
 import org.openstreetmap.josm.data.DataSource;
@@ -127 +129 @@
     private final Object selectionLock = new Object();
 
+    private Area cachedDataSourceArea;
+    private List<Bounds> cachedDataSourceBounds;
+
     /**
      * Constructs a new {@code DataSet}.
@@ -184 +189 @@
             copyFrom.getReadLock().unlock();
         }
+    }
+
+    /**
+     * Adds a new data source.
+     * @param source data source to add
+     * @return {@code true} if the collection changed as a result of the call
+     * @since 11626
+     */
+    public synchronized boolean addDataSource(DataSource source) {
+        return addDataSources(Collections.singleton(source));
+    }
+
+    /**
+     * Adds new data sources.
+     * @param sources data sources to add
+     * @return {@code true} if the collection changed as a result of the call
+     * @since 11626
+     */
+    public synchronized boolean addDataSources(Collection<DataSource> sources) {
+        boolean changed = dataSources.addAll(sources);
+        if (changed) {
+            cachedDataSourceArea = null;
+            cachedDataSourceBounds = null;
+        }
+        return changed;
     }
 
@@ -917 +947 @@
             fireSelectionChanged();
         }
+    }
+
+    @Override
+    public synchronized Area getDataSourceArea() {
+        if (cachedDataSourceArea == null) {
+            cachedDataSourceArea = Data.super.getDataSourceArea();
+        }
+        return cachedDataSourceArea;
+    }
+
+    @Override
+    public synchronized List<Bounds> getDataSourceBounds() {
+        if (cachedDataSourceBounds == null) {
+            cachedDataSourceBounds = Data.super.getDataSourceBounds();
+        }
+        return Collections.unmodifiableList(cachedDataSourceBounds);
     }
 
@@ -1334 +1380 @@
      * @param progressMonitor The progress monitor
      */
-    public void mergeFrom(DataSet from, ProgressMonitor progressMonitor) {
+    public synchronized void mergeFrom(DataSet from, ProgressMonitor progressMonitor) {
         if (from != null) {
             new DataSetMerger(this, from).merge(progressMonitor);
-            dataSources.addAll(from.dataSources);
-            from.dataSources.clear();
+            if (!from.dataSources.isEmpty()) {
+                if (dataSources.addAll(from.dataSources)) {
+                    cachedDataSourceArea = null;
+                    cachedDataSourceBounds = null;
+                }
+                from.dataSources.clear();
+                from.cachedDataSourceArea = null;
+                from.cachedDataSourceBounds = null;
+            }
         }
     }

trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateNode.java

@@ -309 +309 @@
     @Override
     public Command fixError(TestError testError) {
-        if (!isFixable(testError)) return null;
         Collection<OsmPrimitive> sel = new LinkedList<>(testError.getPrimitives());
         Set<Node> nodes = new LinkedHashSet<>(OsmPrimitive.getFilteredList(sel, Node.class));

trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateRelation.java

@@ -293 +293 @@
         }
 
-        //Delete all relations in the list
+        // Delete all relations in the list
         relFix.remove(relationToKeep);
         commands.add(new DeleteCommand(relFix));
@@ -306 +306 @@
         // We fix it only if there is no more than one relation that is relation member.
         Collection<? extends OsmPrimitive> sel = testError.getPrimitives();
-        Set<Relation> relations = new HashSet<>();
+        Set<Relation> rels = new HashSet<>();
 
         for (OsmPrimitive osm : sel) {
             if (osm instanceof Relation) {
-                relations.add((Relation) osm);
-            }
-        }
-
-        if (relations.size() < 2)
+                rels.add((Relation) osm);
+            }
+        }
+
+        if (rels.size() < 2)
             return false;
 
         int relationsWithRelations = 0;
-        for (Relation w : relations) {
+        for (Relation w : rels) {
             List<Relation> rel = OsmPrimitive.getFilteredList(w.getReferrers(), Relation.class);
             if (!rel.isEmpty()) {

trunk/src/org/openstreetmap/josm/data/validation/tests/DuplicateWay.java

@@ -249 +249 @@
     public Command fixError(TestError testError) {
         Collection<? extends OsmPrimitive> sel = testError.getPrimitives();
-        Set<Way> ways = new HashSet<>();
+        Set<Way> wayz = new HashSet<>();
 
         for (OsmPrimitive osm : sel) {
             if (osm instanceof Way && !osm.isDeleted()) {
-                ways.add((Way) osm);
-            }
-        }
-
-        if (ways.size() < 2)
+                wayz.add((Way) osm);
+            }
+        }
+
+        if (wayz.size() < 2)
             return null;
 
         long idToKeep = 0;
-        Way wayToKeep = ways.iterator().next();
+        Way wayToKeep = wayz.iterator().next();
         // Find the way that is member of one or more relations. (If any)
         Way wayWithRelations = null;
         List<Relation> relations = null;
-        for (Way w : ways) {
+        for (Way w : wayz) {
             List<Relation> rel = OsmPrimitive.getFilteredList(w.getReferrers(), Relation.class);
             if (!rel.isEmpty()) {
@@ -284 +284 @@
 
         // Fix relations.
-        if (wayWithRelations != null && wayToKeep != wayWithRelations) {
+        if (wayWithRelations != null && relations != null && wayToKeep != wayWithRelations) {
             for (Relation rel : relations) {
                 Relation newRel = new Relation(rel);
@@ -297 +297 @@
         }
 
-        //Delete all ways in the list
-        //Note: nodes are not deleted, these can be detected and deleted at next pass
-        ways.remove(wayToKeep);
-        commands.add(new DeleteCommand(ways));
+        // Delete all ways in the list
+        // Note: nodes are not deleted, these can be detected and deleted at next pass
+        wayz.remove(wayToKeep);
+        commands.add(new DeleteCommand(wayz));
         return new SequenceCommand(tr("Delete duplicate ways"), commands);
     }
@@ -309 +309 @@
             return false;
 
-        //Do not automatically fix same ways with different tags
+        // Do not automatically fix same ways with different tags
         if (testError.getCode() != DUPLICATE_WAY) return false;
 
         // We fix it only if there is no more than one way that is relation member.
         Collection<? extends OsmPrimitive> sel = testError.getPrimitives();
-        Set<Way> ways = new HashSet<>();
+        Set<Way> wayz = new HashSet<>();
 
         for (OsmPrimitive osm : sel) {
             if (osm instanceof Way) {
-                ways.add((Way) osm);
-            }
-        }
-
-        if (ways.size() < 2)
+                wayz.add((Way) osm);
+            }
+        }
+
+        if (wayz.size() < 2)
             return false;
 
         int waysWithRelations = 0;
-        for (Way w : ways) {
+        for (Way w : wayz) {
             List<Relation> rel = OsmPrimitive.getFilteredList(w.getReferrers(), Relation.class);
             if (!rel.isEmpty()) {

trunk/src/org/openstreetmap/josm/gui/conflict/tags/CombinePrimitiveResolverDialog.java

@@ -132 +132 @@
      */
     public void setTargetPrimitive(final OsmPrimitive primitive) {
+        setTargetPrimitive(primitive, true);
+    }
+
+    /**
+     * Sets the primitive the collection of primitives is merged or combined to.
+     *
+     * @param primitive the target primitive
+     * @param updateTitle {@code true} to call {@link #updateTitle} in EDT (can be a slow operation)
+     * @since 11626
+     */
+    private void setTargetPrimitive(final OsmPrimitive primitive, boolean updateTitle) {
         this.targetPrimitive = primitive;
-        GuiHelper.runInEDTAndWait(() -> {
-            updateTitle();
-            if (primitive instanceof Way) {
-                pnlRelationMemberConflictResolver.initForWayCombining();
-            } else if (primitive instanceof Node) {
-                pnlRelationMemberConflictResolver.initForNodeMerging();
-            }
-        });
+        if (updateTitle) {
+            GuiHelper.runInEDTAndWait(this::updateTitle);
+        }
     }
 
@@ -153 +159 @@
             helpAction.setHelpTopic(ht("/Action/CombineWay#ResolvingConflicts"));
             getRootPane().putClientProperty("help", ht("/Action/CombineWay#ResolvingConflicts"));
+            pnlRelationMemberConflictResolver.initForWayCombining();
         } else if (targetPrimitive instanceof Node) {
             setTitle(tr("Conflicts when merging nodes - target node is ''{0}''", targetPrimitive
@@ -158 +165 @@
             helpAction.setHelpTopic(ht("/Action/MergeNodes#ResolvingConflicts"));
             getRootPane().putClientProperty("help", ht("/Action/MergeNodes#ResolvingConflicts"));
+            pnlRelationMemberConflictResolver.initForNodeMerging();
         }
     }
@@ -295 +303 @@
      */
     public void prepareDefaultDecisions() {
-        getTagConflictResolverModel().prepareDefaultTagDecisions();
-        getRelationMemberConflictResolverModel().prepareDefaultRelationDecisions();
+        prepareDefaultDecisions(true);
+    }
+
+    /**
+     * Prepares the default decisions for populated tag and relation membership conflicts.
+     * @param fireEvent {@code true} to call {@code fireTableDataChanged} (can be a slow operation)
+     * @since 11626
+     */
+    private void prepareDefaultDecisions(boolean fireEvent) {
+        getTagConflictResolverModel().prepareDefaultTagDecisions(fireEvent);
+        getRelationMemberConflictResolverModel().prepareDefaultRelationDecisions(fireEvent);
     }
 
@@ -501 +518 @@
             final CombinePrimitiveResolverDialog dialog = CombinePrimitiveResolverDialog.getInstance();
 
-            dialog.getTagConflictResolverModel().populate(tagsToEdit, completeWayTags.getKeysWithMultipleValues());
-            dialog.getRelationMemberConflictResolverModel().populate(parentRelations, primitives);
-            dialog.prepareDefaultDecisions();
+            dialog.getTagConflictResolverModel().populate(tagsToEdit, completeWayTags.getKeysWithMultipleValues(), false);
+            dialog.getRelationMemberConflictResolverModel().populate(parentRelations, primitives, false);
+            dialog.prepareDefaultDecisions(false);
 
             // Ensure a proper title is displayed instead of a previous target (fix #7925)
             if (targetPrimitives.size() == 1) {
-                dialog.setTargetPrimitive(targetPrimitives.iterator().next());
+                dialog.setTargetPrimitive(targetPrimitives.iterator().next(), false);
             } else {
-                dialog.setTargetPrimitive(null);
+                dialog.setTargetPrimitive(null, false);
             }
 
             // Resolve tag conflicts if necessary
             if (!dialog.isResolvedCompletely()) {
+                GuiHelper.runInEDTAndWait(() -> {
+                    dialog.getTagConflictResolverModel().fireTableDataChanged();
+                    dialog.getRelationMemberConflictResolverModel().fireTableDataChanged();
+                    dialog.updateTitle();
+                });
                 dialog.setVisible(true);
                 if (!dialog.isApplied()) {
@@ -520 +542 @@
             }
             for (OsmPrimitive i : targetPrimitives) {
-                dialog.setTargetPrimitive(i);
+                dialog.setTargetPrimitive(i, false);
                 cmds.addAll(dialog.buildResolutionCommands());
             }

trunk/src/org/openstreetmap/josm/gui/conflict/tags/RelationMemberConflictResolverModel.java

@@ -129 +129 @@
         case 4: /* decision */
             d.decide((RelationMemberConflictDecisionType) value);
-            refresh();
+            refresh(false);
             break;
         default: // Do nothing
@@ -159 +159 @@
      */
     public void populate(Collection<Relation> relations, Collection<? extends OsmPrimitive> memberPrimitives) {
+        populate(relations, memberPrimitives, true);
+    }
+
+    /**
+     * Populates the model with the relation members belonging to one of the relations in <code>relations</code>
+     * and referring to one of the primitives in <code>memberPrimitives</code>.
+     *
+     * @param relations  the parent relations. Empty list assumed if null.
+     * @param memberPrimitives the child primitives. Empty list assumed if null.
+     * @param fireEvent {@code true} to call {@code fireTableDataChanged} (can be a slow operation)
+     * @since 11626
+     */
+    void populate(Collection<Relation> relations, Collection<? extends OsmPrimitive> memberPrimitives, boolean fireEvent) {
         decisions.clear();
         relations = relations == null ? Collections.<Relation>emptyList() : relations;
@@ -169 +182 @@
         this.relations = relations;
         this.primitives = memberPrimitives;
-        refresh();
+        refresh(fireEvent);
     }
 
@@ -199 +212 @@
      */
     public void prepareDefaultRelationDecisions() {
-
+        prepareDefaultRelationDecisions(true);
+    }
+
+    /**
+     * Prepare the default decisions for the current model.
+     *
+     * Keep/delete decisions are made if every member has the same role and the members are in consecutive order within the relation.
+     * For multiple occurrences those conditions are tested stepwise for each occurrence.
+     *
+     * @param fireEvent {@code true} to call {@code fireTableDataChanged} (can be a slow operation)
+     * @since 11626
+     */
+    void prepareDefaultRelationDecisions(boolean fireEvent) {
         if (primitives.stream().allMatch(Node.class::isInstance)) {
             final Collection<OsmPrimitive> primitivesInDecisions = new HashSet<>();
@@ -256 +281 @@
         }
 
-        refresh();
+        refresh(fireEvent);
     }
 
@@ -300 +325 @@
      */
     public void refresh() {
+        refresh(true);
+    }
+
+    /**
+     * Refreshes the model state. Invoke this method to trigger necessary change
+     * events after an update of the model data.
+     * @param fireEvent {@code true} to call {@code fireTableDataChanged} (can be a slow operation)
+     * @since 11626
+     */
+    void refresh(boolean fireEvent) {
         updateNumConflicts();
-        GuiHelper.runInEDTAndWait(this::fireTableDataChanged);
+        if (fireEvent) {
+            GuiHelper.runInEDTAndWait(this::fireTableDataChanged);
+        }
     }
 

trunk/src/org/openstreetmap/josm/gui/conflict/tags/TagConflictResolverModel.java

@@ -73 +73 @@
      */
     public void rebuild() {
+        rebuild(true);
+    }
+
+    /**
+     * initializes the model from the current tags
+     * @param fireEvent {@code true} to call {@code fireTableDataChanged} (can be a slow operation)
+     * @since 11626
+     */
+    void rebuild(boolean fireEvent) {
         if (tags == null) return;
         for (String key: tags.getKeys()) {
@@ -102 +111 @@
         refreshNumConflicts();
         sort();
-        GuiHelper.runInEDTAndWait(this::fireTableDataChanged);
+        if (fireEvent) {
+            GuiHelper.runInEDTAndWait(this::fireTableDataChanged);
+        }
     }
 
@@ -113 +124 @@
      */
     public void populate(TagCollection tags, Set<String> keysWithConflicts) {
+        populate(tags, keysWithConflicts, true);
+    }
+
+    /**
+     * Populates the model with the tags for which conflicts are to be resolved.
+     *
+     * @param tags  the tag collection with the tags. Must not be null.
+     * @param keysWithConflicts the set of tag keys with conflicts
+     * @param fireEvent {@code true} to call {@code fireTableDataChanged} (can be a slow operation)
+     * @throws IllegalArgumentException if tags is null
+     * @since 11626
+     */
+    void populate(TagCollection tags, Set<String> keysWithConflicts, boolean fireEvent) {
         CheckParameterUtil.ensureParameterNotNull(tags, "tags");
         this.tags = tags;
@@ -120 +144 @@
         }
         decisions = new HashMap<>();
-        rebuild();
+        rebuild(fireEvent);
     }
 
@@ -250 +274 @@
      */
     public void prepareDefaultTagDecisions() {
+        prepareDefaultTagDecisions(true);
+    }
+
+    /**
+     * Prepare the default decisions for the current model
+     * @param fireEvent {@code true} to call {@code fireTableDataChanged} (can be a slow operation)
+     * @since 11626
+     */
+    void prepareDefaultTagDecisions(boolean fireEvent) {
         for (MultiValueResolutionDecision decision: decisions.values()) {
             List<String> values = decision.getValues();
@@ -260 +293 @@
             // else: Do not suggest to keep all values in order to reduce the wrong usage of semicolon values, see #9104!
         }
-        rebuild();
+        rebuild(fireEvent);
     }
 

trunk/src/org/openstreetmap/josm/gui/dialogs/ValidatorDialog.java

@@ -57 +57 @@
 import org.openstreetmap.josm.tools.JosmRuntimeException;
 import org.openstreetmap.josm.tools.Shortcut;
+import org.openstreetmap.josm.tools.Utils;
 import org.xml.sax.SAXException;
 
@@ -595 +596 @@
         protected void fixError(TestError error) throws InterruptedException, InvocationTargetException {
             if (error.isFixable()) {
+                long start = System.currentTimeMillis();
                 final Command fixCommand = error.getFix();
+                long phase1 = System.currentTimeMillis() - start;
+                start = System.currentTimeMillis();
                 if (fixCommand != null) {
                     SwingUtilities.invokeAndWait(() -> Main.main.undoRedo.addNoRedraw(fixCommand));
+                    if (Main.isDebugEnabled()) {
+                        long phase2 = System.currentTimeMillis() - start;
+                        Main.debug(String.format("%s fix: %s + %s",
+                                error.getTester().getClass().getSimpleName(), Utils.getDurationString(phase1),
+                                Utils.getDurationString(phase2)));
+                    }
                 }
                 // It is wanted to ignore an error if it said fixable, even if fixCommand was null

trunk/src/org/openstreetmap/josm/gui/dialogs/relation/ParentRelationLoadingTask.java

@@ -161 +161 @@
 
                 // copy the merged layer's data source info
-                getLayer().data.dataSources.addAll(referrers.dataSources);
+                getLayer().data.addDataSources(referrers.getDataSources());
                 // FIXME: this is necessary because there are dialogs listening
                 // for DataChangeEvents which manipulate Swing components on this thread.

trunk/src/org/openstreetmap/josm/gui/layer/OsmDataLayer.java

@@ -406 +406 @@
         // draw the hatched area for non-downloaded region. only draw if we're the active
         // and bounds are defined; don't draw for inactive layers or loaded GPX files etc
-        if (active && Main.pref.getBoolean("draw.data.downloaded_area", true) && !data.dataSources.isEmpty()) {
+        if (active && Main.pref.getBoolean("draw.data.downloaded_area", true) && !data.getDataSources().isEmpty()) {
             // initialize area with current viewport
             Rectangle b = mv.getBounds();
@@ -496 +496 @@
         // copy the merged layer's data source info.
         // only add source rectangles if they are not contained in the layer already.
-        for (DataSource src : from.dataSources) {
+        for (DataSource src : from.getDataSources()) {
             if (a == null || !a.contains(src.bounds.asRect())) {
-                data.dataSources.add(src);
+                data.addDataSource(src);
             }
         }
@@ -846 +846 @@
         // we'll assume that if this has no data sources
         // that it also has no borders
-        if (this.data.dataSources.isEmpty())
+        if (this.data.getDataSources().isEmpty())
             return true;
 
         boolean layerBoundsPoint = false;
-        for (DataSource src : this.data.dataSources) {
+        for (DataSource src : this.data.getDataSources()) {
             if (src.bounds.contains(coor)) {
                 layerBoundsPoint = true;

trunk/src/org/openstreetmap/josm/io/OsmReader.java

@@ -198 +198 @@
             }
             DataSource src = new DataSource(bounds, origin);
-            ds.dataSources.add(src);
+            ds.addDataSource(src);
         } else {
             throwException(tr("Missing mandatory attributes on element ''bounds''. " +

trunk/src/org/openstreetmap/josm/io/OsmWriter.java

@@ -174 +174 @@
 
     public void writeDataSources(DataSet ds) {
-        for (DataSource s : ds.dataSources) {
+        for (DataSource s : ds.getDataSources()) {
             out.println("  <bounds minlat='"
                     + s.bounds.getMin().latToString(CoordinateFormat.DECIMAL_DEGREES)

trunk/src/org/openstreetmap/josm/io/OverpassDownloadReader.java

@@ -169 +169 @@
 
         // add bounds if necessary (note that Overpass API does not return bounds in the response XML)
-        if (ds != null && ds.dataSources.isEmpty() && overpassQuery.contains("{{bbox}}")) {
+        if (ds != null && ds.getDataSources().isEmpty() && overpassQuery.contains("{{bbox}}")) {
             if (crosses180th) {
                 Bounds bounds = new Bounds(lat1, lon1, lat2, 180.0);
                 DataSource src = new DataSource(bounds, getBaseUrl());
-                ds.dataSources.add(src);
+                ds.addDataSource(src);
 
                 bounds = new Bounds(lat1, -180.0, lat2, lon2);
                 src = new DataSource(bounds, getBaseUrl());
-                ds.dataSources.add(src);
+                ds.addDataSource(src);
             } else {
                 Bounds bounds = new Bounds(lat1, lon1, lat2, lon2);
                 DataSource src = new DataSource(bounds, getBaseUrl());
-                ds.dataSources.add(src);
+                ds.addDataSource(src);
             }
         }