Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#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:


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)

validator_seems_to_hang.patch (7.4 KB) - added by GerdP 2 years ago.
validator_seems_to_hang_2.patch (1.6 KB) - added by GerdP 2 years ago.
validator_seems_to_hang_3.patch (2.1 KB) - added by GerdP 2 years ago.
validator_seems_to_hang_4.patch (1.3 KB) - added by GerdP 2 years ago.
13948_v5.patch (1.3 KB) - added by simon04 2 years ago.

Download all attachments as: .zip

Change History (29)

Changed 2 years ago by GerdP

comment:1 Changed 2 years ago by Don-vip

Milestone: 16.11

comment:2 Changed 2 years ago by bastiK

Resolution: fixed
Status: newclosed

In 11235/josm:

applied #13948 - Validator seems to hang in last executed test (patch by Gerd Petermann)

comment:3 Changed 2 years ago by simon04

Resolution: fixed
Status: closedreopened

I get an exception directly after uploading a changeset:

2016-11-20 16:17:20.110 INFO: POST (3.17 kB) ...
2016-11-20 16:17:21.491 INFO: POST -> 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
	at java.util.AbstractList.remove(
	at java.util.AbstractList$Itr.remove(
	at java.util.AbstractList.removeRange(
	at java.util.AbstractList.clear(
	at org.openstreetmap.josm.gui.dialogs.validator.ValidatorTreePanel.primitivesRemoved(
	at org.openstreetmap.josm.gui.PleaseWaitRunnable.doRealRun(
	at java.util.concurrent.ThreadPoolExecutor.runWorker(
	at java.util.concurrent.ThreadPoolExecutor$

Changed 2 years ago by GerdP

comment:4 Changed 2 years ago by GerdP

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 Changed 2 years ago by simon04

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 Changed 2 years ago by GerdP

Yes, would be better. I have to find out where we keep references. Will do that tomorrow.

Changed 2 years ago by GerdP

comment:7 Changed 2 years ago by GerdP

Please review validator_seems_to_hang_3.patch. I am still not familiar with the stream api.

comment:8 Changed 2 years ago by simon04

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/

    diff --git a/src/org/openstreetmap/josm/gui/dialogs/validator/ b/src/org/openstreetmap/josm/gui/dialogs/validator/
    index 820abb9..75cb201 100644
    a b public void destroy() { 
    436436    }
    438438    @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;
    450442        }
     443        setErrorList(
     444                .filter(err -> err.getPrimitives().stream().noneMatch(event.getPrimitives()::contains))
     445                .collect(Collectors.toList()));
    451446    }
    453448    @Override public void primitivesAdded(PrimitivesAddedEvent event) {

comment:9 Changed 2 years ago by GerdP

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 Changed 2 years ago by simon04

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 Changed 2 years ago by GerdP

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 Changed 2 years ago by GerdP

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 Changed 2 years ago by GerdP

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) {

comment:14 Changed 2 years ago by anonymous

You don't like validator_seems_to_hang_3.patch? Do you want me to find a better solution?

comment:15 Changed 2 years ago by bastiK

In 11303/josm:

see #13948 - always listen to dataset events of the current edit layer

comment:16 Changed 2 years ago by bastiK

[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.

Changed 2 years ago by GerdP

comment:17 Changed 2 years ago by GerdP

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:18 in reply to:  16 Changed 2 years ago by bastiK

This doesn't work properly, see comment:16.

comment:19 Changed 2 years ago by GerdP

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 Changed 2 years ago by simon04

attachment:13948_v5.patch​ should work since it modifies the error list instead of replacing it.

Changed 2 years ago by simon04

Attachment: 13948_v5.patch added

comment:21 Changed 2 years ago by GerdP

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 Changed 2 years ago by simon04

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.

comment:23 Changed 2 years ago by simon04

Resolution: fixed
Status: reopenedclosed

In 11335/josm:

fix #13948 fix #14040 - UnsupportedOperationException in ValidatorTreePanel

comment:24 Changed 2 years ago by Don-vip

Milestone: 16.1116.12

Milestone renamed

Modify Ticket

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