Changeset 17102 in josm for trunk/src/org/openstreetmap/josm/command

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/command

Files:

• Command.java (modified) (1 diff)
• DeleteCommand.java (modified) (3 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));
             }
         }