[patch] SelectionListDialog performance issues

[doctau@…]

When you have a number of ways selection with different names (or any objects really) and delete a tag from them, the SelectionListDialog$SelectioListModel.tagsChanged() gets called to handle the tag changes.

Via update() the CONTENTS_CHANGED listeners are called once per selected object, including TitleUpdater. That recounts the number of nodes/ways/relations which can't actually have changed in a tagsChanged event.

Because it counts the selected objects one at a time, once per selected object, this is a N^{2 operation - for something that will be the same as it was originally.}

Ideally the updater wouldn't need to be called for something that won't have changed, but I don't know enough about JOSM's code to know if that's possible to fix.

Having the SelectionListDialog either maintain counts itself (updated when the selection changes), or a "counts dirty" flag so it only recomputes it once after being changed would be good, and reduce it to an O(n) thing rather than O(n^2).

[bastiK]

First question: Is this of practical relevance, i.e. is there a measurable speedup when commenting out these updates? As test cases you could use country extracts that can be quite big.

Generally, there certainly is much potential for optimizations in JOSM. (E.g. the painting in josm-ng is still orders of magnitude faster than in josm...)

Btw.: The josm developers don't know each part of the code and often have to invest a similar amount of time for understanding than you would, so patches are appreciated. :)

[doctau]

possible patch

[doctau]

I did a test using the dataset where I noticed the problem (about 110k ways using 650k nodes), by using Select All and then deleting one of the tags:

• before, about 8 minutes
• after commenting out the loop in getJOSMSelectionSummary(), 30 seconds (most of which is re-rendering, and updating the tags table)

I haven't given it extensive testing yet, but the attached patch make it queue a title update in the AWT event queue and not queue another one until that has been run. The result is that it only recomputes the title once, rather than once per object changed, taking about the same 30s that no doing it all takes.

[stoecker]

I don't think your approach is right. Rather than fixing the receiving end, the sender should be fixed, so it doesn't issue multiple signals when doing a batch job. One signal at the end should be enough.

[stoecker]

No improvements?

[stoecker]

Ticket #5897 has been marked as a duplicate of this ticket.

[ax]

see ticket:5897#comment:10 ff. for a profiler view of the problem, and ticket:5897#comment:12 for a first idea how to solve it.

[ax]

i agree that "the sender should be fixed, so it doesn't issue multiple signals when doing a batch job. One signal at the end should be enough" . the hard question is: how to implement this?

from what i can see, what is happening when doing a command an a selection of elements is that

• the command gets executed on every primitive of the selection, one by one (see ChangePropertyCommand::executeCommand())
• executing the command on one primitive fires command specific events to all dataset listeners, eg. tagsChanged (see OsmPrimitive::keysChangedImpl())

how can this be changed to only fire after the batch operation? what i can think of:

1. add a "batch operation" flag to OsmPrimitive and only fire events if the flag is false. set the flag before and unset it after the batch operation.
2. before the batch operation, remove all dataset listeners, after the batch operation, re-add them.

in both cases, the appropriate event(s) have to be fired from the command afterwards.

i did an experiment with method b) (had to patch DataSet to get access to its listeners), and the effect was impressive: deleting one property from a selection of 10,000 elements was almost immediately.

what do you think?

[stoecker]

There is beginUpdate() and endUpdate() in DataSet class. This should do exactly the "join all signals".

[ax]

Replying to stoecker:

There is beginUpdate() and endUpdate() in DataSet class. This should do exactly the "join all signals".

ok - this helps if i put it into ChangePropertyCommand::executeCommand(). however, doing an undo afterwards exhibits the old problem. any suggestion where to put beginUpdate()/endUpdate() to make undo/redo fast, too?

[stoecker]

Probably at multiple places in src/org/openstreetmap/josm/command/, e.g. SequenceCommand and so on. I doubt it is only one place where these are missing.

[ax]

ok, here is a patch. as suggested by stoecker, it simply wraps ChangePropertyCommand::executeCommand() and UndoRedoHandler::undo() into DataSet::{begin|end}Update().

on my test file, on a selection of 9,921 nodes and 274 ways, it speeds up deleting one property from 34 to 10 sec; undoing this deletion goes from 59 to 9 sec. the test case from ticket:5897#comment:3, which also stalls my machine, needs 17 sec and doesn't stall anymore after the patch is applied.

Replying to stoecker:

Probably at multiple places in src/org/openstreetmap/josm/command/, e.g. SequenceCommand and so on. I doubt it is only one place where these are missing.

besides ChangePropertyCommand::executeCommand(), the only additional DataSet::{begin|end}Update() wrap i did is UndoRedoHandler::undo(). the redo works in this case as it does the same as ChangePropertyCommand::executeCommand(). i don't feel confident enough to touch any more commands/whatever. feel free to do so, though :)

[ax]

i would much appreciate if someone could have a look at this patch. it fixes a not-so-uncommon stalling/crashing of josm. and i spent quite some time on it :$ ... thank you!

[bastiK]

team: come on guys, i'm not the patch monkey

ax: It can take a few days before patches are applied. However, if there is no reaction after 1-2 weeks, it is a good idea to bump the ticket.

[stoecker]

In [3910/josm]:

apply #5209 - reduce repeated signaling

[stoecker]

@ax:

Please do patches relativ to core josm directory, so that src/... is contained in path.

@bastik:

Well, team is you and me only and I'm still very busy at work ;-)

[anonymous]

Replying to stoecker:

@ax:
Please do patches relativ to core josm directory, so that src/... is contained in path.

ok, will do so in the future. thanks for applying this!

@team:
Well, team is you and me only and I'm still very busy at work ;-)

maybe you should enlarge the team? or does nobody want to be the patch monkey? :D i would not mind helping out a little.

[ax]

last comment was me

[bastiK]

Replying to anonymous:

Replying to stoecker:

@ax:
Please do patches relativ to core josm directory, so that src/... is contained in path.

ok, will do so in the future. thanks for applying this!

@team:
Well, team is you and me only and I'm still very busy at work ;-)

maybe you should enlarge the team? or does nobody want to be the patch monkey? :D

Applying a patch is easy. The hard part comes when there are problems with it...

i would not mind helping out a little.

Ok, like in many other open source projects, you send patches and the maintainer (stoecker) offers you svn access based on these contributions. (Which makes you part of the team.)

cheers, Sebastian

[stoecker]

@ax: When you look at report/8, then you see all pending patches. Usually there is some rework needed before they can be applied. Otherwise you can try and fix any bug. When you did enough to allow me better understanding of you, then you get a SVN account.