-
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
Make ZST checks in core/alloc more readable #102169
Conversation
There's a bunch of these checks because of special handing for ZSTs in various unsafe implementations of stuff. This lets them be `T::IS_ZST` instead of `mem::size_of::<T>() == 0` every time, making them both more readable and more terse. *Not* proposed for stabilization at this time. Would be `pub(crate)` except `alloc` wants to use it too. (And while it doesn't matter now, if we ever get something like 85836 making it a const can help codegen be simpler.)
r? @thomcc (rust-highfive has picked a reviewer for you, use r? to override) |
This comment was marked as resolved.
This comment was marked as resolved.
Hm, neat. I know you said it shouldn't impact codegen at all, but I'm a little curious. @bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit cbbcd9f with merge 62c55795633b779a3a1d8085381e862b9104bf4d... |
All I meant by that was that there's currently no special handling for I agree this should have a perf run, but was waiting on the PR build because I'm too good at under-testing locally. Looks like when I fixed some of the warnings(-as-errors) about unused imports I broke the docs 🤦 |
💔 Test failed - checks-actions |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 96314f9cfb3a489b83e05ce70a48ef045aca6d62 with merge 20e3190ff975b91b5b541a7f5b264084b94ace56... |
☀️ Try build successful - checks-actions |
Queued 20e3190ff975b91b5b541a7f5b264084b94ace56 with parent 77e7e88, future comparison URL. |
Finished benchmarking commit (20e3190ff975b91b5b541a7f5b264084b94ace56): 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)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.
Footnotes |
Hmm, that's a weird build failure. RustDoc handled it fine, but now it's a linkchecker error? Local repro time, I guess -- |
96314f9
to
f0dc359
Compare
This comment has been minimized.
This comment has been minimized.
Thank you. @bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (e58621a): comparison URL. Overall result: ❌ regressions - no action needed@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.
Footnotes |
There's a bunch of these checks because of special handing for ZSTs in various unsafe implementations of stuff.
This lets them be
T::IS_ZST
instead ofmem::size_of::<T>() == 0
every time, making them both more readable and more terse.Not proposed for stabilization. Would be
pub(crate)
exceptalloc
wants to use it too.(And while it doesn't matter now, if we ever get something like #85836 making it a const can help codegen be simpler.)