Back BooleanOps with i_overlay to avoid crashes from floating point collapse #1234

michaelkirk · 2024-10-24T23:36:25Z

I agree to follow the project's code of conduct.
I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

This PR replaces our existing BooleanOps implementation with one backed by i_overlay.

We've had several reports of crashes in our BooleanOps implementation over the years. Some of them have been addressed, but there remains a long tail of crashes.

The root of the issue (as I understand it) is that floating point rounding errors break the invariants of our sweep line algorithm. After a couple years, it seems like a "simple" resolution is not in sight.

In the meanwhile, this i_overlay crate has appeared. It is well documented (seriously, check out the algorithm description and interactive demo). Additionally, the maintainer @NailxSharipov has been responsive at answering questions and even making some changes to match our needs.

Performance

Performance is interesting. For our benches the new implementation is faster for smaller inputs and slower for larger inputs. The break even size is about 2k points. My guess is that this will be a win for most people, but it's unfortunate that it's a slowdown for large inputs.

(obligatory disclaimer that our benches might not accurately reflect real world use cases)

Apparently there are some performance oriented changes in the works upstream: iShape-Rust/iOverlay#6 (comment)

`cargo bench` output with this PR

Circular polygon boolean-ops/bops::intersection/256
                        time:   [504.81 µs 506.86 µs 508.31 µs]
                        change: [-45.875% -45.511% -45.214%] (p = 0.00 < 0.05)
                        Performance has improved.
Circular polygon boolean-ops/bops::union/256
                        time:   [501.04 µs 502.43 µs 504.02 µs]
                        change: [-54.190% -53.778% -53.332%] (p = 0.00 < 0.05)
                        Performance has improved.
Circular polygon boolean-ops/bops::intersection/512
                        time:   [1.5031 ms 1.5140 ms 1.5224 ms]
                        change: [-27.932% -27.187% -26.436%] (p = 0.00 < 0.05)
                        Performance has improved.
