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

Improve btree's unwrap_unchecked #74693

Closed
wants to merge 1 commit into from

Conversation

ssomers
Copy link
Contributor

@ssomers ssomers commented Jul 23, 2020

The seemingly optimized function unwrap_unchecked used in btree code appears to be slower than standard unwrap. Redirecting to unwrap as in this PR, we get some juicy improvements:

cargo-benchcmp.exe benchcmp old unwrap --threshold 10
 name                                           old ns/iter  unwrap ns/iter  diff ns/iter   diff %  speedup
 btree::map::iter_0                             1,509        1,762                    253   16.77%   x 0.86
 btree::map::iteration_1000                     4,079        951                   -3,128  -76.69%   x 4.29
 btree::map::iteration_100000                   497,565      275,210             -222,355  -44.69%   x 1.81
 btree::map::iteration_20                       81           20                       -61  -75.31%   x 4.05
 btree::map::iteration_mut_1000                 3,913        955                   -2,958  -75.59%   x 4.10
 btree::map::iteration_mut_100000               480,135      277,160             -202,975  -42.27%   x 1.73
 btree::map::iteration_mut_20                   77           19                       -58  -75.32%   x 4.05
 btree::set::difference_random_100_vs_100       736          647                      -89  -12.09%   x 1.14
 btree::set::difference_random_10k_vs_10k       187,103      161,630              -25,473  -13.61%   x 1.16
 btree::set::difference_staggered_100_vs_100    737          658                      -79  -10.72%   x 1.12
 btree::set::difference_staggered_10k_vs_10k    71,737       64,181                -7,556  -10.53%   x 1.12
 btree::set::intersection_random_100_vs_100     624          334                     -290  -46.47%   x 1.87
 btree::set::intersection_random_10k_vs_10k     162,862      120,976              -41,886  -25.72%   x 1.35
 btree::set::intersection_staggered_100_vs_100  636          354                     -282  -44.34%   x 1.80
 btree::set::intersection_staggered_10k_vs_10k  60,853       33,245               -27,608  -45.37%   x 1.83
 btree::set::is_subset_100_vs_100               589          301                     -288  -48.90%   x 1.96
 btree::set::is_subset_10k_vs_10k               59,345       29,873               -29,472  -49.66%   x 1.99

But this is also an opener for the obvious question: why?

  • Changing the inline(always) to inline makes no difference.
  • Removing the inline altogether has a terribly negative impact on many tests (up to factor 5) except a 40% improvement on clone tests.
  • It doesn't happen in the laboratory. A standalone microbenchmark says that unwrap takes twice as long as unwrap_or_else(|| unreachable_unchecked())) for small and big option contents.
  • There's one unwrap_unchecked call in a DropGuard that is averse to panic, but changing that alone doesn't change performance.

Putting on draft because it's weird and because it interferes with #73971.

@rust-highfive
Copy link
Collaborator

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 23, 2020
@bors
Copy link
Contributor

bors commented Jul 28, 2020

☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts.

@nbdd0121
Copy link
Contributor

nbdd0121 commented Aug 7, 2020

Link to the likely root cause #74615.

@ssomers ssomers changed the title Optimize btree's unwrap_unchecked Improve btree's unwrap_unchecked Aug 7, 2020
@LeSeulArtichaut
Copy link
Contributor

Does someone from T-compiler want to review or help with this? Maybe r? @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

I am inclined to not try and change unwrap_unchecked since it seems likely that the improvements are optimizer noise (e.g., will be regressed/improved for various changes just because of slightly different inlining decisions and such), so I am going to close this, but if people feel differently feel free to reopen/let me know.

@ssomers
Copy link
Contributor Author

ssomers commented Sep 9, 2020

Ralf seemed to like the added doc comment, when I split this off of some other PR.

@Mark-Simulacrum
Copy link
Member

I am happy to accept the doc comment (either in this PR, reopened and adjusted, or a new one). That would be a quick review too :)

@ssomers
Copy link
Contributor Author

ssomers commented Sep 9, 2020

Can't reopen so another 5 digit issue number sacrificed.

tmandry added a commit to tmandry/rust that referenced this pull request Sep 10, 2020
…ulacrum

Document btree's unwrap_unchecked

rust-lang#74693's second wind
tmandry added a commit to tmandry/rust that referenced this pull request Sep 10, 2020
…ulacrum

Document btree's unwrap_unchecked

rust-lang#74693's second wind
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants