[Patch] Poor performance in validator hang when checking complex Multipolygon

[GerdP]

What steps will reproduce the problem?

1. Download relation 7379046 (attached as sample file)
2. Run validator

What is the expected result?

Quick result that MP is ok with informational messages that it has lots of shared segments

What happens instead?

Dialog shows popup "Starting Multipolygon" for > 6 minutes.

Please provide any additional information below. Attach a screenshot if possible.

VisualVM shows that the lines
sharedByPolygons.retainAll(pd1.getNodes());
sharedByPolygons.retainAll(pd2.getNodes());
in org.openstreetmap.josm.data.validation.tests.MultipolygonTest.checkPolygonsForSharedNodes()
is the problem. These calls are slow when the polygon has many shared segments.
The attached patch improves performance because it avoids the complex routine for many polygon rings.
The check is done within a few seconds now.
There is still room for improvements, it should be possibe to fill a map so that the retainAll calls are no longer needed.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2018-10-28 22:27:31 +0100 (Sun, 28 Oct 2018)
Build-Date:2018-10-28 21:33:32
Revision:14382
Relative:URL: ^/trunk

Identification: JOSM/1.5 (14382 en) Windows 10 64-Bit
OS Build number: Windows 10 Home 1803 (17134)
Memory Usage: 2818 MB / 5461 MB (2088 MB allocated, but free)
Java version: 1.8.0_191-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: \Display0 1920x1080
Maximum Screen Size: 1920x1080
Dataset consistency test: No problems found

Plugins:
+ OpeningHoursEditor (34535)
+ apache-commons (34506)
+ buildings_tools (34572)
+ download_along (34503)
+ ejml (34389)
+ geotools (34513)
+ jaxb (34506)
+ jts (34524)
+ measurement (34529)
+ merge-overlap (34664)
+ o5m (34405)
+ opendata (34698)
+ pbf (34576)
+ poly (34546)
+ reverter (34552)
+ undelete (34568)
+ utilsplugin2 (34506)

Last errors/warnings:
- W: No configuration settings found.  Using hardcoded default values for all pools.

[GerdP]

I tried to run the unit test MultipolygonTestTest in Eclipse but it seems I have to install something new? It complains about JMockit:
java.lang.ExceptionInInitializerError

at org.openstreetmap.josm.testutils.JOSMTestRules.before(JOSMTestRules.java:543)
at org.openstreetmap.josm.testutils.JOSMTestRules$CreateJosmEnvironment.evaluate(JOSMTestRules.java:640)
at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:48)
at org.junit.rules.RunRules.evaluate(RunRules.java:20)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:86)
at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:459)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:678)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:382)
at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:192)

Caused by: java.lang.IllegalStateException: JMockit didn't get initialized; please check the -javaagent JVM initialization parameter was used

at mockit.internal.startup.Startup.verifyInitialization(Startup.java:91)
at mockit.MockUp.<clinit>(MockUp.java:56)
... 19 more

[GerdP]

I was able to run the unit test via ant.
The first version of the patch did not work with negative ids, it used getId() instead of getUniqueId().
This is really a bad trap. getId() returns 0 if id is negative. I think it would be more intuitive to have a simple getter here.
Probably too late to say that...

[GerdP]

In 14408/josm:

fix #16942 - improve performance of MultipolygonTest

[Don-vip]

Replying to GerdP:

I tried to run the unit test MultipolygonTestTest in Eclipse but it seems I have to install something new? It complains about JMockit:

@ris: It's a pain to run JUnit tests from Eclipse now... :( How could we improve this?

[Don-vip]

Replying to GerdP:

The first version of the patch did not work with negative ids, it used getId() instead of getUniqueId().
This is really a bad trap. getId() returns 0 if id is negative. I think it would be more intuitive to have a simple getter here.
Probably too late to say that...

Yes, it's here for like.. forever and used everywhere. This the kind of things we expect every developers to know :)

[anonymous]

Performance related things are very appreciated. When I load polygons from the whole country for error correcting it's humping to wait sooo long.