Skip to content

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
Contributor

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
Collaborator

bors commented Jul 28, 2020

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

@nbdd0121
Copy link
Member

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