Conversation
|
r? @Nilstrieb rustbot has assigned @Nilstrieb. Use |
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
Optimize `EscapeIterInner` This optimizes `EscapeIterInner` by using `MaybeUninit` for unused array elements instead of initializing them with `ascii::Char::Null`. Follow up to rust-lang#124307, CC `@reitermarkus`
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (a2f180d): comparison URL. Overall result: ❌ regressions - no 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. @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)Results (primary 3.0%)This 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.
CyclesResults (secondary -2.2%)This 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 sizeResults (primary -0.1%, secondary -0.0%)This 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: 670.321s -> 670.058s (-0.04%) |
rustbot
removed
the
S-waiting-on-perf
| self.data.get_unchecked(usize::from(self.alive.start)..usize::from(self.alive.end)) | ||
| let data = self.data.get_unchecked(self.alive.start as usize..self.alive.end as usize); |
There was a problem hiding this comment.
Why did you change usize::fromto as here? I prefer the from conversions
|
Do you have any benchmarks or at least assembly examples to show that this makes the code faster? If you do, r=me after addressing my other question (and either changing it back or not). The unsafe here is not very complex but I don't want to merge it without any proof of at least some improvements. |
| // The element type ensures this is always ASCII, and thus also valid UTF-8. | ||
| data: [ascii::Char; N], | ||
| // Invariant: all elements inside the range indexed by `alive` are initialized | ||
| data: [MaybeUninit<ascii::Char>; N], |
There was a problem hiding this comment.
Note that, for small N, it's not obvious at all to me that this is necessarily a win. Zero-initializing a [ascii::Char; 8] for example is just a single 0_u64, and having it be MaybeUninit instead comes with the cost of it not getting noundef when it's passed around, reducing optimization possibilities for what LLVM can do with it.
For large, especially heap-allocated buffers it can help to avoid initializing them, but making all this code safety-critical (EscapeIterInner::ascii now has to be considered for understanding if it's UB, for example) is not at all obvious that it's a good tradeoff.
(We really need unsafe structs so that potential UB stops hiding in struct literals.)
|
@rustbot author |
rustbot
added
S-waiting-on-author
|
What exactly is the status of this? Is it actually worth it to make these changes? |
|
@joboet any updates on this? thanks |
|
On second thought I agree with Scott's assessment and am closing this. |
10 participants
This optimizes
EscapeIterInnerby usingMaybeUninitfor unused array elements instead of initializing them withascii::Char::Null.Follow up to #124307, CC @reitermarkus