GeometryGraph and PreparedGeometry multi-threaded (Send) #1198

gauteh · 2024-07-08T17:27:51Z

Make GeometryGraph and PreparedGeometry Send so that it can be used by multiple threads.

Based on and depends on: #1197

geo/src/algorithm/relate/geomgraph/index/rstar_edge_set_intersector.rs

geo/src/algorithm/relate/geomgraph/geometry_graph.rs

michaelkirk · 2024-07-26T20:34:42Z

#1197 has been merged - could you do a rebase or merge to minimize the diff?

gauteh · 2024-07-27T14:02:47Z

I have rebased, but I haven't had time to work any further on this yet (feel free to make changes if you have time).

gauteh · 2024-08-16T08:09:11Z

Some of the test fails seem like numeric precision errors, they are also present on main. There is one failing test left (debug assert), which I don't know why is failing.

 3  failed: algorithm::interior_point::test::polygon_cell_test                          ▐
 thread 'algorithm::interior_point::test::polygon_cell_test' panicked at geo/src/algorit▐
 hm/relate/geomgraph/edge_end_bundle_star.rs:112:21:                                    ▐
 side_location conflict with coordinate: Coord { x: 2.0, y: 1.0 }, right_location: Insid▐
 e, current_location: Outside                                                           ▐

gauteh · 2024-08-16T09:53:13Z

The lifetime in prepared geometry is tedious to work with. Could that be replaced by a Cow or maybe a owned version of the geometrygraph?

gauteh · 2024-08-16T13:13:45Z

The lifetime in prepared geometry is tedious to work with. Could that be replaced by a Cow or maybe a owned version of the geometrygraph?

Just realized that is exactly how it is now...

gauteh · 2024-08-17T12:00:28Z

This seems to be working now.

It appears that a new geometrygraph + intersection matrix is created for every point I want to check against the preparedgeometry.

urschrei · 2024-08-17T12:57:17Z

It appears that a new geometrygraph + intersection matrix is created for every point I want to check against the preparedgeometry.

Can you expand on this and / or illustrate it with a minimal test case?

gauteh · 2024-08-17T13:41:27Z

My use case is roaring-landmask, here's a PR where I am running against these changes: gauteh/roaring-landmask#28, in https://github.com/gauteh/roaring-landmask/blob/georust-prepared/src/shapes.rs#L129 I check if a point is in any of the polygons using is_covers:

self.geom.relate(&p).is_covers()

this creates a geometrygraph of the point p and tests it against the PreparedGeometry. I believe geos has a PreparedPolygon/PreparedMultiPolygon with specializations for checking various relates towards a point.

In roaring-landmask this may be tested with e.g.: cargo test --features nightly --release -j 1 many -- --nocapture

michaelkirk · 2024-08-17T17:58:17Z

It appears that a new geometrygraph + intersection matrix is created for every point I want to check against the preparedgeometry.

That's right. The difference with PreparedGeometry is that the Polygon's geometry graph isn't created from scratch every time.

The point's graph will be created from scratch, since it's a different point every time, but it should also be a pretty trivial graph.

I check if a point is in any of the polygons using is_covers:

Note that in the case of points and polygons, polygon.relate(points).is_covers() is the same as polygon.instersects(point).

The GeometryGraph is a very general tool, built to handle all the de-9im relations for all geometry types. If you are only interested in "point intersects polygon" checks, there is likely a faster approach involving less machinery - maybe ray casting, but I don't think we have an implementation of that yet.

gauteh · 2024-08-18T06:27:24Z

It appears that a new geometrygraph + intersection matrix is created for every point I want to check against the preparedgeometry.

That's right. The difference with PreparedGeometry is that the Polygon's geometry graph isn't created from scratch every time.

The point's graph will be created from scratch, since it's a different point every time, but it should also be a pretty trivial graph.

Indeed, the tree has no members.

I check if a point is in any of the polygons using is_covers:

Note that in the case of points and polygons, polygon.relate(points).is_covers() is the same as polygon.instersects(point).

Is there any performance difference? Or exactly the same?

The GeometryGraph is a very general tool, built to handle all the de-9im relations for all geometry types. If you are only interested in "point intersects polygon" checks, there is likely a faster approach involving less machinery - maybe ray casting, but I don't think we have an implementation of that yet.

