Modify

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#5209 closed defect (fixed)

[patch] SelectionListDialog performance issues

Reported by: doctau@… Owned by: doctau@…
Priority: normal Milestone:
Component: Core Version: latest
Keywords: Cc:

Description

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 N2 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(n2).

Attachments (2)

SelectionListDialog-queueTitle.patch (2.5 KB) - added by doctau 9 years ago.
possible patch
josm_5209.patch (3.0 KB) - added by ax 9 years ago.

Download all attachments as: .zip

Change History (23)

comment:1 Changed 9 years ago by bastiK

Owner: changed from team to doctau@…
Status: newneedinfo

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

Changed 9 years ago by doctau

possible patch

comment:2 Changed 9 years ago by 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.

comment:3 Changed 9 years ago by 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.

comment:4 Changed 9 years ago by stoecker

No improvements?

comment:5 Changed 9 years ago by stoecker

Summary: SelectionListDialog performance issues[patch needs rework] SelectionListDialog performance issues

comment:6 Changed 9 years ago by stoecker

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

comment:7 Changed 9 years ago by 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.

comment:8 Changed 9 years ago by 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?

Last edited 9 years ago by ax (previous) (diff)

comment:9 Changed 9 years ago by stoecker

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

comment:10 in reply to:  9 Changed 9 years ago by 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?

comment:11 Changed 9 years ago by 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.

comment:12 Changed 9 years ago by 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 :)

Changed 9 years ago by ax

Attachment: josm_5209.patch added

comment:13 Changed 9 years ago by ax

Summary: [patch needs rework] SelectionListDialog performance issues[patch] SelectionListDialog performance issues

comment:14 Changed 9 years ago by 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!

comment:15 Changed 9 years ago by 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.

comment:16 Changed 9 years ago by stoecker

Resolution: fixed
Status: needinfoclosed

In [3910/josm]:

apply #5209 - reduce repeated signaling

comment:17 Changed 9 years ago by 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 ;-)

comment:18 in reply to:  17 ; Changed 9 years ago by 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.

comment:19 Changed 9 years ago by ax

last comment was me

comment:20 in reply to:  18 Changed 9 years ago by 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

comment:21 Changed 9 years ago by 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.

Last edited 9 years ago by stoecker (previous) (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain doctau@….
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.