Opened 14 years ago
Closed 7 years ago
#4646 closed defect (fixed)
Slowness, no undoability and exception on cancellation for "fix untagged unconnected nodes"
| Reported by: | bilbo | Owned by: | team |
|---|---|---|---|
| Priority: | normal | Milestone: | |
| Component: | Core validator | Version: | latest |
| Keywords: | performance | Cc: |
Description (last modified by )
Load attached file (manufactured test case) and run validator. You will get about 4500 untagged and unconnected nodes. Press "fix" to start fixing them. You'll see progressbar (at my machine going at about 100 nodes deleted per second)
I think 100 nodes/sec are quite slow (45 secs to delete them all), because if I just select all the 4500 nodes and press "d" they disappear in just about 2 seconds.
But what is worse: If I press cancel in middle of deleting the nodes, then some nodes will be deleted, but no "undo" item will be added to the command stack (not good, there should be undoable item with part of the nodes that were deleted) and pressing "Fix" again will cause exception in the plugin (because it is trying to delete already deleted nodes)
If I let the validator finish, it will create about 4500 sequences, each one deleting single node. I think it would be better to create one sequence and put all deleted nodes in it. This way it is impossible to simply undo the fixing, as you'll have to press "undo" 4500 times...
Attachments (2)
Change History (15)
by , 14 years ago
| Attachment: | dupfixtest0.osm.bz2 added |
|---|
by , 13 years ago
| Attachment: | 4646.patch added |
|---|
comment:1 by , 13 years ago
| Summary: | Slowness, no undoability and exception on cancellation for "fix untagged unconnected nodes" → [Patch] Slowness, no undoability and exception on cancellation for "fix untagged unconnected nodes" |
|---|
Patch attached:
- ignores nodes outside downloaded area
- For each fixable errors (no key, only created_by/watch/source), put all corresponding in a single TestError. This enables fast deletion and only 1 entry in command td (easy undoing).
Any comments?
follow-up: 3 comment:2 by , 13 years ago
No, I d don't thing grouping them is a good idea. The delay is probably caused by permanent updates. Very likely turning off signaling before fixing and enabling after fixing will solve this issue without this grouping.
follow-up: 4 comment:3 by , 13 years ago
Replying to stoecker:
Very likely turning off signaling before fixing and enabling after fixing will solve this issue without this grouping.
This wouldn't solve the undo-ability, would it?
follow-up: 5 comment:4 by , 13 years ago
Replying to simon04:
This wouldn't solve the undo-ability, would it?
No. That must be a bug. It should not be possible to have modified data without a Command entry. Maybe validator uses some very old stuff instead of proper calls to DeleteAction? Validator exists for a long time, so it may be there are code parts which no longer match the current design, but still work most of the time.
comment:5 by , 13 years ago
Replying to stoecker:
Replying to simon04:
This wouldn't solve the undo-ability, would it?
No. That must be a bug. It should not be possible to have modified data without a Command entry.
One command entry is added for each node deleted. When deleting 6k nodes, 6k commands are added which is practically undo-able.
comment:6 by , 13 years ago
This could be grouped in a SequenceCommand "Automatic Validation Fixes" or something like that. So it would be only on entry with 6000 sub-entries.
comment:7 by , 13 years ago
This could solve the problem, but would be more difficult to implement as currently fixError(TestError) is called for each error found. This method can do whatever it wants to do (i.e., does not necessarily need to add a command).
Why don't you like the grouping of unconnected nodes? IMHO one would mostly like to automatically fix these errors.
comment:8 by , 13 years ago
When they are grouped I gets much harder to handle each node individual.
Probably it would anyway be a better idea if fixError always returns a Command? This could solve a number of other possible errors as well.
BTW: When each call adds an individual Command directly to the stack, then Undo should be possible and I don't understand the original description.
follow-up: 10 comment:9 by , 13 years ago
When they are grouped I gets much harder to handle each node individual.
Is an individual handling required at all?
BTW: When each call adds an individual Command directly to the stack, then Undo should be possible and I don't understand the original description.
It is possible, but impracticable when fixing a huge amount of unconnected nodes (e.g., undoing 6000 times).
comment:10 by , 13 years ago
Replying to simon04:
Is an individual handling required at all?
Unless you exactly know what you're doing it is recommended always.
comment:11 by , 12 years ago
| Description: | modified (diff) |
|---|---|
| Summary: | [Patch] Slowness, no undoability and exception on cancellation for "fix untagged unconnected nodes" → Slowness, no undoability and exception on cancellation for "fix untagged unconnected nodes" |
A different approach is needed …
comment:12 by , 10 years ago
| Keywords: | performance added |
|---|
comment:13 by , 7 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
Everything has been fixed in the past 5 years:
- no crash, the validator is robust
- much faster: about 600 nodes per second, the whole area is cleaned up in about 5/6 seconds
- undo entries are added to the command stack
- can undo 1000 entries at once using the command stack dialog
Test case