-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Improve btree's unwrap_unchecked #74693
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
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_uncheckedused in btree code appears to be slower than standardunwrap. Redirecting tounwrapas in this PR, we get some juicy improvements:But this is also an opener for the obvious question: why?
inline(always)toinlinemakes no difference.inlinealtogether has a terribly negative impact on many tests (up to factor 5) except a 40% improvement on clone tests.unwraptakes twice as long asunwrap_or_else(|| unreachable_unchecked()))for small and big option contents.unwrap_uncheckedcall 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.