So it appears, and that makes sense (to start with). I think that this PR is mostly functional now. I had to make a few slightly messy changes to satisfy borrowing rules, without doing a much greater re-structuring. There is some cleanup to be done, but it should be a useful feature (and actually a pretty cool plus over geos, where it is not safe to share the PreparedGeometry) to other applications. Sadly, I don't think it will make much difference to my issue (#1184).

urschrei · 2024-10-28T21:14:39Z

geo/src/algorithm/relate/relate_operation.rs

-        let segment_intersector = self
-            .graph_a
-            .compute_edge_intersections(&self.graph_b, Box::new(self.line_intersector.clone()));
+        // let segment_intersector = self


I'm not clear on what's happening here.

urschrei · 2024-10-28T21:18:27Z

@michaelkirk I've rebased and tidied this up and appeased young master Clipperton – I only have one query regarding the "satisfy rust's borrowing rules section" but otherwise this hasn't added any obvious complexity to our GeomGraph machinery (though it's difficult to imagine how that would be possible…), and the Send bound will probably come in useful at some point. WDYT?

michaelkirk · 2024-10-28T21:53:28Z

geo/src/algorithm/relate/geomgraph/geometry_graph.rs

@@ -35,7 +34,7 @@ where
 {
    arg_index: usize,
    parent_geometry: GeometryCow<'a, F>,
-    tree: Option<Rc<RTree<Segment<F>>>>,
+    tree: Option<RTree<Segment<F>>>,


There are some positive changes in this PR, but there is a serious perf regression with:

relate prepared polygons time: [142.78 ms 143.14 ms 143.63 ms] change: [+192.56% +193.59% +194.76%] (p = 0.00 < 0.05) Performance has regressed.

I haven't zeroed in on the cause yet - but this line is suspect. The point of the Rc<Tree> is that it was cheap to clone. It seems like we've removed that and now have a "cache the tree and invalidate it as necessary" approach.

Note there are also some nice performance wins to the non-prepared geometry operations in this PR, but I personally don't think it's worth it as phrased.

Ooof yeah that regression is a show-stopper at the moment.

geo/src/algorithm/relate/geomgraph/index/prepared_geometry.rs

michaelkirk

whoops - didn't mean to approve yet.

michaelkirk

It's nice to get rid of the Rc<RefCell<T>>, but there is a lot of new complexity for the sake of Send. In the meanwhile, people can Send their geometry and make a PreparedGeometry on each thread.

Is there a way it can be simplified?

michaelkirk · 2024-10-29T00:52:25Z

geo/src/algorithm/relate/geomgraph/geometry_graph.rs

+        }
+    }
+
+    fn invalidate_tree(&mut self) {


This got a little more complex.

michaelkirk · 2024-10-29T00:52:46Z

geo/src/algorithm/relate/geomgraph/index/rstar_edge_set_intersector.rs

-                    edge_1,
-                    segment_1.segment_idx,
-                );
+                if segment_1.edge_idx == segment_0.edge_idx {


This got quite a lot more complex.

michaelkirk · 2024-10-29T00:53:02Z

geo/src/algorithm/relate/geomgraph/index/segment_intersector.rs

@@ -95,25 +94,85 @@ where
        false
    }

