-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Optimize core::char::CaseMappingIter
#122616
Optimize core::char::CaseMappingIter
#122616
Conversation
Godbolt says this saves a few instructions…
r? @Nilstrieb rustbot has assigned @Nilstrieb. Use r? to explicitly pick a reviewer |
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.
The changes seem fine, but this CaseMappingIter
seems pretty complex for doing a simple operation. What's the performance of just using an array::IntoIter
and stopping when that one sees a \0
?
I did a bit of digging on the origin of this type and it seems to have been around forever (addaa5b).
Makes the iterator 2*usize larger, but I doubt that matters much. In exchange, we save a lot on instruction count. In the absence of delegation syntax, we must forward all the specialized impls manually…
core::char::CaseMappingIter
layoutcore::char::CaseMappingIter
@Nilstrieb That is a much better approach, adopted. @rustbot -S-waiting-on-author S-waiting-on-review |
Thanks! |
One(char), | ||
Zero, | ||
} | ||
struct CaseMappingIter(core::array::IntoIter<char, 3>); |
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.
Unsure: are these something that people would commonly keep in a struct? Because while I do like the array::IntoIter
phrasing of this, it means that iter will be 2×usize bigger than it used to be.
Feels like probably not, more likely something they'd just use directly and toss away, rather than use partially and keep around, but figured I'd mention it just in case.
(I wish we had an ArraySize<N>
type that could be smaller for small N
.)
…ut, r=Nilstrieb Optimize `core::char::CaseMappingIter` Godbolt says this saves a few instructions… `@rustbot` label T-libs A-layout C-optimization
💔 Test failed - checks-actions |
The job Click to see the possible cause of the failure (guessed by this bot)
|
|
@bors retry #122671 (comment) |
☀️ Test successful - checks-actions |
Finished benchmarking commit (1c19595): 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.
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: 667.865s -> 668.406s (0.08%) |
Godbolt says this saves a few instructions…
@rustbot label T-libs A-layout C-optimization