Circular polygon boolean-ops/bops::union/512
                        time:   [1.4961 ms 1.5018 ms 1.5100 ms]
                        change: [-38.724% -38.322% -37.902%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high mild
Circular polygon boolean-ops/bops::intersection/1024
                        time:   [4.6707 ms 4.6775 ms 4.6882 ms]
                        change: [-3.9197% -3.2149% -2.5433%] (p = 0.00 < 0.05)
                        Performance has improved.
Circular polygon boolean-ops/bops::union/1024
                        time:   [4.6747 ms 4.6760 ms 4.6779 ms]
                        change: [-14.886% -14.512% -14.110%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 10 measurements (20.00%)
  1 (10.00%) high mild
  1 (10.00%) high severe
Circular polygon boolean-ops/bops::intersection/2048
                        time:   [13.782 ms 13.893 ms 14.076 ms]
                        change: [+6.3261% +7.4124% +8.6468%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/bops::union/2048
                        time:   [13.806 ms 13.849 ms 13.896 ms]
                        change: [-3.1394% -2.3106% -1.5126%] (p = 0.00 < 0.05)
                        Performance has improved.
Circular polygon boolean-ops/bops::intersection/4096
                        time:   [52.305 ms 52.551 ms 52.777 ms]
                        change: [+46.829% +47.830% +48.861%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/bops::union/4096
                        time:   [51.539 ms 51.934 ms 52.353 ms]
                        change: [+32.417% +33.518% +34.717%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/bops::intersection/8192
                        time:   [193.01 ms 194.36 ms 195.83 ms]
                        change: [+58.035% +63.068% +68.310%] (p = 0.00 < 0.05)
                        Performance has regressed.
Circular polygon boolean-ops/bops::union/8192
                        time:   [191.69 ms 193.30 ms 195.13 ms]
                        change: [+40.478% +43.519% +46.983%] (p = 0.00 < 0.05)
                        Performance has regressed.

Alternatives

Another solution is proposed by @RobWalt, implementing boolean operations by "triangulating" polygons and then "stitching" that triangulation back together. This too avoids crashing in all known test cases. Unfortunately it's quite a bit slower. I haven't seen it run on the additional JTS test cases we're now using, but getting that assurance of correctness would be good if we were to pursue that route.

`cargo bench` output with SpadeBoolops from #1089

$ cargo bench --bench="*" -p geo-bool-ops-benches -- --baseline=main                                                                                               
Circular polygon boolean-ops/bops::intersection/256
                        time:   [136.37 ms 138.60 ms 139.89 ms]
                        change: [+14325% +14657% +15026%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 10 measurements (20.00%)
  1 (10.00%) low mild
  1 (10.00%) high mild
Benchmarking Circular polygon boolean-ops/bops::union/256: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 7.7s or enable flat sampling.
Circular polygon boolean-ops/bops::union/256
                        time:   [137.70 ms 139.56 ms 141.00 ms]
                        change: [+12369% +12674% +13012%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 10 measurements (20.00%)
  1 (10.00%) low mild
  1 (10.00%) high mild
Benchmarking Circular polygon boolean-ops/bops::intersection/512: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 11.0s.
Circular polygon boolean-ops/bops::intersection/512
                        time:   [1.0931 s 1.0947 s 1.0957 s]
                        change: [+52157% +52477% +52789%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) low severe
Benchmarking Circular polygon boolean-ops/bops::union/512: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 11.2s.
Circular polygon boolean-ops/bops::union/512
                        time:   [1.0980 s 1.1025 s 1.1086 s]
                        change: [+44791% +45106% +45454%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 3 outliers among 10 measurements (30.00%)
  1 (10.00%) low mild
  2 (20.00%) high severe
Benchmarking Circular polygon boolean-ops/bops::intersection/1024: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 93.0s.
Circular polygon boolean-ops/bops::intersection/1024
                        time:   [9.4214 s 9.4391 s 9.4570 s]
                        change: [+193659% +195047% +196361%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 10 measurements (20.00%)
  1 (10.00%) low mild
  1 (10.00%) high mild
Benchmarking Circular polygon boolean-ops/bops::union/1024: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 93.6s.
Circular polygon boolean-ops/bops::union/1024sanitizer_cfi_generalize_pointers`, `sanitizer_cfi_normalize_integers`, `target_abi`, `target_arch`, `target_endian`, `t
                        time:   [9.4143 s 9.4242 s 9.4352 s]dor`, `test`, `ub_checks`, `unix`, and `windows`
                        change: [+171260% +172012% +172732%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking Circular polygon boolean-ops/bops::intersection/2048: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 472.4s.
Circular polygon boolean-ops/bops::intersection/2048
                        time:   [47.246 s 47.308 s 47.378 s]
                        change: [+362795% +365337% +368048%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking Circular polygon boolean-ops/bops::union/2048: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 471.8s.
...

FIXES #913, #948, #976, #1053, #1064, #1103, #1104, #1127, #1174, #1189, #1193

Also a little progress towards #1203, in that you can now mix and match Polygon x MultiPolygon, but you still can't generically "bop" Point x LineString or LineString x LineString.

michaelkirk · 2024-10-25T00:23:18Z

@rmanoka - I'd love to get your input on this.

michaelkirk · 2024-10-25T00:30:51Z

jts-test-runner/src/input.rs

@@ -215,6 +215,11 @@ pub(crate) enum Operation {
        op: BoolOp,
        expected: Geometry<f64>,
    },
+    ClipOp {


JTS doesn't have a special "clip" operation. Instead it supports boolean operations on LineString x Polygons, so I've mapped those to our BooleanOp::clip method to get some more test coverage.

RobWalt · 2024-10-25T06:16:16Z

Looks great! Maybe we can squeeze out a bit more performance in the future but for now I'd say getting a non panicking version of the algorithms would be far more important and a big relief for a lot of people. I'll review it as soon as I'll be back home!

RobWalt

Excellect! LGTM!

TotalKrill · 2024-10-25T08:08:38Z

Been testing this on data where I basically perform thousands of union and differences in loops, working with geo map data, where I get every road segment as a polygon, then union them all into one large Multipolygon struct.

Then with this I want the difference of these multipolygon structs. To create a puzzle like map. Which for now I have not been able to crash this after the hotfix to iOverlay :)

dabreegster · 2024-10-25T08:09:23Z

Just wanted to say a huge thank you to @michaelkirk, @NailxSharipov, @RobWalt, and everyone else who's been pushing this forward. Stable boolean ops are incredibly useful to a bunch of downstream projects!

RobWalt · 2024-10-25T08:26:18Z

geo/src/algorithm/bool_ops/i_overlay_integration.rs

+        type StringOverlayType = F64StringOverlay;
+    }
+
+    impl super::BoolOpsCoord<f64> for F64Point {


Maybe inlining all of these method impls would be a good idea to get a bit more performance

Typically I don't like to back seat drive the compiler, but there are some nice boosts here.

re: https://std-dev-guide.rust-lang.org/policy/inline.html

Maybe this falls under "public, small, non-generic functions."

cargo bench from inline

Circular polygon boolean-ops/bops::intersection/256 time: [456.98 µs 461.66 µs 467.85 µs] change: [-1.5757% -0.8205% +0.0412%] (p = 0.11 > 0.05) No change in performance detected. Found 1 outliers among 10 measurements (10.00%) 1 (10.00%) high severe Circular polygon boolean-ops/bops::union/256 time: [457.72 µs 458.64 µs 459.76 µs] change: [-2.0602% -1.7872% -1.5125%] (p = 0.00 < 0.05) Performance has improved. Circular polygon boolean-ops/bops::intersection/512 time: [1.3860 ms 1.3888 ms 1.3916 ms] change: [-10.975% -10.623% -10.346%] (p = 0.00 < 0.05) Performance has improved. Circular polygon boolean-ops/bops::union/512 time: [1.3902 ms 1.3922 ms 1.3943 ms] change: [-11.574% -10.967% -10.469%] (p = 0.00 < 0.05) Performance has improved. Found 1 outliers among 10 measurements (10.00%) 1 (10.00%) high mild Circular polygon boolean-ops/bops::intersection/1024 time: [4.2887 ms 4.2972 ms 4.3112 ms] change: [-8.7721% -7.8505% -7.1175%] (p = 0.00 < 0.05) Performance has improved. Found 2 outliers among 10 measurements (20.00%) 1 (10.00%) high mild 1 (10.00%) high severe Circular polygon boolean-ops/bops::union/1024 time: [4.2956 ms 4.3075 ms 4.3290 ms] change: [-7.9012% -7.3024% -6.5849%] (p = 0.00 < 0.05) Performance has improved. Found 2 outliers among 10 measurements (20.00%) 1 (10.00%) high mild 1 (10.00%) high severe Circular polygon boolean-ops/bops::intersection/2048 time: [14.081 ms 14.098 ms 14.122 ms] change: [+0.1742% +0.4568% +0.8040%] (p = 0.01 < 0.05) Change within noise threshold. Found 1 outliers among 10 measurements (10.00%) 1 (10.00%) high severe Circular polygon boolean-ops/bops::union/2048 time: [14.078 ms 14.116 ms 14.160 ms] change: [-0.2573% +0.3188% +0.8433%] (p = 0.30 > 0.05) No change in performance detected. Circular polygon boolean-ops/bops::intersection/4096 time: [50.853 ms 50.954 ms 51.038 ms] change: [-0.8439% -0.2164% +0.4735%] (p = 0.54 > 0.05) No change in performance detected. Found 2 outliers among 10 measurements (20.00%) 1 (10.00%) high mild 1 (10.00%) high severe Circular polygon boolean-ops/bops::union/4096 time: [50.889 ms 50.969 ms 51.048 ms] change: [-0.7440% -0.2436% +0.3009%] (p = 0.40 > 0.05) No change in performance detected. Found 2 outliers among 10 measurements (20.00%) 2 (20.00%) high severe Circular polygon boolean-ops/bops::intersection/8192 time: [188.02 ms 188.28 ms 188.58 ms] change: [-1.2461% -1.0530% -0.8445%] (p = 0.00 < 0.05) Change within noise threshold. Circular polygon boolean-ops/bops::union/8192 time: [187.91 ms 188.13 ms 188.39 ms] change: [-1.1870% -1.0354% -0.8655%] (p = 0.00 < 0.05) Change within noise threshold.

frewsxcv · 2024-10-25T19:50:39Z

geo/src/algorithm/sweep/proc.rs

-        //         debug!("\t{geom:?}", geom = aseg.0.geom());
-        //         false
-        //     });
-        // }


Instead of commenting it out, what if we put this logic behind a debug assertions flag? https://stackoverflow.com/a/39205417

This commented out code was altogether deleted, so I don't think there's anything further for us to do here.

Wow I completely misread the diff

frewsxcv · 2024-10-25T19:52:16Z

geo/src/algorithm/bool_ops/i_overlay_integration.rs

+        overlay::F64Overlay,
+        string::{F64StringGraph, F64StringOverlay},
+    };
+    use i_overlay::i_float::f64_point::F64Point;


I wonder if they'd be open to incorporating num-traits in the future. Might simplify some of this?

Very likely they just didnt know about it, or there were some issues using it, the maintainer seems very nice so you could probably just ask them over at their github

https://github.com/iShape-Rust/iOverlay/issues

frewsxcv

🎉 🎉 🎉

frewsxcv · 2024-10-28T01:01:03Z

Any blockers to merging @michaelkirk ?

We've had several reports of crashes in our BooleanOps implementation over the years. Some of them have been addressed, but there remains a long tail of crashes. FIXES #913, #948, #976, #1053, #1064, #1103, #1104, #1127, #1174, #1189, 1193 The root of the issue (as I understand it) is that floating point rounding errors break the invariants of our sweep line algorithm. After a couple years, it seems like a "simple" resolution is not in sight. In the meanwhile, the i_overlay crate appeared. It uses a strategy that maps floating point geometries to a scaled fixed point grid, which nicely avoids the kind of problems we were having. Related changes included: Included are some tests that cover all reports in the issue tracker of the existing BoolOps panic'ing JTS supports Bops between all geometry types. We support a more limited set: 1. Two 2-Dimensional geometries `boolean_op`. 2. A 1-Dimenstinoal geometry with a 2-D geometry, which we call `clip`. So this maps JTS's Line x Poly intersection tests to our Clip method. - rm unused sweep code now that old boolops is gone I marked a couple fields as `allow(unused)` because they are used for printing debug repr in tests. Speed up benches by only benching local boolop impl by default

michaelkirk · 2024-10-28T17:18:01Z

rebased and squashed, since my commits were kind of a mess.

Any blockers to merging @michaelkirk ?

I'll queue for merge now. 👍

michaelkirk force-pushed the mkirk/i-overlay branch 2 times, most recently from 65b936c to 02487c1 Compare October 25, 2024 00:05

michaelkirk marked this pull request as ready for review October 25, 2024 00:22

michaelkirk commented Oct 25, 2024

View reviewed changes

michaelkirk mentioned this pull request Oct 25, 2024

change the boolops trait to return result #1233

Closed

2 tasks

michaelkirk force-pushed the mkirk/i-overlay branch from fcb660b to 8f3105d Compare October 25, 2024 01:09

RobWalt approved these changes Oct 25, 2024

View reviewed changes

RobWalt mentioned this pull request Oct 25, 2024

Implement SpadeBoolops trait #1089

Closed

6 tasks

RobWalt reviewed Oct 25, 2024

View reviewed changes

frewsxcv reviewed Oct 25, 2024

View reviewed changes

frewsxcv approved these changes Oct 25, 2024

View reviewed changes

TotalKrill approved these changes Oct 28, 2024

View reviewed changes

michaelkirk force-pushed the mkirk/i-overlay branch from 2b0f994 to 4915dfc Compare October 28, 2024 17:17

michaelkirk enabled auto-merge October 28, 2024 17:18

michaelkirk added this pull request to the merge queue Oct 28, 2024

Merged via the queue into main with commit 818344b Oct 28, 2024
18 checks passed

michaelkirk deleted the mkirk/i-overlay branch October 28, 2024 17:23

urschrei mentioned this pull request Oct 28, 2024

Relax restrictions on bool ops between polygon and multipolygon #1205

Closed

2 tasks

This was referenced Oct 28, 2024

Panic in polygon difference #913

Closed

Panic: could not compare segments #948

Closed

Panic in BooleanOps::difference for Multipolygon #976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Back BooleanOps with i_overlay to avoid crashes from floating point collapse #1234

Back BooleanOps with i_overlay to avoid crashes from floating point collapse #1234

michaelkirk commented Oct 24, 2024 •

edited

Loading

michaelkirk commented Oct 25, 2024

michaelkirk Oct 25, 2024

RobWalt commented Oct 25, 2024

RobWalt left a comment

TotalKrill commented Oct 25, 2024

dabreegster commented Oct 25, 2024

RobWalt Oct 25, 2024 •

edited

Loading

michaelkirk Oct 25, 2024 •

edited

Loading

frewsxcv Oct 25, 2024

michaelkirk Oct 25, 2024

frewsxcv Oct 25, 2024

frewsxcv Oct 25, 2024

TotalKrill Oct 25, 2024 •

edited

Loading

frewsxcv left a comment

frewsxcv commented Oct 28, 2024

michaelkirk commented Oct 28, 2024

Back BooleanOps with i_overlay to avoid crashes from floating point collapse #1234

Back BooleanOps with i_overlay to avoid crashes from floating point collapse #1234

Conversation

michaelkirk commented Oct 24, 2024 • edited Loading

Performance

Alternatives

michaelkirk commented Oct 25, 2024

michaelkirk Oct 25, 2024

Choose a reason for hiding this comment

RobWalt commented Oct 25, 2024

RobWalt left a comment

Choose a reason for hiding this comment

TotalKrill commented Oct 25, 2024

dabreegster commented Oct 25, 2024

RobWalt Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

michaelkirk Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

frewsxcv Oct 25, 2024

Choose a reason for hiding this comment

michaelkirk Oct 25, 2024

Choose a reason for hiding this comment

frewsxcv Oct 25, 2024

Choose a reason for hiding this comment

frewsxcv Oct 25, 2024

Choose a reason for hiding this comment

TotalKrill Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

frewsxcv left a comment

Choose a reason for hiding this comment

frewsxcv commented Oct 28, 2024

michaelkirk commented Oct 28, 2024

michaelkirk commented Oct 24, 2024 •

edited

Loading

RobWalt Oct 25, 2024 •

edited

Loading

michaelkirk Oct 25, 2024 •

edited

Loading

TotalKrill Oct 25, 2024 •

edited

Loading