Add an assume that the index is inbounds to slice::get_unchecked #116915
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
bend-n
force-pushed
the
unwet
branch
3 times, most recently
from
78efff4
to
a70548d
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
Use `.get().unwrap()` in `[T]::get_unchecked` Fixes rust-lang#116878
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f9b6f93): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 628.777s -> 629.685s (0.14%) |
rustbot
added
perf-regression
|
looks like the large regression is in LLVM, probably due to more optimizations triggering? |
could also just be the additional work of the extra abstractions |
|
The increased complexity of the optimized MIR backs up that conclusion: https://godbolt.org/z/Kned9hxx6 I strongly suspect an implementation with |
|
Im trying it with the |
|
There's no harm in trying all the ideas, especially when the perf queue isn't very busy :) |
|
@bors try @rust-timer queue |
bors
added
S-waiting-on-bors
Add an assume that the index is inbounds to slice::get_unchecked Fixes rust-lang#116878
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-review
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (85a4bd8): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDNext Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 672.824s -> 673.692s (0.13%) |
|
@saethlin: are the binary size regression expected? They seem surprising to me, given that this change was intended to produce more streamlined code :/ |
|
Presumably more optimized code. |
|
The fact that The top of the cachegrind diff for regex opt-full is: Which at least suggests that some of the increased icount in LLVM is processing the assumes. Whether it's helping is less clear. Just doing an To Hardly a massive win, but the change does suggest we optimized out some panics that used to be in there. But grepping for So I have two guesses:
|
|
We have a codegen test in this PR that verifies that it helps |
- Use internal to go around is_foreign_item() issue - Update tests after an issue: rust-lang/rust#116915 - Update the toolchain
Changes required due to: - rust-lang/rust@99ac405b96 Move MetadataLoader{,Dyn} to rustc_metadata. - rust-lang/rust@c997c6d822 Add more information to stable Instance - rust-lang/rust#116915 This also fixes an issue in the `simd_shuffle` implementation that was exposed by the update. Resolves #2911 --------- Co-authored-by: Celina G. Val <celinval@amazon.com> Co-authored-by: Adrian Palacios <accorell@amazon.com>
| @@ -233,7 +233,10 @@ unsafe impl<T> SliceIndex<[T]> for usize { | |||
| // cannot be longer than `isize::MAX`. They also guarantee that | |||
| // `self` is in bounds of `slice` so `self` cannot overflow an `isize`, | |||
| // so the call to `add` is safe. | |||
| unsafe { slice.as_ptr().add(self) } | |||
| unsafe { | |||
| crate::intrinsics::assume(self < slice.len()); | |||
There was a problem hiding this comment.
Something I just noticed here: why is there an assume in get_unchecked, but not in get_unchecked_mut below?
There was a problem hiding this comment.
I couldn't figure out how to justify the compile-time regression: #120762
There was a problem hiding this comment.
Actually it seems there's a pretty robust effect on the size of libstd.so that I just didn't notice before. Hunh.
Fixes #116878