Modify

#22808 closed enhancement (fixed)

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

Reported by: GerdP Owned by: team
Priority: normal Milestone: 23.05
Component: Core Version:
Keywords: template_report performance Cc:

Description

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

Attachments (5)

22808-screen.JPG (192.1 KB ) - added by GerdP 20 months ago.
screenshot from visual-vm monitoring the mentioned action
22808.patch (1.3 KB ) - added by GerdP 20 months ago.
quick hack to work around the update selection problem
22808.2.patch (4.2 KB ) - added by taylor.smock 19 months ago.
22808.3.patch (5.7 KB ) - added by taylor.smock 19 months ago.
Avoid calling clearSelection for each removed primitive in bulk removal methods
22808.4.patch (6.0 KB ) - added by taylor.smock 19 months ago.
Fix form typos

Download all attachments as: .zip

Change History (14)

by GerdP, 20 months ago

Attachment: 22808-screen.JPG added

screenshot from visual-vm monitoring the mentioned action

comment:1 by GerdP, 20 months ago

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?

by GerdP, 20 months ago

Attachment: 22808.patch added

quick hack to work around the update selection problem

comment:2 by GerdP, 20 months ago

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

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).

comment:3 by taylor.smock, 19 months ago

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

by taylor.smock, 19 months ago

Attachment: 22808.2.patch added

comment:4 by stoecker, 19 months ago

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 :-)
Last edited 19 months ago by stoecker (previous) (diff)

in reply to:  4 comment:5 by taylor.smock, 19 months ago

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.

Last edited 19 months ago by taylor.smock (previous) (diff)

by taylor.smock, 19 months ago

Attachment: 22808.3.patch added

Avoid calling clearSelection for each removed primitive in bulk removal methods

comment:6 by GerdP, 19 months ago

The patch works well for me.

comment:7 by stoecker, 19 months ago

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

by taylor.smock, 19 months ago

Attachment: 22808.4.patch added

Fix form typos

in reply to:  7 comment:8 by taylor.smock, 19 months ago

Milestone: 23.05

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.

comment:9 by taylor.smock, 18 months ago

Resolution: fixed
Status: newclosed

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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.