-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts. |
Link to the likely root cause #74615. |
0f74c78
to
76a8049
Compare
Does someone from |
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. |
Ralf seemed to like the added doc comment, when I split this off of some other PR. |
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 :) |
Can't reopen so another 5 digit issue number sacrificed. |
…ulacrum Document btree's unwrap_unchecked rust-lang#74693's second wind
…ulacrum Document btree's unwrap_unchecked rust-lang#74693's second wind
The seemingly optimized function
unwrap_unchecked
used in btree code appears to be slower than standardunwrap
. Redirecting tounwrap
as in this PR, we get some juicy improvements:But this is also an opener for the obvious question: why?
inline(always)
toinline
makes no difference.inline
altogether has a terribly negative impact on many tests (up to factor 5) except a 40% improvement on clone tests.unwrap
takes twice as long asunwrap_or_else(|| unreachable_unchecked()))
for small and big option contents.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.