-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Simplify the implementation of iterators over slices of ZSTs #111395
Conversation
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
⌛ Trying commit b3389f2eb53d001e94ff67b177cb3bc41f7bbc71 with merge 84b202a37f44bc97a1f2bec83a95928739984869... |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (84b202a37f44bc97a1f2bec83a95928739984869): comparison URL. Overall result: ❌✅ regressions and improvements - 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)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: 658.646s -> 657.694s (-0.14%) |
b3389f2
to
8c82c45
Compare
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
r? @the8472 |
Currently, slice iterators over ZSTs store `end = start.wrapping_byte_add(len)`. That's slightly convenient for `is_empty`, but kinda annoying for pretty much everything else -- see bugs like 42789, for example. This PR instead changes it to just `end = ptr::invalid(len)` instead. That's easier to think about (IMHO, at least) as well as easier to represent.
8c82c45
to
15aa7fa
Compare
Interesting, some debug binary size wins. I guess debug doesn't even eliminate the IS_ZST branches? @bors r+ |
🌲 The tree is currently closed for pull requests below priority 50. This pull request will be tested once the tree is reopened. |
Looks like it doesn't, yeah: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=5c4cd54a7b07a07ccecea6a7a092c57e br i1 true, label %bb1, label %bb2, !dbg !149 And %_12.i = alloca %"core::ptr::metadata::PtrComponents<u32>", align 8
%_11.i = alloca %"core::ptr::metadata::PtrRepr<u32>", align 8
%_10.i = alloca %"core::ptr::metadata::PtrRepr<u32>", align 8 that isn't there in just the |
⌛ Testing commit 15aa7fa with merge 41c9748da663a483561273aa0ae71d47dba10c61... |
💔 Test failed - checks-actions |
@bors retry |
Well, https://www.githubstatus.com/incidents/nf7s6933tnn8 says things are supposed to be working, and #111475 hit a legit problem, not an infrastructure problem, so let's give another one a shot 🤞 @bors p=101 (not important, just getting through the tree, feel free to interrupt) |
☀️ Test successful - checks-actions |
Finished benchmarking commit (5b24e12): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. 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.
CyclesThis benchmark run did not return any relevant results for this metric. 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: 659.726s -> 661.63s (0.29%) |
Remove useless `assume`s from `slice::iter(_mut)` You were right in rust-lang#111395 (comment), r? `@the8472` LLVM already removes these assumes while optimizing, as can be seen in <https://rust.godbolt.org/z/KTfWKbdEM>.
Remove useless `assume`s from `slice::iter(_mut)` You were right in rust-lang/rust#111395 (comment), r? `@the8472` LLVM already removes these assumes while optimizing, as can be seen in <https://rust.godbolt.org/z/KTfWKbdEM>.
Remove useless `assume`s from `slice::iter(_mut)` You were right in rust-lang/rust#111395 (comment), r? `@the8472` LLVM already removes these assumes while optimizing, as can be seen in <https://rust.godbolt.org/z/KTfWKbdEM>.
Currently, slice iterators over ZSTs store
end = start.wrapping_byte_add(len)
.That's slightly convenient for
is_empty
, but kinda annoying for pretty much everything else -- see bugs like #42789, for example.This PR instead changes it to just
end = ptr::invalid(len)
instead.That's easier to think about (IMHO, at least) as well as easier to represent.
next
is still to big to get inlined into the mir-opt/pre-codegen/ tests, but if I bump the inline threshold to force it to show the whole thing, this implementation is also less MIR:(That's ≈70 lines less for
Iter::next
, for example.)r? @ghost
Built atop #111282, so draft until that lands.