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.

Sign up for GitHub

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

Jump to bottom

small perf improvements to TriangulateSpade #1122

Merged
merged 2 commits into from
Dec 7, 2023
Merged

small perf improvements to TriangulateSpade #1122

merged 2 commits into from
Dec 7, 2023

Conversation

michaelkirk
Copy link
Member

@michaelkirk michaelkirk commented Dec 5, 2023 •
edited
Loading

  • 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.

Bench output:

TriangulateSpade (unconstrained)
                        time:   [8.8442 ms 8.8522 ms 8.8619 ms]
                        change: [-2.9583% -2.7775% -2.6122%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
  1 (1.00%) high mild
  3 (3.00%) high severe

TriangulateSpade (constrained)
                        time:   [8.9126 ms 8.9274 ms 8.9444 ms]
                        change: [-2.4017% -2.1584% -1.9066%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  3 (3.00%) high severe

TriangulateEarcut       time:   [6.9632 ms 6.9750 ms 6.9883 ms]
                        change: [-1.0174% -0.7692% -0.5114%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)

Note this is an internal API, so we can safely break the API.

@michaelkirk michaelkirk marked this pull request as draft December 5, 2023 17:02
@michaelkirk michaelkirk force-pushed the mkirk/integrate-spade-coords-iter branch from 3fd831d to f55e977 Compare December 5, 2023 19:57
@michaelkirk michaelkirk marked this pull request as ready for review December 5, 2023 19:57
urschrei
urschrei approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@urschrei urschrei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interested to hear how you came up with it, but lgtm.

@michaelkirk
Copy link
Member Author

Interested to hear how you came up with it, but lgtm.

When the TriangulateSpade feature was merged, I made a non-blocking suggestion to try something like this. It didn't make it into the original PR, but I decided to give it a shot myself before I forgot.

@michaelkirk michaelkirk requested a review from RobWalt December 5, 2023 22:16
RobWalt
RobWalt approved these changes Dec 6, 2023
View reviewed changes
Copy link
Contributor

@RobWalt RobWalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice! Thanks for taking a shot at it, you nailed it 🎯

RobWalt
RobWalt requested changes Dec 6, 2023
View reviewed changes
geo/benches/triangulate.rs Outdated Show resolved Hide resolved
@michaelkirk michaelkirk force-pushed the mkirk/integrate-spade-coords-iter branch 2 times, most recently from e2bd59b to c611d0c Compare December 6, 2023 21:11
@michaelkirk
benchmarks for triangulate
58d6bac
@michaelkirk
PERF: TriangulateSpade uses Iter, not Vec
aa71ffd 
This is an internal API, so we can safely break the API.

Bench output:
```
TriangulateSpade (unconstrained) - small polys
                        time:   [8.7534 ms 8.7641 ms 8.7777 ms]
                        change: [-2.6587% -2.1965% -1.7360%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe

Benchmarking TriangulateSpade (constrained) - small polys: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 98.7s, or reduce sample count to 10.
TriangulateSpade (constrained) - small polys
                        time:   [986.61 ms 987.03 ms 987.51 ms]
                        change: [-5.2413% -5.0808% -4.9190%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 17 outliers among 100 measurements (17.00%)
  7 (7.00%) high mild
  10 (10.00%) high severe

TriangulateEarcut - small polys
                        time:   [6.9938 ms 6.9993 ms 7.0041 ms]
                        change: [-1.3566% -0.9176% -0.5312%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 12 outliers among 100 measurements (12.00%)
  5 (5.00%) low severe
  5 (5.00%) low mild
  2 (2.00%) high mild

TriangulateSpade (unconstrained) - large_poly
                        time:   [3.4280 ms 3.4390 ms 3.4521 ms]
                        change: [+0.2922% +0.7947% +1.3032%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 13 outliers among 100 measurements (13.00%)
  7 (7.00%) high mild
  6 (6.00%) high severe

Benchmarking TriangulateSpade (constrained) - large_poly: Warming up for 3.0000 s
Warning: Unable to complete 100 samples in 5.0s. You may wish to increase target time to 178.3s, or reduce sample count to 10.
TriangulateSpade (constrained) - large_poly
                        time:   [1.7869 s 1.8005 s 1.8178 s]
                        change: [-7.9992% -5.8498% -4.2765%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 14 outliers among 100 measurements (14.00%)
  5 (5.00%) high mild
  9 (9.00%) high severe

TriangulateEarcut - large_poly
                        time:   [2.2287 ms 2.2346 ms 2.2413 ms]
                        change: [+1.0859% +1.4903% +1.9614%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 14 outliers among 100 measurements (14.00%)
  10 (10.00%) high mild
  4 (4.00%) high severe
```
@michaelkirk michaelkirk force-pushed the mkirk/integrate-spade-coords-iter branch from c611d0c to aa71ffd Compare December 7, 2023 17:09
@michaelkirk michaelkirk enabled auto-merge December 7, 2023 17:09
@michaelkirk michaelkirk added this pull request to the merge queue Dec 7, 2023
Merged via the queue into main with commit 47fadf1 Dec 7, 2023
15 checks passed
@urschrei urschrei deleted the mkirk/integrate-spade-coords-iter branch February 5, 2025 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@urschrei urschrei urschrei approved these changes

@frewsxcv frewsxcv frewsxcv approved these changes

@RobWalt RobWalt RobWalt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelkirk @urschrei @frewsxcv @RobWalt