#13948 closed enhancement (fixed)
[Patch] Validator seems to hang in last executed test
| Reported by: | GerdP | Owned by: | team |
|---|---|---|---|
| Priority: | normal | Milestone: | 16.12 |
| Component: | Core validator | Version: | |
| Keywords: | performance | Cc: |
Description
When validating a dataset with many errors/warnings the validator seems to hang in the last executed test. The problem is caused by the change in r5911, we call DataSet.addDataSetListener(DataSetListener dsl) for each single error, and this routine calls
listeners.addIfAbsent(dsl) which means an (unsuccesfull) seq. search through all previously added listeners.
This is quite slow when validator produces thousands of TestError instances.
If I got that right, we need only one listener for the list of errors, this is implemented in the attached patch.
Attachments (5)
Change History (29)
by , 8 years ago
| Attachment: | validator_seems_to_hang.patch added |
|---|
comment:1 by , 8 years ago
| Milestone: | → 16.11 |
|---|
comment:2 by , 8 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:3 by , 8 years ago
| Resolution: | fixed |
|---|---|
| Status: | closed → reopened |
I get an exception directly after uploading a changeset:
2016-11-20 16:17:20.110 INFO: POST https://api.openstreetmap.org/api/0.6/changeset/43824052/upload (3.17 kB) ... 2016-11-20 16:17:21.491 INFO: POST https://api.openstreetmap.org/api/0.6/changeset/43824052/upload -> 200 (355 B) 2016-11-20 16:17:21.491 INFO: OK 2016-11-20 16:17:21.814 SEVERE: Handled by bug report queue: java.lang.UnsupportedOperationException java.lang.UnsupportedOperationException at java.util.AbstractList.remove(AbstractList.java:161) at java.util.AbstractList$Itr.remove(AbstractList.java:374) at java.util.AbstractList.removeRange(AbstractList.java:571) at java.util.AbstractList.clear(AbstractList.java:234) at org.openstreetmap.josm.gui.dialogs.validator.ValidatorTreePanel.primitivesRemoved(ValidatorTreePanel.java:445) at org.openstreetmap.josm.data.osm.event.PrimitivesRemovedEvent.fire(PrimitivesRemovedEvent.java:25) at org.openstreetmap.josm.data.osm.DataSet.fireEventToListeners(DataSet.java:1175) at org.openstreetmap.josm.data.osm.DataSet.endUpdate(DataSet.java:1155) at org.openstreetmap.josm.io.DiffResultProcessor.postProcess(DiffResultProcessor.java:146) at org.openstreetmap.josm.io.OsmApi.uploadDiff(OsmApi.java:553) at org.openstreetmap.josm.io.OsmServerWriter.uploadChangesAsDiffUpload(OsmServerWriter.java:143) at org.openstreetmap.josm.io.OsmServerWriter.uploadOsm(OsmServerWriter.java:219) at org.openstreetmap.josm.gui.io.UploadPrimitivesTask.realRun(UploadPrimitivesTask.java:248) at org.openstreetmap.josm.gui.PleaseWaitRunnable.doRealRun(PleaseWaitRunnable.java:93) at org.openstreetmap.josm.gui.PleaseWaitRunnable.run(PleaseWaitRunnable.java:141) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617) at java.lang.Thread.run(Thread.java:745)
by , 8 years ago
| Attachment: | validator_seems_to_hang_2.patch added |
|---|
comment:4 by , 8 years ago
I found two flaws.
1) the listener was not always added
2) If the TestError instance was created using an unmodifiable collection of primitives the
clear() will produce the UnsupportedOperationException
I see two ways to fix this:
1) Change TestError to create a copy of the collection so that it is modifiable
2) Add a 2nd try+catch block in ValidatorTreePanel.primitivesRemoved()
The patch validator_seems_to_hang_2.patch implements 2), but I think I'd prefer 1) if we really have to remove the primitives.
comment:5 by , 8 years ago
What about just dropping test errors in case a contained primitive is removed? IMO it doesn't make sense to alter the contained primitives from the outside (it may be that the error is gone because of removing the primitive).
comment:6 by , 8 years ago
Yes, would be better. I have to find out where we keep references. Will do that tomorrow.
by , 8 years ago
| Attachment: | validator_seems_to_hang_3.patch added |
|---|
comment:7 by , 8 years ago
Please review validator_seems_to_hang_3.patch. I am still not familiar with the stream api.
comment:8 by , 8 years ago
Don't we end up with an additional DataSetListener being registered for every setError* call? What about simply:
-
src/org/openstreetmap/josm/gui/dialogs/validator/ValidatorTreePanel.java
diff --git a/src/org/openstreetmap/josm/gui/dialogs/validator/ValidatorTreePanel.java b/src/org/openstreetmap/josm/gui/dialogs/validator/ValidatorTreePanel.java index 820abb9..75cb201 100644
a b public void destroy() { 436 436 } 437 437 438 438 @Override public void primitivesRemoved(PrimitivesRemovedEvent event) { 439 // Remove purged primitives (fix #8639) 440 for (TestError err : errors) { 441 try { 442 err.getPrimitives().removeAll(event.getPrimitives()); 443 } catch (UnsupportedOperationException e) { 444 if (event.getPrimitives().containsAll(err.getPrimitives())) { 445 err.getPrimitives().clear(); 446 } else { 447 Main.warn(e, "Unable to remove primitives from "+err+'.'); 448 } 449 } 439 // Remove errors which list of primitives refer to purged primitives (fix #8639) 440 if (errors.isEmpty()) { 441 return; 450 442 } 443 setErrorList(errors.stream() 444 .filter(err -> err.getPrimitives().stream().noneMatch(event.getPrimitives()::contains)) 445 .collect(Collectors.toList())); 451 446 } 452 447 453 448 @Override public void primitivesAdded(PrimitivesAddedEvent event) {
comment:9 by , 8 years ago
Will not work when the validator window is already visible before opening a data layer.
The method Dataset.addDataSetListener() uses listeners.addIfAbsent() so we will never add more than one instance.
Don't know if we can rely on this fact?
comment:10 by , 8 years ago
I don't get the problem. The addDataSetListener wasn't needed before. We try to resolve the problem from comment:3? Instead of changing the primitive list of test errors, we drop test errors containing primitives from the PrimitivesRemovedEvent. Opening a data layer should not fire primitivesRemoved?
comment:11 by , 8 years ago
Without the new patch the listener was only added in the constructor of ValidatorTreePanel. This doesn't work if Main.getLayerManager().getEditDataSet() returns null. I've just noticed that the call of addDataSetListener() in the constructor is now obsolete.
comment:12 by , 8 years ago
I am not sure what should happen if you have multiple data layers. We have to make sure that the listener in ValidatorTreePanel is triggered. Maybe this still doesn't happen with the new patch.
comment:13 by , 8 years ago
OK, we have only one instance of ValidatorTreePanel and each OsmDataLayer has a field validationErrors. The method ValidatorDialog.activeOrEditLayerChanged() makes sure that ValidatorTreePanel uses the active list of errors.
Maybe it would be better to add the listener in ValidatorDialog.activeOrEditLayerChanged() like this:
if (e.getSource().getEditDataSet() != null) { e.getSource().getEditDataSet().addDataSetListener(tree); }
comment:14 by , 8 years ago
You don't like validator_seems_to_hang_3.patch? Do you want me to find a better solution?
follow-up: 18 comment:16 by , 8 years ago
[11303] should take care that listener is always registered to the right dataset.
One issue with Simons solution as implemented in validator_seems_to_hang_3.patch is that the field ValidatorTreePanel.errors is not permanent and will be overridden by ValidatorDialog.activeOrEditLayerChanged when the user switches the active layer back and forth.
by , 8 years ago
| Attachment: | validator_seems_to_hang_4.patch added |
|---|
comment:17 by , 8 years ago
OK, validator_seems_to_hang_4.patch contains only the change that removes elements from the error list instead of removing primitives from the error itself.
comment:19 by , 8 years ago
I see, the code creates a copy of the erros list instead of changing the original list.
If we really want to remove the errors from the list this should probably not happen in ValidatorTreePanel ?
comment:20 by , 8 years ago
attachment:13948_v5.patch should work since it modifies the error list instead of replacing it.
by , 8 years ago
| Attachment: | 13948_v5.patch added |
|---|
comment:21 by , 8 years ago
Yes, should work. I still don't like the situation that the error list is kept and modified in different classes, but I have no time to work on a better solution.
comment:22 by , 8 years ago
Dropping a few errors from the error list is not ideal, but iMO far better than fiddling around with reported errors and dropping a few primitives from those.
In 11235/josm: