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.

Sign up for GitHub

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

Jump to bottom

GeometryGraph and PreparedGeometry multi-threaded (Send) #1198

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

GeometryGraph and PreparedGeometry multi-threaded (Send) #1198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
1c9ef1b
geometry_graph: make Send (rm Rc + RefCell)
gauteh Jul 8, 2024
40caee3
more &mut's rather than Rc<RefCells>.
gauteh Jul 8, 2024
bb464fa
it builds...
gauteh Aug 15, 2024
da14a97
rstar: handle check against same segment
gauteh Aug 16, 2024
b26ba4c
clone preparedgeom + flip add_intersections if necessary
gauteh Aug 16, 2024
b141d5e
geom graph: ensure the tree is always built
gauteh Aug 17, 2024
bda1008
invalidate and rebuild tree if necessary
gauteh Aug 17, 2024
067fa06
some checks on self interctin
gauteh Aug 17, 2024
eeff476
fix idx confusion
gauteh Aug 17, 2024
3a281f0
dbg + remove a bunch of (hopefully) unnecessary invalidates
gauteh Aug 17, 2024
f65e815
remove printlns
gauteh Aug 18, 2024
8f47d5b
fmt
gauteh Aug 18, 2024
d6023df
Remove unused imports
urschrei Oct 28, 2024
7494ea3
Remove unused imports and apply a nicer TODO
urschrei Oct 28, 2024
4ffeef5
Appease clippy
urschrei Oct 28, 2024
bdf9e51
Remove another unneeded line
urschrei Oct 28, 2024
87d3ff0
More unneeded Arc removals
urschrei Oct 28, 2024
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
7 changes: 2 additions & 5 deletions geo/src/algorithm/relate/edge_end_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,6 @@
use super::geomgraph::{Edge, EdgeEnd, EdgeIntersection};
use crate::GeoFloat;

use std::cell::RefCell;
use std::rc::Rc;

/// Computes the [`EdgeEnd`]s which arise from an [`Edge`] who has had its `edge_intersections`
/// populated with self and proper [`EdgeIntersection`]s.
///
Expand All @@ -19,10 +16,10 @@ impl<F: GeoFloat> EdgeEndBuilder<F> {
}
}

