Modify

Opened 6 weeks ago

Closed 5 weeks ago

#18728 closed enhancement (fixed)

[patch WIP] Join overlapping Areas is slow when combining many complex ways

Reported by: GerdP Owned by: GerdP
Priority: normal Milestone: 20.02
Component: Core Version:
Keywords: Cc:

Description

Load sample file join.osm.bz2 from #9911, select all elements and press Shift+J to "Join overlapping Areas"
Note that it takes several seconds to join the areas, on my machine ~12 secs.

One reason for this is that class DeleteCommand calculates a Hashset for thounsands of objects within a loop instead of doing this once outside of the loop. This takes ~ 60% of the complete time.

Attachments (4)

18728.patch (854 bytes) - added by GerdP 6 weeks ago.
18728.2.patch (4.4 KB) - added by GerdP 6 weeks ago.
18728.3.patch (6.5 KB) - added by GerdP 6 weeks ago.
18728.4.patch (6.1 KB) - added by GerdP 6 weeks ago.

Download all attachments as: .zip

Change History (12)

Changed 6 weeks ago by GerdP

Attachment: 18728.patch added

comment:1 Changed 6 weeks ago by GerdP

In 15874/josm:

see #18728: Join overlapping Areas is slow when combining many complex ways
Improve performance of DeleteCommand.delete() when thousands of ways are removed

Don't calculate set of nodes inside the loop which adds delete commands for the ways, the set doesn't change in this loop.

comment:2 Changed 6 weeks ago by GerdP

In 15875/josm:

see #18728: Join overlapping Areas is slow when combining many complex ways

  • use UndoRedoHandler.undo(int num) instead of calling UndoRedoHandler.undo()num times

Changed 6 weeks ago by GerdP

Attachment: 18728.2.patch added

comment:3 Changed 6 weeks ago by GerdP

The patch 18728.2.patch changes the logic when splitting ways so that only one SequenceCommand is added to undo/redo stack instead of many single SplitWayCommand instances.
This is a bit faster as it avoids to undo/redo the actions to combine them in one command.
More important: If the undo stack contains many (close to undo.max) commands it is less likely that the various temporay added commands remove anything from the stack before they are all undone again.
I think with a small value in undo.max the undo/redo logic is risky, esp. the one which catches the UserCancelException.
JoinAreasAction assumes that all tempoary added actions are on the stack when performing makeCommitsOneAction()

There are more places in the code where this might be done. Working on it...

Changed 6 weeks ago by GerdP

Attachment: 18728.3.patch added

comment:4 Changed 6 weeks ago by GerdP

18728.3.patch replaces 18728.2.patch and is a much better solution. It builds an internal undo stack and uses the same improvements as the ValidatorDialog when e.g. fixing thousands of duplicated nodes.
Unless all actions are done, nothing is added to the UndoRedoHandler.
This also allows to avoid the dirty trick when a UserCancelException is thrown.

Changed 6 weeks ago by GerdP

Attachment: 18728.4.patch added

comment:5 Changed 6 weeks ago by GerdP

Milestone: 20.02
Owner: changed from team to GerdP
Status: newassigned

18728.4.patch also improves handling of "Join areas internal error.". Data changes are reverted before the crash report is produced. I think the old code simply never worked, it left all the commands on the undo stack. I am not sure what to do with the

throw new IllegalArgumentException("Way not circular");

Should I also catch this one or change it to

throw new JosmRuntimeException("Join areas internal error.");

or should I add a new class JoinAreaException and try to write different texts for the different places where it is thrown?

comment:6 Changed 6 weeks ago by GerdP

In 15890/josm:

see #18728: Join overlapping Areas is slow when combining many complex ways

  • don't use UndoRedoHandler to store lots of intermediate steps followed by slow undo/redo actions
  • catch Exceptions and try to undo actions instead of leaving the undo stack messed up

reduces execution time by ~50% for the complex case

comment:7 Changed 5 weeks ago by Don-vip

Is this ticket fixed?

comment:8 Changed 5 weeks ago by GerdP

Resolution: fixed
Status: assignedclosed

Yes, I thought about another change but it didn't improve performance.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.