Changeset 17102 in josm for trunk

Timestamp:

2020-10-07T20:04:06+02:00 (4 years ago)

Author:

GerdP

Message:

fix #19881: Poor performance in delete action

• avoids a repeated sequential search when the flag IS_OUTSIDE is already set. The search is sequential because in this scenario the collection in ignore is a SubclassFilteredCollection which always performs a sequential search when method contains() is used.
• avoids to create change commands for ways which are in the collection of primitives which should be deleted
• use Dataset.update() in DeleteCommand

Location:

trunk/src/org/openstreetmap/josm

Files:

• command/Command.java (modified) (1 diff)
• command/DeleteCommand.java (modified) (3 diffs)
• data/osm/Way.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/command/Command.java

@@ -228 +228 @@
             if (osm.isIncomplete()) {
                 res |= IS_INCOMPLETE;
-            } else if ((osm.isOutsideDownloadArea()
+            } else if ((res & IS_OUTSIDE) == 0 && (osm.isOutsideDownloadArea()
                     || (osm instanceof Node && !osm.isNew() && osm.getDataSet() != null && osm.getDataSet().getDataSourceBounds().isEmpty()))
                             && (ignore == null || !ignore.contains(osm))) {

trunk/src/org/openstreetmap/josm/command/DeleteCommand.java

@@ -185 +185 @@
     public boolean executeCommand() {
         ensurePrimitivesAreInDataset();
-        // Make copy and remove all references (to prevent inconsistent dataset (delete referenced) while command is executed)
-        for (OsmPrimitive osm: toDelete) {
-            if (osm.isDeleted())
-                throw new IllegalArgumentException(osm + " is already deleted");
-            clonedPrimitives.put(osm, osm.save());
-
-            if (osm instanceof Way) {
-                ((Way) osm).setNodes(null);
-            } else if (osm instanceof Relation) {
-                ((Relation) osm).setMembers(null);
-            }
-        }
-
-        for (OsmPrimitive osm: toDelete) {
-            osm.setDeleted(true);
-        }
-
+
+        getAffectedDataSet().update(() -> {
+            // Make copy and remove all references (to prevent inconsistent dataset (delete referenced) while command is executed)
+            for (OsmPrimitive osm : toDelete) {
+                if (osm.isDeleted())
+                    throw new IllegalArgumentException(osm + " is already deleted");
+                clonedPrimitives.put(osm, osm.save());
+
+                if (osm instanceof Way) {
+                    ((Way) osm).setNodes(null);
+                } else if (osm instanceof Relation) {
+                    ((Relation) osm).setMembers(null);
+                }
+            }
+
+            for (OsmPrimitive osm : toDelete) {
+                osm.setDeleted(true);
+            }
+        });
         return true;
     }
@@ -209 +211 @@
         ensurePrimitivesAreInDataset();
 
-        for (OsmPrimitive osm: toDelete) {
-            osm.setDeleted(false);
-        }
-
-        for (Entry<OsmPrimitive, PrimitiveData> entry: clonedPrimitives.entrySet()) {
-            entry.getKey().load(entry.getValue());
-        }
+        getAffectedDataSet().update(() -> {
+            for (OsmPrimitive osm : toDelete) {
+                osm.setDeleted(false);
+            }
+
+            for (Entry<OsmPrimitive, PrimitiveData> entry : clonedPrimitives.entrySet()) {
+                entry.getKey().load(entry.getValue());
+            }
+        });
     }
 
@@ -427 +431 @@
         Set<Node> nodesToRemove = new HashSet<>(Utils.filteredCollection(primitivesToDelete, Node.class));
         for (Way w : waysToBeChanged) {
-            Way wnew = new Way(w);
-            wnew.removeNodes(nodesToRemove);
-            if (wnew.getNodesCount() < 2) {
+            if (primitivesToDelete.contains(w))
+                continue;
+            List<Node> remainingNodes = w.calculateRemoveNodes(nodesToRemove);
+            if (remainingNodes.size() < 2) {
                 primitivesToDelete.add(w);
             } else {
-                cmds.add(new ChangeNodesCommand(w, wnew.getNodes()));
+                cmds.add(new ChangeNodesCommand(w, remainingNodes));
             }
         }

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

@@ -364 +364 @@
         boolean locked = writeLock();
         try {
-            boolean closed = isClosed() && selection.contains(lastNode());
-            List<Node> copy = Arrays.stream(nodes)
-                    .filter(n -> !selection.contains(n))
-                    .collect(Collectors.toList());
-
-            int i = copy.size();
-            if (closed && i > 2) {
-                copy.add(copy.get(0));
-            } else if (i >= 2 && i <= 3 && copy.get(0) == copy.get(i-1)) {
-                copy.remove(i-1);
-            }
-            setNodes(removeDouble(copy));
+            setNodes(calculateRemoveNodes(selection));
             for (Node n : selection) {
                 n.clearCachedStyle();
@@ -382 +371 @@
             writeUnlock(locked);
         }
+    }
+
+    /**
+     * Calculate the remaining nodes after a removal of the given set of {@link Node nodes} from this way.
+     * @param selection The selection of nodes to remove. Ignored, if null
+     * @return result of the removal, can be empty
+     * @since 17101
+     */
+    public List<Node> calculateRemoveNodes(Set<? extends Node> selection) {
+        if (selection == null || isIncomplete())
+            return getNodes();
+        boolean closed = isClosed() && selection.contains(lastNode());
+        List<Node> copy = Arrays.stream(nodes)
+                .filter(n -> !selection.contains(n))
+                .collect(Collectors.toList());
+
+        int i = copy.size();
+        if (closed && i > 2) {
+            copy.add(copy.get(0));
+        } else if (i >= 2 && i <= 3 && copy.get(0) == copy.get(i-1)) {
+            copy.remove(i-1);
+        }
+        return removeDouble(copy);
     }