pub fn compute_ends_for_edges(&self, edges: &[Rc<RefCell<Edge<F>>>]) -> Vec<EdgeEnd<F>> {
pub fn compute_ends_for_edges(&self, edges: &mut [Edge<F>]) -> Vec<EdgeEnd<F>> {
let mut list = vec![];
for edge in edges {
self.compute_ends_for_edge(&mut edge.borrow_mut(), &mut list);
self.compute_ends_for_edge(edge, &mut list);
}
list
}
Expand Down
45 changes: 29 additions & 16 deletions geo/src/algorithm/relate/geomgraph/geometry_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ use crate::HasDimensions;
use crate::{Coord, GeoFloat, GeometryCow, Line, LineString, Point, Polygon};

use rstar::{RTree, RTreeNum};
use std::cell::RefCell;
use std::rc::Rc;

/// The computation of the [`IntersectionMatrix`](crate::algorithm::relate::IntersectionMatrix) relies on the use of a
/// structure called a "topology graph". The topology graph contains nodes (CoordNode) and
Expand All @@ -35,7 +33,7 @@ where
{
arg_index: usize,
parent_geometry: GeometryCow<'a, F>,
tree: Option<Rc<RTree<Segment<F>>>>,
tree: Option<RTree<Segment<F>>>,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

use_boundary_determination_rule: bool,
has_computed_self_nodes: bool,
planar_graph: PlanarGraph<F>,
Expand All @@ -49,23 +47,33 @@ impl<F> GeometryGraph<'_, F>
where
F: GeoFloat,
{
pub(crate) fn set_tree(&mut self, tree: Rc<RTree<Segment<F>>>) {
self.tree = Some(tree);
pub(crate) fn get_tree(&mut self) -> &RTree<Segment<F>> {
self.update_tree();
self.tree.as_ref().unwrap()
}

pub(crate) fn get_or_build_tree(&self) -> Rc<RTree<Segment<F>>> {
self.tree
.clone()
.unwrap_or_else(|| Rc::new(self.build_tree()))
pub(crate) fn tree_and_edges_mut(&mut self) -> (&RTree<Segment<F>>, &mut [Edge<F>]) {
self.update_tree();
let edges = self.planar_graph.edges_mut();
(self.tree.as_ref().unwrap(), edges)
}

pub(crate) fn build_tree(&self) -> RTree<Segment<F>> {
pub(crate) fn update_tree(&mut self) {
if self.tree.is_none() {
self.tree = Some(self.build_tree());
}
}

fn invalidate_tree(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

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

This got a little more complex.

self.tree = None;
}

fn build_tree(&self) -> RTree<Segment<F>> {
let segments: Vec<Segment<F>> = self
.edges()
.iter()
.enumerate()
.flat_map(|(edge_idx, edge)| {
let edge = RefCell::borrow(edge);
let start_of_final_segment: usize = edge.coords().len() - 1;
(0..start_of_final_segment).map(move |segment_idx| {
let p1 = edge.coords()[segment_idx];
Expand All @@ -92,6 +100,7 @@ where
self.has_computed_self_nodes,
"should only be called after computing self nodes"
);
debug_assert!(self.tree.is_some());
let planar_graph = self
.planar_graph
.clone_for_arg_index(self.arg_index, arg_index);
Expand All @@ -105,11 +114,16 @@ where
}
}

pub(crate) fn edges(&self) -> &[Rc<RefCell<Edge<F>>>] {
pub(crate) fn edges(&self) -> &[Edge<F>] {
self.planar_graph.edges()
}

pub(crate) fn edges_mut(&mut self) -> &mut [Edge<F>] {
self.planar_graph.edges_mut()
}

pub(crate) fn insert_edge(&mut self, edge: Edge<F>) {
self.invalidate_tree();
self.planar_graph.insert_edge(edge)
}

Expand Down Expand Up @@ -159,7 +173,7 @@ where
}
}

fn boundary_nodes(&self) -> impl Iterator<Item = &CoordNode<F>> {
pub(crate) fn boundary_nodes(&self) -> impl Iterator<Item = &CoordNode<F>> {
self.planar_graph.boundary_nodes(self.arg_index)
}

Expand Down Expand Up @@ -352,8 +366,8 @@ where
}

pub(crate) fn compute_edge_intersections(
&self,
other: &GeometryGraph<F>,
&'a mut self,
other: &mut GeometryGraph<'a, F>,
line_intersector: Box<dyn LineIntersector<F>>,
) -> SegmentIntersector<F> {
let mut segment_intersector = SegmentIntersector::new(line_intersector, false);
Expand Down Expand Up @@ -404,7 +418,6 @@ where
let positions_and_intersections: Vec<(CoordPos, Vec<Coord<F>>)> = self
.edges()
.iter()
.map(|cell| cell.borrow())
.map(|edge| {
let position = edge
.label()
Expand Down
6 changes: 3 additions & 3 deletions geo/src/algorithm/relate/geomgraph/index/edge_set_intersector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ pub(crate) trait EdgeSetIntersector<F: GeoFloat> {
/// `segment_intersector`: the SegmentIntersector to use
fn compute_intersections_within_set(
&self,
graph: &GeometryGraph<F>,
graph: &mut GeometryGraph<F>,
check_for_self_intersecting_edges: bool,
segment_intersector: &mut SegmentIntersector<F>,
);
Expand All @@ -23,8 +23,8 @@ pub(crate) trait EdgeSetIntersector<F: GeoFloat> {
/// the intersecting edges.
fn compute_intersections_between_sets<'a>(
&self,
graph_0: &GeometryGraph<'a, F>,
graph_1: &GeometryGraph<'a, F>,
graph_0: &mut GeometryGraph<'a, F>,
graph_1: &mut GeometryGraph<'a, F>,
segment_intersector: &mut SegmentIntersector<F>,
);
}
6 changes: 3 additions & 3 deletions geo/src/algorithm/relate/geomgraph/index/prepared_geometry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use crate::GeometryCow;
use crate::{GeoFloat, Relate};

use std::cell::RefCell;
use std::rc::Rc;

use rstar::{RTree, RTreeNum};

Expand All @@ -26,6 +25,7 @@ use rstar::{RTree, RTreeNum};
/// assert!(prepared_polygon.relate(&contained_line).is_contains());
///
/// ```
#[derive(Clone)]
pub struct PreparedGeometry<'a, F: GeoFloat + RTreeNum = f64> {
geometry_graph: GeometryGraph<'a, F>,
}
Expand All @@ -38,7 +38,7 @@ mod conversions {
Geometry, GeometryCollection, Line, LineString, MultiLineString, MultiPoint, MultiPolygon,
Point, Polygon, Rect, Triangle,
};
use std::rc::Rc;
use std::sync::Arc;

impl<'a, F: GeoFloat> From<Point<F>> for PreparedGeometry<'a, F> {
fn from(point: Point<F>) -> Self {
Expand Down Expand Up @@ -156,7 +156,7 @@ mod conversions {
impl<'a, F: GeoFloat> From<GeometryCow<'a, F>> for PreparedGeometry<'a, F> {
fn from(geometry: GeometryCow<'a, F>) -> Self {
let mut geometry_graph = GeometryGraph::new(0, geometry);
geometry_graph.set_tree(Rc::new(geometry_graph.build_tree()));
geometry_graph.update_tree(); // TODO: maybe unecessary

// TODO: don't pass in line intersector here - in theory we'll want pluggable line intersectors
// and the type (Robust) shouldn't be hard coded here.
Expand Down
68 changes: 45 additions & 23 deletions geo/src/algorithm/relate/geomgraph/index/rstar_edge_set_intersector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,42 +15,64 @@ where
{
fn compute_intersections_within_set(
&self,
graph: &GeometryGraph<F>,
graph: &mut GeometryGraph<F>,
check_for_self_intersecting_edges: bool,
segment_intersector: &mut SegmentIntersector<F>,
) {
let edges = graph.edges();

let tree = graph.get_or_build_tree();
for (segment_0, segment_1) in tree.intersection_candidates_with_other_tree(&tree) {
let (tree, edges) = graph.tree_and_edges_mut();
for (segment_0, segment_1) in tree.intersection_candidates_with_other_tree(tree) {
if check_for_self_intersecting_edges || segment_0.edge_idx != segment_1.edge_idx {
let edge_0 = &edges[segment_0.edge_idx];
let edge_1 = &edges[segment_1.edge_idx];
segment_intersector.add_intersections(
edge_0,
segment_0.segment_idx,
edge_1,
segment_1.segment_idx,
);
if segment_1.edge_idx == segment_0.edge_idx {
Copy link
Member

Choose a reason for hiding this comment

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

This got quite a lot more complex.

let edge_0 = &mut edges[segment_0.edge_idx];
segment_intersector.add_intersections_against_self(
edge_0,
segment_0.segment_idx,
segment_1.segment_idx,
);
} else {
// TODO: use get_many_mut when available.
let mi = segment_0.edge_idx.min(segment_1.edge_idx);
let mx = segment_0.edge_idx.max(segment_1.edge_idx);

assert!(mx > mi);

let (e0, e1) = edges.split_at_mut(mi + 1);

let edge_0 = &mut e0[mi];
let edge_1 = &mut e1[mx - (mi + 1)];

if segment_0.edge_idx > segment_1.edge_idx {
segment_intersector.add_intersections(
edge_1,
segment_0.segment_idx,
edge_0,
segment_1.segment_idx,
);
} else {
segment_intersector.add_intersections(
edge_0,
segment_0.segment_idx,
edge_1,
segment_1.segment_idx,
);
}
}
}
}
}

fn compute_intersections_between_sets<'a>(
&self,
graph_0: &GeometryGraph<'a, F>,
graph_1: &GeometryGraph<'a, F>,
graph_0: &mut GeometryGraph<'a, F>,
graph_1: &mut GeometryGraph<'a, F>,
segment_intersector: &mut SegmentIntersector<F>,
) {
let edges_0 = graph_0.edges();
let edges_1 = graph_1.edges();

let tree_0 = graph_0.get_or_build_tree();
let tree_1 = graph_1.get_or_build_tree();
let (tree_0, edges_0) = graph_0.tree_and_edges_mut();
let (tree_1, edges_1) = graph_1.tree_and_edges_mut();

for (segment_0, segment_1) in tree_0.intersection_candidates_with_other_tree(&tree_1) {
let edge_0 = &edges_0[segment_0.edge_idx];
let edge_1 = &edges_1[segment_1.edge_idx];
for (segment_0, segment_1) in tree_0.intersection_candidates_with_other_tree(tree_1) {
let edge_0 = &mut edges_0[segment_0.edge_idx];
let edge_1 = &mut edges_1[segment_1.edge_idx];
segment_intersector.add_intersections(
edge_0,
segment_0.segment_idx,
Expand Down
Loading
Loading