Upgrade to i_overlay ~~1.8~~ 1.9 #1275
Conversation
i_overlay 1.8 introduced a new trait based approach which allows us to remove some of our own trait juggling. However, our old friend, the orphan trait rule, prevents this from happening easily. For now, I'm creating wrapper structs in geo for this algo. i_overlay 1.8 also introduced a new method signature for doing bool_ops, removing the old ones. This caused our trait juggling to infinitely recurse: See #1270 Now that we're using the new methods, the problem is avoided.
|
As noted in #1273, I'm seeing a perf improvement for a large workload. |
Note there is a serious performance regression for larger geometries in our benchmarks vs. 1.8.
cargo bench --bench=boolean_ops -- --baseline=main
... small ones are fine, but big ones have huge regression
Circular polygon boolean-ops/bops::union/1024
time: [4.3625 ms 4.3922 ms 4.4093 ms]
change: [+1.7493% +2.4971% +3.2026%] (p = 0.00 < 0.05)
Performance has regressed.
Circular polygon boolean-ops/bops::intersection/2048
time: [14.057 ms 14.115 ms 14.157 ms]
change: [+9.0224% +10.438% +11.658%] (p = 0.00 < 0.05)
Performance has regressed.
Circular polygon boolean-ops/bops::union/2048
time: [14.012 ms 14.071 ms 14.134 ms]
change: [+11.212% +12.141% +13.317%] (p = 0.00 < 0.05)
Performance has regressed.
Found 1 outliers among 10 measurements (10.00%)
1 (10.00%) high severe
Circular polygon boolean-ops/bops::intersection/4096
time: [360.66 ms 361.18 ms 361.83 ms]
change: [+709.38% +713.12% +716.34%] (p = 0.00 < 0.05)
Performance has regressed.
Found 2 outliers among 10 measurements (20.00%)
2 (20.00%) high severe
Circular polygon boolean-ops/bops::union/4096
time: [361.35 ms 361.71 ms 362.13 ms]
change: [+710.58% +713.76% +716.59%] (p = 0.00 < 0.05)
Performance has regressed.
Likely this is because there is a new heuristic for "solver" strategies
based on how big your input is - the cliff likely indicates the switch
to one of the solvers used for larger inputs.
See iShape-Rust/iOverlay#13
There are significant gains with some "real world" data, e.g. urshrei's cascaded
union tests:
#1273 (comment)
I'm more inclined to trust that more realistic data over the synthetic
geometries we use in our benchmarks.
Plus, I'd prefer to leave in performance twiddling to upstream and
contribute changes there rather than fine-tuning things at our own
call sites.
michaelkirk
changed the title
|
update: I now think we should update all the way to 1.9. @urschrei has verified that both 1.8 and 1.9 solve his crash and I have at least a probable explanation for the performance regression seen in 1.9 - see iShape-Rust/iOverlay#13 (I've also verified the crash is fixed with my branch mkirk/cascaded_union) Further, the perf regression is with our benchmarks which use synthetic geometries. I think it's more interesting that @urschrei is seeing moderate performance improvements with his real world data. Plus, I'd rather leave any performance twiddling as a responsibility of upstream rather than having our own customizations optimized for our benchmarks. |
|
I agree that 1.9 is the way to go. iShape-Rust/iOverlay#13 (comment) is interesting – it'll become clearer in time whether there's a need for an additional API with a selectable solver in |
CHANGES.mdif knowledge of this change could be valuable to users.This
iswas a draft because it's a little bit slower (see "Performance"). It might solve a problem #1273 (?), in which case, it's probably worth merging this and taking the modest perf hit.There's actually an even newer release (1.9) (see my branch: mkirk/i_overlay_1.9) but it has some more serious performance regressions.
I'll open an issue upstream about that.updated: here it is: iShape-Rust/iOverlay#13update:
I now think we should update all the way to 1.9.
@urschrei has verified that both 1.8 and 1.9 solve his crash and I have at least a probable explanation for the performance regression seen in 1.9 - see iShape-Rust/iOverlay#13
(I've also verified the crash is fixed with my branch mkirk/cascaded_union)
Further, the perf regression is with our benchmarks which use synthetic geometries. I think it's more interesting that @urschrei is seeing moderate performance improvements with his real world data. Plus, I'd rather leave any performance twiddling as a responsibility of upstream rather than having our own customizations optimized for our benchmarks.
Description of Changes
i_overlay 1.8 introduced a new trait based approach which allows us to
remove some of our own trait juggling.
However, our old friend, the orphan trait rule, prevents this from
happening easily. For now, I'm creating wrapper structs in geo for this
algo.
i_overlay 1.8 also introduced a new method signature for doing bool_ops,
removing the old ones. This caused our trait juggling to infinitely
recurse: See #1270
Now that we're using the new methods, the problem is avoided.
Note that I've also opened iShape-Rust/iOverlay#12 to discuss using semver with i_overlay for future releases. For now though, until we hear otherwise, I think we should assume that breaking changes will occur in minor release - the
YinX.Y.Z.FIXES #1270
Performance
Bench output is a little mixed, but mostly a bit slower:
bench output with i_overlay 1.8
bench output with i_overlay 1.9