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.

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

Open
michaelkirk opened this issue Nov 26, 2024 · 2 comments
Open

performance regression with 1.9.0 #13

michaelkirk opened this issue Nov 26, 2024 · 2 comments

Comments

@michaelkirk
Copy link

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

georust/geo$ cargo bench --bench=boolean_ops -- --baseline=main

Circular polygon boolean-ops/bops::intersection/256                                                                                                   
                        time:   [438.45 µs 440.68 µs 445.14 µs]                                                                                       
                        change: [-7.0558% -6.1120% -4.8985%] (p = 0.00 < 0.05)                                                                        
                        Performance has improved.                                                                                                     
Circular polygon boolean-ops/bops::union/256                                                                                                          
                        time:   [448.53 µs 450.78 µs 454.71 µs]                                                                                       
                        change: [-6.2725% -5.7614% -5.0363%] (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/512                                                                                                   
                        time:   [1.4381 ms 1.4409 ms 1.4442 ms]                                                                                       
                        change: [+4.0769% +4.3027% +4.4898%] (p = 0.00 < 0.05)                                                                        
                        Performance has regressed.                                                                                                    
Found 1 outliers among 10 measurements (10.00%)                                                                                                       
  1 (10.00%) low mild                                                                                                                                 
Circular polygon boolean-ops/bops::union/512                                                                                                          
                        time:   [1.4642 ms 1.4880 ms 1.5130 ms]                                                                                       
                        change: [+1.8288% +3.2331% +4.8314%] (p = 0.00 < 0.05)                                                                        
                        Performance has regressed.                                                                                                    
Found 1 outliers among 10 measurements (10.00%)                                                                                                       
  1 (10.00%) high mild                                                                                                                                
Circular polygon boolean-ops/bops::intersection/1024                                                                                                  
                        time:   [4.3295 ms 4.3493 ms 4.3684 ms]                                                                                       
                        change: [+1.1227% +2.1430% +3.0016%] (p = 0.00 < 0.05)                                                                        
                        Performance has regressed.                                                                                                    
Found 1 outliers among 10 measurements (10.00%)                                                                                                       
  1 (10.00%) high mild                                                                                                                                
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.
Found 1 outliers among 10 measurements (10.00%)
  1 (10.00%) high mild
Benchmarking Circular polygon boolean-ops/bops::intersection/8192: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 15.4s.
Circular polygon boolean-ops/bops::intersection/8192
                        time:   [1.5360 s 1.5404 s 1.5452 s]
                        change: [+816.16% +837.03% +850.36%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking Circular polygon boolean-ops/bops::union/8192: Warming up for 3.0000 s
Warning: Unable to complete 10 samples in 5.0s. You may wish to increase target time to 15.4s.
Circular polygon boolean-ops/bops::union/8192
                        time:   [1.5359 s 1.5375 s 1.5393 s]
                        change: [+820.30% +832.33% +843.12%] (p = 0.00 < 0.05)
                        Performance has regressed.

Any idea what's causing it or how we might avoid it?

@NailxSharipov
Copy link
Member

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:

  • List: Small ranges (0 to ~32k edges).
  • Tree: Medium ranges (~4k to ~128k edges).
  • Fragment: Large ranges (~32k to 100M+ edges).

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.

@michaelkirk
Copy link
Author

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 cargo bench in this repository, but it doesn't seem like any are integrated here.

michaelkirk added a commit to georust/geo that referenced this issue Nov 26, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants