slice_split_once: bounds check optimization note#158282
Conversation
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @jhpratt (or someone else) some time within the next two weeks. 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 (
Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
| let index = self.iter().position(pred)?; | ||
| Some((&self[..index], &self[index + 1..])) | ||
| // SAFETY: `index` is inside `self` | ||
| let left = unsafe { self.get_unchecked(0..index) }; | ||
| // SAFETY: `index` is inside `self` | ||
| let right = unsafe { self.get_unchecked(index + 1..self.len()) }; | ||
| Some((left, right)) |
There was a problem hiding this comment.
I just did a quick check: both versions actually produce identical code at -Copt-level=1 https://rust.godbolt.org/z/n8cTdY4rT. So I think you can actually just leave the code as-is to save the unsafe, but instead add a comment saying the bounds check is trivially optimized out. Unless @vmmon happens to remember more about the circumstances of #112811 (comment).
Sorry about the back and forth here!
There was a problem hiding this comment.
(PR also needs a squash and commit message update before merge)
There was a problem hiding this comment.
squashing can be done with bors now :)
The comment you linked makes it sound like the compiler requires providing panic machinery because it's (theoretically) being called, which an embedded user might not want to do because of the possibility of it accidentally being used elsewhere. I could be wildly off on that; embedded is not my forte.
There was a problem hiding this comment.
squashing can be done with bors now :)
🎉 I keep forgetting we have a new bors in town
The comment you linked makes it sound like the compiler requires providing panic machinery because it's (theoretically) being called, which an embedded user might not want to do because of the possibility of it accidentally being used elsewhere. I could be wildly off on that; embedded is not my forte.
I dug a bit more and looks like that check wasn't optimized out at -O1 around the time that compiler was written, https://rust.godbolt.org/z/n8fqr61bb has a call to slice_start_index_len_fail. It was at -O2 so maybe some optimizations moved around since then?
There was a problem hiding this comment.
Up to you on which you prefer. unsafe is obviously future-proof regarding optimizations, but I also wouldn't expect regressions here.
There was a problem hiding this comment.
r=me with the current version unless you have a strong preference, I wouldn't expect anyone concerned about binary size to be running with no optimizations
There was a problem hiding this comment.
I think it would be preferable not to rely on the optimizer to do what we want it to. I would expect that someone would have to regularly go in and fact check that comment, while having unsafe guarantees the behavior we want regardless of how things change.
|
@bors squash msg="slice_split_once: bounds check optimization note" |
This comment has been minimized.
This comment has been minimized.
|
🔨 5 commits were squashed into 5d07ed5. |
879e4c6 to
5d07ed5
Compare
… r=tgross35,jhpratt slice_split_once: bounds check optimization note Tracking issue: rust-lang#112811 Use unchecked sub-slicing operations for `<T>::split_once` and `<T>::rsplit_once`.
… r=tgross35,jhpratt slice_split_once: bounds check optimization note Tracking issue: rust-lang#112811 ~~Use unchecked sub-slicing operations for `<T>::split_once` and `<T>::rsplit_once`.~~ Note that unchecked sub-slicing operations are equivalent to the current checked sub-slicing operations.
Rollup of 8 pull requests Successful merges: - #157960 (delegation: add support for infers in generics) - #158105 (Extract all instance shim variants into new `ShimKind` enum) - #158207 (Resolver: local/external split of `resolve_ident_in_module_non_globs_unadjusted` ) - #158222 (format: ignore println newline in foreign format hints) - #158252 (Use `cfg_select` in `std::os`) - #158257 ( fix escaping placeholder check in next solver normalization folder) - #158274 (triagebot: Stop pinging myself) - #158282 (slice_split_once: bounds check optimization note)
Rollup of 8 pull requests Successful merges: - #157960 (delegation: add support for infers in generics) - #158105 (Extract all instance shim variants into new `ShimKind` enum) - #158207 (Resolver: local/external split of `resolve_ident_in_module_non_globs_unadjusted` ) - #158222 (format: ignore println newline in foreign format hints) - #158252 (Use `cfg_select` in `std::os`) - #158257 ( fix escaping placeholder check in next solver normalization folder) - #158274 (triagebot: Stop pinging myself) - #158282 (slice_split_once: bounds check optimization note)
… r=tgross35,jhpratt slice_split_once: bounds check optimization note Tracking issue: rust-lang#112811 ~~Use unchecked sub-slicing operations for `<T>::split_once` and `<T>::rsplit_once`.~~ Note that unchecked sub-slicing operations are equivalent to the current checked sub-slicing operations.
…uwer Rollup of 16 pull requests Successful merges: - #156885 (Fix misattributed type inference error span for index expressions) - #157271 (simplify some `proc_macro` things) - #157883 (Remove strict invariant node_type on hir_type during ty privacy visit) - #157921 (trait solver: Resolve region vars in max universe) - #157960 (delegation: add support for infers in generics) - #158105 (Extract all instance shim variants into new `ShimKind` enum) - #158207 (Resolver: local/external split of `resolve_ident_in_module_non_globs_unadjusted` ) - #158279 (Follow goto and drop when linting unreachable code) - #157807 (don't ice on non-lifetime binders under `-Zassumptions-on-binders`) - #158020 (Update mingw-w64 C toolchain) - #158222 (format: ignore println newline in foreign format hints) - #158223 (Move target checking for #[lang] to the attribute parser) - #158252 (Use `cfg_select` in `std::os`) - #158257 ( fix escaping placeholder check in next solver normalization folder) - #158274 (triagebot: Stop pinging myself) - #158282 (slice_split_once: bounds check optimization note) Failed merges: - #158256 (Avoid parser panics bubbling out to proc macros)
… r=tgross35,jhpratt slice_split_once: bounds check optimization note Tracking issue: rust-lang#112811 ~~Use unchecked sub-slicing operations for `<T>::split_once` and `<T>::rsplit_once`.~~ Note that unchecked sub-slicing operations are equivalent to the current checked sub-slicing operations.
… r=tgross35,jhpratt slice_split_once: bounds check optimization note Tracking issue: rust-lang#112811 ~~Use unchecked sub-slicing operations for `<T>::split_once` and `<T>::rsplit_once`.~~ Note that unchecked sub-slicing operations are equivalent to the current checked sub-slicing operations.
…uwer Rollup of 23 pull requests Successful merges: - #158315 (`rust-analyzer` subtree update) - #155739 (Add temporary scope to assert_eq and assert_ne) - #156885 (Fix misattributed type inference error span for index expressions) - #157271 (simplify some `proc_macro` things) - #157883 (Remove strict invariant node_type on hir_type during ty privacy visit) - #157921 (trait solver: Resolve region vars in max universe) - #157960 (delegation: add support for infers in generics) - #157983 (Lift the same-signature restriction for `extern "tail"`) - #158105 (Extract all instance shim variants into new `ShimKind` enum) - #158207 (Resolver: local/external split of `resolve_ident_in_module_non_globs_unadjusted` ) - #158279 (Follow goto and drop when linting unreachable code) - #157527 (Move derive tests into their dedicated folder) - #157807 (don't ice on non-lifetime binders under `-Zassumptions-on-binders`) - #158020 (Update mingw-w64 C toolchain) - #158222 (format: ignore println newline in foreign format hints) - #158223 (Move target checking for #[lang] to the attribute parser) - #158252 (Use `cfg_select` in `std::os`) - #158257 ( fix escaping placeholder check in next solver normalization folder) - #158263 (Only load the feature list once in the entire resolver) - #158274 (triagebot: Stop pinging myself) - #158282 (slice_split_once: bounds check optimization note) - #158300 (Improve unknown crate_type diagnostic suggestions) - #158304 (mailmap: update mu001999) Failed merges: - #158256 (Avoid parser panics bubbling out to proc macros)
… r=tgross35,jhpratt slice_split_once: bounds check optimization note Tracking issue: rust-lang#112811 ~~Use unchecked sub-slicing operations for `<T>::split_once` and `<T>::rsplit_once`.~~ Note that unchecked sub-slicing operations are equivalent to the current checked sub-slicing operations.
Rollup of 27 pull requests Successful merges: - #158315 (`rust-analyzer` subtree update) - #155739 (Add temporary scope to assert_eq and assert_ne) - #156885 (Fix misattributed type inference error span for index expressions) - #157271 (simplify some `proc_macro` things) - #157883 (Remove strict invariant node_type on hir_type during ty privacy visit) - #157921 (trait solver: Resolve region vars in max universe) - #157960 (delegation: add support for infers in generics) - #157983 (Lift the same-signature restriction for `extern "tail"`) - #158053 (Optimize network address parser) - #158105 (Extract all instance shim variants into new `ShimKind` enum) - #158207 (Resolver: local/external split of `resolve_ident_in_module_non_globs_unadjusted` ) - #158279 (Follow goto and drop when linting unreachable code) - #157527 (Move derive tests into their dedicated folder) - #157807 (don't ice on non-lifetime binders under `-Zassumptions-on-binders`) - #158020 (Update mingw-w64 C toolchain) - #158039 (c-variadic: test that we use equality up to free lifetimes) - #158222 (format: ignore println newline in foreign format hints) - #158223 (Move target checking for #[lang] to the attribute parser) - #158252 (Use `cfg_select` in `std::os`) - #158257 ( fix escaping placeholder check in next solver normalization folder) - #158263 (Only load the feature list once in the entire resolver) - #158267 (FromUtf8Error::into_utf8_lossy better example and suggest use) - #158274 (triagebot: Stop pinging myself) - #158282 (slice_split_once: bounds check optimization note) - #158300 (Improve unknown crate_type diagnostic suggestions) - #158304 (mailmap: update mu001999) - #158309 (Update `rustc-literal-escaper` version to `0.0.8`) Failed merges: - #158256 (Avoid parser panics bubbling out to proc macros)
… r=tgross35,jhpratt slice_split_once: bounds check optimization note Tracking issue: rust-lang#112811 ~~Use unchecked sub-slicing operations for `<T>::split_once` and `<T>::rsplit_once`.~~ Note that unchecked sub-slicing operations are equivalent to the current checked sub-slicing operations.
… r=tgross35,jhpratt slice_split_once: bounds check optimization note Tracking issue: rust-lang#112811 ~~Use unchecked sub-slicing operations for `<T>::split_once` and `<T>::rsplit_once`.~~ Note that unchecked sub-slicing operations are equivalent to the current checked sub-slicing operations.
Tracking issue: #112811
Use unchecked sub-slicing operations for<T>::split_onceand<T>::rsplit_once.Note that unchecked sub-slicing operations are equivalent to the current checked sub-slicing operations.