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

fix: use a stable sort in sortf function #39

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kurtlawrence
Copy link

I ran into an issue upgrading Rust 1.80 to 1.82 which broke some of our testing.

I tracked the issue down to a change in the output of triangualate, driven by not using a stable sort.

By using a stable sorting algorithm, the output of the triangulate function will be stable across Rust versions.

@mourner
Copy link
Owner

mourner commented Nov 6, 2024

@kurtlawrence as far as I remember, the unstable sort was used intentionally because it should be faster (and sort order doesn't impact triangulation correctness). Can you see how much the change affects benchmark results?

@kurtlawrence
Copy link
Author

(and sort order doesn't impact triangulation correctness).

The issue we found is that we triangulate a grid of points. Thus, there are point that are equidistant from the seed triangles circumcenter.
One can imagine that given a set of points ABCD, there are 2 ways to construct triangles: ABC|BCD or ABD|ADC
It is only a minor change, but it does affect normal calculations.
By using a stable sort, the output of the triangles is stable as well, given the same sequence of input points.

Can you see how much the change affects benchmark results?

I'll look into doing a benchmark

@kurtlawrence
Copy link
Author

@mourner This is the result of the benchmark on my machine.

…/delaunator-rs => cargo bench                                                                                                                                                                                                             master ⇕  v1.82.0
   Compiling delaunator v1.0.2 (/tmp/delaunator-rs)
    Finished `bench` profile [optimized] target(s) in 1.25s
     Running benches/bench.rs (target/release/deps/bench-de372526918d0b54)
Gnuplot not found, using plotters backend
triangulate/100         time:   [7.9376 µs 7.9704 µs 8.0049 µs]
                        change: [+1.9580% +3.7152% +5.5551%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
  1 (1.00%) low mild
  7 (7.00%) high mild
  4 (4.00%) high severe
triangulate/1000        time:   [99.965 µs 100.33 µs 100.72 µs]
                        change: [+0.9385% +2.7014% +4.4689%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 14 outliers among 100 measurements (14.00%)
  8 (8.00%) high mild
  6 (6.00%) high severe
triangulate/10000       time:   [1.9243 ms 1.9310 ms 1.9387 ms]
                        change: [+4.1406% +5.1413% +5.9536%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 15 outliers among 100 measurements (15.00%)
  3 (3.00%) high mild
  12 (12.00%) high severe
triangulate/100000      time:   [27.885 ms 28.057 ms 28.246 ms]
                        change: [-0.3175% +0.5530% +1.3821%] (p = 0.20 > 0.05)
                        No change in performance detected.
Found 16 outliers among 100 measurements (16.00%)
  5 (5.00%) high mild
  11 (11.00%) high severe

@mourner
Copy link
Owner

mourner commented Nov 7, 2024

@kurtlawrence so, the change looks like a 3–5% hit to performance. Do you think this would be worth the hit, or perhaps there is a workaround you could go for on your project side to keep performance intact?

@kurtlawrence
Copy link
Author

@kurtlawrence so, the change looks like a 3–5% hit to performance. Do you think this would be worth the hit, or perhaps there is a workaround you could go for on your project side to keep performance intact?

For now, we have forked the repository and made the alteration. There isn't anything we can do code wise unfortunately.

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

Successfully merging this pull request may close these issues.

2 participants