Fix SortedPackedIntervalRtree to be thread-safe #746
Conversation
Signed-off-by: veselovs <[email protected]>
ivanveselovsky-tomtom
changed the title
|
Can you describe the core changes which allow concurrency? |
|
|
||
| private void printNode(IntervalRTreeNode node) | ||
| { | ||
| System.out.println(WKTWriter.toLineString(new Coordinate(node.min, level), new Coordinate(node.max, level))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Because this is a dead code, method never invoked.
There was a problem hiding this comment.
Just comment it out then. It may be used for debugging in the future.
There was a problem hiding this comment.
fixed.
| @@ -102,15 +102,13 @@ public int locate(Coordinate p) | |||
| private synchronized void createIndex() { | |||
| if (index == null) { | |||
| index = new IntervalIndexedGeometry(geom); | |||
| // no need to hold onto geom | |||
| geom = null; | |||
There was a problem hiding this comment.
Is there a reason the geometry can't be freed?
There was a problem hiding this comment.
The reason is that final geom field looks clearer, and the performance effect of nulling it is likely negligible.
However, it can be safely nulled, fixed in this way.
| @@ -68,7 +66,7 @@ public void insert(double min, double max, Object item) | |||
| leaves.add(new IntervalRTreeLeafNode(min, max, item)); | |||
| } | |||
|
|
|||
| private void init() | |||
| private synchronized void init() | |||
There was a problem hiding this comment.
It looks like this is the key change for concurrency?
There was a problem hiding this comment.
Yes. From practical viewpoint only this change is enough to fix the problem.
Other changes make the code formally correct from concurrency viewpoint , + add some cleanup .
| @@ -0,0 +1,67 @@ | |||
| package org.locationtech.jts.geom.prep; | |||
There was a problem hiding this comment.
This should go into the test.jts.perf.geom.prep package, because:
- it requires custom VM settings
- it is not guaranteed to reveal the issue on every run
- it may require significant execution time
There was a problem hiding this comment.
fixed.
Critical change is synchronizing method org.locationtech.jts.index.intervalrtree.SortedPackedIntervalRTree#init . Other changes make the code more correct and reliable from the concurrency viewpoint, e.g "double-checked locking" in method org.locationtech.jts.algorithm.locate.IndexedPointInAreaLocator#createIndex can only work correctly if |
dr-jts
changed the title
SortedPackedIntervalRtreeis not thread-safe, because theinitmethod is not synchronized. This ensures thatPreparedPolygonis thread-safe.This also contains some cleanup for
IndexedPointInAreaLocator.Fixes #747.
Signed-off-by: veselovs [email protected]