[Patch] Undoing "Paste" for ways of a route relation is very slow

[GerdP]

What steps will reproduce the problem?

1. Download members of a route relation, e.g. 14974348 to an empty layer
2. Select all way members, presss Ctrl+C to copy
3. press Ctrl+N to create new data layer
4. press Ctrl+V to paste the ways as new objects
5. undo

What is the expected result?

The action is undone within a fraction of a second

What happens instead?

CPU usage for one core is 100% for some seconds. The more ways the longer it takes.

Please provide any additional information below. Attach a screenshot if possible.

Noticed that while trying to create a simplified GPX for the relation.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2023-03-01 15:17:42 +0100 (Wed, 01 Mar 2023)
Revision:18678
Build-Date:2023-03-02 02:30:57
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18678 en) Windows 10 64-Bit
OS Build number: Windows 10 Home 2009 (19045)
Memory Usage: 554 MB / 1972 MB (287 MB allocated, but free)
Java version: 17.0.4+8-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: en_DE
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Djpackage.app-version=1.5.18531, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, --add-exports=java.base/sun.security.action=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, -Djpackage.app-path=%UserProfile%\AppData\Local\JOSM\HWConsole.exe]

Plugins:
+ OpeningHoursEditor (35924)
+ RoadSigns (36038)
+ apache-commons (36034)
+ buildings_tools (36011)
+ contourmerge (v0.1.9)
+ ejml (35924)
+ geotools (36028)
+ jackson (36034)
+ jaxb (35952)
+ jts (36004)
+ o5m (35893)
+ opendata (36025)
+ pbf (36034)
+ poly (35976)
+ reltoolbox (35976)
+ reverter (36043)
+ undelete (36011)
+ utilsplugin2 (36011)

Validator rules:
+ c:\josm\core\resources\data\validator\combinations.mapcss
+ c:\josm\core\resources\data\validator\geometry.mapcss
+ c:\josm\core\resources\data\validator\unnecessary.mapcss
+ d:\java_tools\JOSM\mygeometry.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Rules/GermanySpecific&zip=1

Last errors/warnings:
- 00000.667 W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF'
- 00000.669 W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF'
- 00001.251 E: java.security.KeyStoreException: Windows-ROOT not found. Cause: java.security.NoSuchAlgorithmException: Windows-ROOT KeyStore not available

[GerdP]

screenshot from visual-vm monitoring the mentioned action

[GerdP]

Obviously the problem is that the just pasted ways are selected and the undo action modifies this selection for each pasted object.
When you clear the selction before using undo the performacnce is good. Maybe org.openstreetmap.josm.command.AddPrimitivesCommand.undoCommand() could be changed to clear the selection first and calculate a new selction at the end?

[GerdP]

quick hack to work around the update selection problem

[GerdP]

The simple patch fixes the problem in the described scenario. No idea how many other actions suffer from the same problem when lots of objects are removed with a serious of calls of org.openstreetmap.josm.data.osm.DataSet.removePrimitive(PrimitiveId primitiveId).

[taylor.smock]

I'd rather add a bulk removal call, e.g. removePrimitives(Collection<PrimitiveId> primitiveIds).

[stoecker]

Patch looks wrong to me:

• sounds like ds.beginUpdate() and ds.endUpdate() should be used instead
• also sounds like a SequenceCommand() should be used to create only one Undo/Redo entry (which probably makes point 1 unnecessary :-)

[taylor.smock]

Replying to stoecker:

Patch looks wrong to me:

• sounds like ds.beginUpdate() and ds.endUpdate() should be used instead

Unfortunately, no. The problem lies in the removePrimitiveImpl -- it calls clearSelection, which fires an event immediately.

My patch is still wrong -- I cannot use the removePrimitiveImpl since it does call clearSelection.

EDIT: I was going to see what would be involved in making clearSelection wait for the dataset to finish being processed, but the removePrimitiveImpl code depends upon clearSelection not being delayed.

[taylor.smock]

Avoid calling clearSelection for each removed primitive in bulk removal methods

[GerdP]

The patch works well for me.

[stoecker]

Multiple typos "from"/"form"... ;-)

[taylor.smock]

Fix form typos

[taylor.smock]

Replying to stoecker:

Multiple typos "from"/"form"... ;-)

That is what I get for copying and pasting the docs from removePrimitive...

Replying to GerdP

The patch works well for me.

Excellent. It looks like other locations in the code that removes objects in loops do the same thing you suggested in attachment:22808.patch​.

[taylor.smock]

In 18724/josm:

Fix #22808: Add bulk removal method to DataSet

This fixes an issue where selecting a large number of objects that will be
removed after pasting them will cause a massive number of events to be fired.