-
Notifications
You must be signed in to change notification settings - Fork 13.9k
Optimize indexing slices and strs with inclusive ranges #145024
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
base: master
Are you sure you want to change the base?
Conversation
|
r? @ibraheemdev rustbot has assigned @ibraheemdev. Use |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you saw from the PR build failure, if you're touching things like slice_index_order_fail you'll need to look through every mention of those in the repo -- there are a bunch of codegen tests that look for their absence, and you need to make sure that you don't make those tests useless by renaming what they're looking for.
(You probably also need to re-bless some MIR tests that include indexing.)
eab010d to
61bcd82
Compare
This comment has been minimized.
This comment has been minimized.
61bcd82 to
2546221
Compare
This comment has been minimized.
This comment has been minimized.
2546221 to
a61fde0
Compare
|
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
48b325f to
87a3889
Compare
This comment has been minimized.
This comment has been minimized.
87a3889 to
bd63d02
Compare
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Optimize indexing slices and strs with inclusive ranges
bd63d02 to
194cfc3
Compare
This comment has been minimized.
This comment has been minimized.
|
@rustbot ready |
03fdbbd to
7bc62e2
Compare
This comment has been minimized.
This comment has been minimized.
…nicking, r=jhpratt Consolidate panicking functions in `slice/index.rs` Consolidate all the panicking functions in `slice/index.rs` to use a single `slice_index_fail` function, similar to how it is done in `str/traits.rs`. Split off from rust-lang#145024
7bc62e2 to
6f5164d
Compare
This comment has been minimized.
This comment has been minimized.
|
@ibraheemdev ping? |
|
This doesn't seem to be an improvement, but I'll pass it along for a second look. r? libs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like the plan of not handling MAX separately, if we can naturally have it handled by the normal check against len(). This definitely isn't the kind of place where we're trying to use overflow flags from the addition to do a check.
That said, I'd like to see some kind of codegen-related testing that demonstrates this coalescing the length checks together. The check for overflow is at least incredibly predictable, so if this change doesn't remove a check then it's potentially slightly worse.
Maybe a codegen-llvm test showing slice[0..=i] or slice[j..=k]? (I thought we had a mir-opt test that this would have changed, but I guess not 😕)
6f5164d to
7e8cf44
Compare
This comment has been minimized.
This comment has been minimized.
Add a `codegen-llvm` test to check the number of `icmp` instrucitons generated for each `SliceIndex` method on the various range types. This will be updated in the next commit when `SliceIndex::get` is optimized for `RangeInclusive`.
The check for `self.end() == usize::MAX` can be combined with the `self.end() + 1 > slice.len()` check into `self.end() >= slice.len()`, since `self.end() < slice.len()` implies both `self.end() <= slice.len()` and `self.end() < usize::MAX`.
Same reasoning as previous commit.
7e8cf44 to
466066b
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
Same reasoning as previous two commits.
466066b to
62172c4
Compare
This comment has been minimized.
This comment has been minimized.
62172c4 to
d2b6a1d
Compare
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Instead of separately checking for
end == usize::MAXandend + 1 > slice.len(), we can check forend >= slice.len(). Also consolidate all the slice indexing related panic functions into a single function which reports the correct error depending on the arguments, as the str indexing code already does.The downside of all this is that the panic message is slightly less specific when trying to index with
[..=usize::MAX]: instead of saying "attempted to index slice up to maximum usize" it just says "range end index {end} out of range for slice of length {len}". But this is a rare enough case that I think it is acceptable