Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix SortedPackedIntervalRtree to be thread-safe #746

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public class IndexedPointInAreaLocator
{

private Geometry geom;
private IntervalIndexedGeometry index = null;
private volatile IntervalIndexedGeometry index = null;

/**
* Creates a new locator for a given {@link Geometry}.
Expand Down Expand Up @@ -110,7 +110,7 @@ private synchronized void createIndex() {
private static class SegmentVisitor
implements ItemVisitor
{
private RayCrossingCounter counter;
private final RayCrossingCounter counter;

public SegmentVisitor(RayCrossingCounter counter)
{
Expand All @@ -126,15 +126,17 @@ public void visitItem(Object item)

private static class IntervalIndexedGeometry
{
private boolean isEmpty = false;
private SortedPackedIntervalRTree index= new SortedPackedIntervalRTree();
private final boolean isEmpty;
private final SortedPackedIntervalRTree index= new SortedPackedIntervalRTree();

public IntervalIndexedGeometry(Geometry geom)
{
if (geom.isEmpty())
isEmpty = true;
else
else {
isEmpty = false;
init(geom);
}
}

private void init(Geometry geom)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@
import java.util.Collections;
import java.util.List;

import org.locationtech.jts.geom.Coordinate;
import org.locationtech.jts.index.ItemVisitor;
import org.locationtech.jts.io.WKTWriter;


/**
Expand All @@ -37,16 +35,16 @@
*/
public class SortedPackedIntervalRTree
{
private List leaves = new ArrayList();
private final List leaves = new ArrayList();

/**
* If root is null that indicates
* that the tree has not yet been built,
* OR nothing has been added to the tree.
* In both cases, the tree is still open for insertions.
*/
private IntervalRTreeNode root = null;
private volatile IntervalRTreeNode root = null;

public SortedPackedIntervalRTree()
{

Expand All @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this is the key change for concurrency?

Copy link
Contributor Author

@ivanveselovsky-tomtom ivanveselovsky-tomtom Jun 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 .

{
// already built
if (root != null) return;
Expand All @@ -82,7 +80,7 @@ private void init()
buildRoot();
}

private synchronized void buildRoot()
private void buildRoot()
{
if (root != null) return;
root = buildTree();
Expand All @@ -109,12 +107,12 @@ private IntervalRTreeNode buildTree()
dest = temp;
}
}
private int level = 0;

//private int level = 0;

private void buildLevel(List src, List dest)
{
level++;
//level++;
dest.clear();
for (int i = 0; i < src.size(); i += 2) {
IntervalRTreeNode n1 = (IntervalRTreeNode) src.get(i);
Expand All @@ -132,12 +130,12 @@ private void buildLevel(List src, List dest)
}
}
}
private void printNode(IntervalRTreeNode node)
{
System.out.println(WKTWriter.toLineString(new Coordinate(node.min, level), new Coordinate(node.max, level)));
}
Comment on lines -135 to -140
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is a dead code, method never invoked.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just comment it out then. It may be used for debugging in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.


// private void printNode(IntervalRTreeNode node)
// {
// System.out.println(WKTWriter.toLineString(new Coordinate(node.min, level), new Coordinate(node.max, level)));
// }

/**
* Search for intervals in the index which intersect the given closed interval
* and apply the visitor to them.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package test.jts.perf.geom.prep;

import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.stream.Collectors;
import java.util.stream.IntStream;

import org.junit.Assert;
import org.junit.Test;
import org.locationtech.jts.geom.Coordinate;
import org.locationtech.jts.geom.Geometry;
import org.locationtech.jts.geom.GeometryFactory;
import org.locationtech.jts.geom.Point;
import org.locationtech.jts.geom.prep.PreparedGeometry;
import org.locationtech.jts.geom.prep.PreparedGeometryFactory;
import org.locationtech.jts.io.WKTReader;

/**
* To achieve maximum reproducibility
* please run this test JVM option " -Djava.compiler=NONE ".
*/
public class PreparedGeometryConcurrencyBugTest {
private final PreparedGeometry preparedGeometry;

public PreparedGeometryConcurrencyBugTest() throws Exception {
Geometry geometry = new WKTReader().read("MULTIPOLYGON (((-68.3233207378872 45.95414744388297, -68.3233207378872 46.36350003948202, -67.78323905592147 46.36350003948202, -67.7831824582579 46.360613558640125, -67.78313216656608 46.35737980285657, -67.78303661235164 46.35108931804467, -67.78300844900423 46.34914504123918, -67.78284449808892 46.33717561858782, -67.78266847716758 46.325590424461865, -67.78267250050291 46.32359786763226, -67.78259505129752 46.31960370146851, -67.78254576543955 46.3175417421042, -67.78251860792597 46.31565680949507, -67.78243110038221 46.309206397103196, -67.78231542949105 46.29783343391671, -67.78229631864816 46.29618990142828, -67.78218668276001 46.28645443572722, -67.78211526855763 46.2793814121907, -67.78211325688996 46.27917320458661, -67.78208710521022 46.275919332126335, -67.78205491852746 46.27178233755782, -67.78201367934017 46.266482599074635, -67.78199859183263 46.264539328102984, -67.78198853349427 46.25005029169182, -67.78186381009857 46.23839468919735, -67.7818255884128 46.235723194528276, -67.7817923958962 46.23238684369343, -67.78174311003822 46.22733152283243, -67.78173707503521 46.22676624421646, -67.78164654998994 46.21753570710118, -67.78163850331926 46.21706397103199, -67.7815147857574 46.20460671897001, -67.7814041440354 46.19073627036815, -67.7813679340173 46.18616877891773, -67.78136692818346 46.18602494467913, -67.78130255481794 46.17465499899416, -67.78129450814725 46.173130154898416, -67.78127539730437 46.169318044659036, -67.78123616978475 46.15945986722994, -67.78119895393282 46.146595252464294, -67.7811959364313 46.14540736270368, -67.7811848722591 46.13702776101388, -67.78116173808087 46.13562663447999, -67.78116173808087 46.134921544960775, -67.78128545564273 46.13293804063568, -67.78117984308992 46.12505330919333, -67.78099577549789 46.12165359082678, -67.78096660631664 46.11688795011065, -67.78094749547375 46.11094749547374, -67.78098269965801 46.105255481794416, -67.78098873466104 46.10430597465299, -67.78091128545564 46.097327499497084, -67.78094548380608 46.09648662240997, -67.78093844296922 46.09618990142829, -67.78092737879702 46.095644739489046, -67.78084691209013 46.091684771675716, -67.78087205793602 46.088170388251854, -67.7808851337759 46.086454435727205, -67.78085596459465 46.07591329712332, -67.78081271373969 46.07353651176825, -67.78073627036814 46.06928082880708, -67.78069503118085 46.06146650573325, -67.78064574532287 46.05410883122107, -67.7806437336552 46.05378193522429, -67.78060350030175 46.04886944276805, -67.78058036612352 46.046000804667074, -67.7804385435526 46.038453027559854, -67.7805602494468 46.03127036813518, -67.78063971031986 46.026529873264934, -67.78052806276403 46.021696841681745, -67.78044659022329 46.01135184067593, -67.78046167773084 46.00444679139005, -67.78057634278817 46.00098672299336, -67.7806075236371 46.00005632669482, -67.7807020720177 45.99244518205592, -67.7807966203983 45.984834037417016, -67.78074331120499 45.984817944075644, -67.78107825387247 45.973686381009855, -67.78134479983906 45.96554817944076, -67.78128243814122 45.96297425065379, -67.78130356065178 45.95796218064777, -67.78124132526604 45.95414744388297, -68.3233207378872 45.95414744388297)))");
preparedGeometry = new PreparedGeometryFactory().create(geometry);
}

void test0() throws Exception {
List<Callable<Boolean>> tasks = IntStream.range(0, 50).mapToObj(i -> (Callable<Boolean>) this::getForPoint).collect(Collectors.toList());
ExecutorService service = Executors.newFixedThreadPool(20);
List<Future<Boolean>> list = service.invokeAll(tasks);
for (Future<Boolean> f : list) {
Boolean su = f.get();
Assert.assertTrue(su);
}
}

@Test
public void test() throws Exception {
for (int i=0; i<100; i++) {
new PreparedGeometryConcurrencyBugTest().test0();
}
}

private boolean getForPoint() {
return getFor(-67.78258, 46.13388705);
}

public boolean getFor(final double x, final double y) {
List<Boolean> speedUnits = IntStream.range(0, 5)
.mapToObj(i -> getForEx(x, y))
.collect(Collectors.toList());
Boolean firstSpeedUnit = speedUnits.get(0);
boolean allMatch = speedUnits.stream().allMatch(unit -> unit.equals(firstSpeedUnit));

Assert.assertTrue("Inconsistent result: " + speedUnits, allMatch);
return firstSpeedUnit;
}

private boolean getForEx(double x, double y) {
Point point = new GeometryFactory().createPoint(new Coordinate(x, y));
return preparedGeometry.contains(point);
}
}