-
Notifications
You must be signed in to change notification settings - Fork 14k
Implement IsZero for (), and optimize IsZero::is_zero for arrays
#148737
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
Conversation
library/alloc/src/vec/is_zero.rs
Outdated
| // We could probably just return `true` here, since implementing | ||
| // `IsZero` for a zero-sized type such that `self.is_zero()` returns | ||
| // `false` would be useless, but to be safe we check anyway. |
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 thought so too at first, but that would conflict with the above implementation – any [T; N] now implements IsZero, so e.g. [NonTrivialCloneButZST; 5] would hit this code path but mustn't be zero-initialised.
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.
This is in the specialization where T: IsZero, so this would not be run for T = NonTrivialCloneButZST, right?
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.
Oh, I see what you mean, if T = [NonTrivialCloneButZST; 5], it would be wrong to return true unconditionally.
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 updated the comment to explain why we can't just return true unconditionally.
@rustbot ready
Implement IsZero for (). Implement default `IsZero` for all arrays, only returning true if the array is empty (making the existing array impl for `IsZero` elements a specialization). Optimize `IsZero::is_zero` for arrays of zero-sized `IsZero` elements.
|
Thanks! |
|
🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened. |
| }; | ||
| } | ||
|
|
||
| impl_is_zero!((), |_: ()| true); // It is needed to impl for arrays and tuples of (). |
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.
fwiw this can just be |()| rather than |_: ()|
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 29a6971 (parent) -> c8f22ca (this PR) Test differencesShow 4 test diffs4 doctest diffs were found. These are ignored, as they are noisy. Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard c8f22ca269a1f2653ac962fe2bc21105065fd6cd --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
|
Finished benchmarking commit (c8f22ca): comparison URL. Overall result: ✅ improvements - no action needed@rustbot label: -perf-regression Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -2.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 477.532s -> 476.181s (-0.28%) |
These are probably not super useful optimizations, but they make it so that
vec![expr; LARGE_LENGTH]has better performance for someexprs, e.g.()and zero-valued integers in debug and release mode()or other zero-sizedIsZerotype in debug modevery rough benchmarks
(checking release mode to make sure this doesn't regress perf there)
The specific expression I ran into a perf issue that this PR addresses is
vec![[(); LARGE]; LARGE], as I was trying to demonstrateVec::into_flattenedpanicking on length overflow in the playground, but got a timeout error instead sincevec![[(); LARGE]; LARGE]took so long to run in debug mode (it runs fine on the playground in release mode)