+    // Copy of `add_intersections` specialized for a single 'edge' against itself.
+    pub fn add_intersections_against_self(


This got quite a lot more complex.

michaelkirk · 2024-10-29T00:53:57Z

geo/src/algorithm/relate/relate_operation.rs

+        // this is a copy of the above functionality to satisfy rust borrowing rules.
+        // from here [...
+        let mut segment_intersector =
+            SegmentIntersector::new(Box::new(self.line_intersector.clone()), false);


This got a bit more complex.

gauteh commented Jul 8, 2024

View reviewed changes

geo/src/algorithm/relate/geomgraph/index/rstar_edge_set_intersector.rs Outdated Show resolved Hide resolved

gauteh commented Jul 8, 2024

View reviewed changes

geo/src/algorithm/relate/geomgraph/geometry_graph.rs Outdated Show resolved Hide resolved

gauteh changed the title ~~graph send~~ GeometryGraph and PreparedGeometry multi-threaded (Send) Jul 8, 2024

gauteh mentioned this pull request Jul 8, 2024

Georust: use one big prepared geometry gauteh/roaring-landmask#28

Open

gauteh force-pushed the graph-send branch from d4cbd32 to b6c76ee Compare July 27, 2024 14:02

gauteh force-pushed the graph-send branch from b6c76ee to 254cecc Compare August 15, 2024 13:26

gauteh mentioned this pull request Aug 16, 2024

performance for multipolygon contains much worse than for geos #1184

Open

gauteh force-pushed the graph-send branch from 041e661 to 5499af0 Compare August 17, 2024 11:16

gauteh added 12 commits October 28, 2024 20:54

geometry_graph: make Send (rm Rc + RefCell)

1c9ef1b

more &mut's rather than Rc<RefCells>.

40caee3

it builds...

bb464fa

rstar: handle check against same segment

da14a97

clone preparedgeom + flip add_intersections if necessary

b26ba4c

geom graph: ensure the tree is always built

b141d5e

invalidate and rebuild tree if necessary

bda1008

some checks on self interctin

067fa06

fix idx confusion

eeff476

dbg + remove a bunch of (hopefully) unnecessary invalidates

3a281f0

remove printlns

f65e815

fmt

8f47d5b

urschrei force-pushed the graph-send branch from 0da8750 to 8f47d5b Compare October 28, 2024 20:56

urschrei added 4 commits October 28, 2024 21:00

Remove unused imports

d6023df

Remove unused imports and apply a nicer TODO

7494ea3

Appease clippy

4ffeef5

Remove another unneeded line

bdf9e51

urschrei reviewed Oct 28, 2024

View reviewed changes

urschrei requested a review from michaelkirk October 28, 2024 21:15

michaelkirk reviewed Oct 28, 2024

View reviewed changes

michaelkirk approved these changes Oct 28, 2024

View reviewed changes

geo/src/algorithm/relate/geomgraph/index/prepared_geometry.rs Outdated Show resolved Hide resolved

michaelkirk requested changes Oct 28, 2024

View reviewed changes

More unneeded Arc removals

87d3ff0

michaelkirk requested changes Oct 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GeometryGraph and PreparedGeometry multi-threaded (Send) #1198

GeometryGraph and PreparedGeometry multi-threaded (Send) #1198

gauteh commented Jul 8, 2024 •

edited

Loading

michaelkirk commented Jul 26, 2024

gauteh commented Jul 27, 2024 •

edited

Loading

gauteh commented Aug 16, 2024

gauteh commented Aug 16, 2024

gauteh commented Aug 16, 2024

gauteh commented Aug 17, 2024

urschrei commented Aug 17, 2024

gauteh commented Aug 17, 2024

michaelkirk commented Aug 17, 2024 •

edited

Loading

gauteh commented Aug 18, 2024

urschrei Oct 28, 2024

urschrei commented Oct 28, 2024

michaelkirk Oct 28, 2024

urschrei Oct 28, 2024

michaelkirk left a comment

michaelkirk left a comment

michaelkirk Oct 29, 2024

michaelkirk Oct 29, 2024

michaelkirk Oct 29, 2024

michaelkirk Oct 29, 2024

GeometryGraph and PreparedGeometry multi-threaded (Send) #1198

Are you sure you want to change the base?

GeometryGraph and PreparedGeometry multi-threaded (Send) #1198

Conversation

gauteh commented Jul 8, 2024 • edited Loading

michaelkirk commented Jul 26, 2024

gauteh commented Jul 27, 2024 • edited Loading

gauteh commented Aug 16, 2024

gauteh commented Aug 16, 2024

gauteh commented Aug 16, 2024

gauteh commented Aug 17, 2024

urschrei commented Aug 17, 2024

gauteh commented Aug 17, 2024

michaelkirk commented Aug 17, 2024 • edited Loading

gauteh commented Aug 18, 2024

urschrei Oct 28, 2024

Choose a reason for hiding this comment

urschrei commented Oct 28, 2024

michaelkirk Oct 28, 2024

Choose a reason for hiding this comment

urschrei Oct 28, 2024

Choose a reason for hiding this comment

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Oct 29, 2024

Choose a reason for hiding this comment

michaelkirk Oct 29, 2024

Choose a reason for hiding this comment

michaelkirk Oct 29, 2024

Choose a reason for hiding this comment

michaelkirk Oct 29, 2024

Choose a reason for hiding this comment

gauteh commented Jul 8, 2024 •

edited

Loading

gauteh commented Jul 27, 2024 •

edited

Loading

michaelkirk commented Aug 17, 2024 •

edited

Loading