-
Notifications
You must be signed in to change notification settings - Fork 5
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
performance regression with 1.9.0 #13
Comments
In 1.9.0, the logic for splitting data was changed to allow for some parallel processing(Fragment Solver). Based on my benchmarks, single-thread performance has stayed mostly the same, but for multi-threaded solvers, it sometimes gives up to a 2x boost. The library currently supports three solvers: List, Tree, and Fragment, each suited for a specific range:
These ranges are approximate, and in different tests, the performance picture can vary significantly. Right now, the ranges aren't properly adjusted, and I’m working on a more robust analyzer for selecting the best solver automatically. Writing better performance tests is also a priority. For now, I suggest experimenting with different solvers. If your data isn’t very dense, the List solver might work better for your use case. |
Ah, that likely explains it! Thanks for the information. Those sound like exciting changes. Our own benchmarks in georust/geo are using some synthetic data, so I'm not super confident in the particular numbers they produce. Do you have benchmarks that you can share? I tried running |
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.
I noticed a severe regression in our benchmarks from 1.8.2 to 1.9.0 (and also present in 1.9.1) for larger inputs.
e.g. The union of polygons with 256-2048 coordinates were mostly unaffected (though a bit slower, but not really worried about that) while those with 4098 or 8192 were much slower, which I'm concerned about.
At first I suspected the switchover to using the generic templated FloatOverlay (this refactor really cleaned up our integration, thank you!), but there was no significant regression in this adoption of 1.8.2. It was only the upgrade from 1.8.2 to 1.9.0/1.9.1 which introduces the slowdown - no other changes.
see the diff https://github.com/georust/geo/compare/mkirk/i_overlay_1.8...mkirk/i_overlay_1.9?expand=1
bench output with i_overlay 1.9
See the bottom of the output for the severe cases
Any idea what's causing it or how we might avoid it?
The text was updated successfully, but these errors were encountered: