-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Implement more methods for vec_deque::IntoIter
#106241
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
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
1d7c644
to
7c13b14
Compare
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.
Have you benchmarked whether they're better than the default impls? If yes please post before/after results.
Vec::IntoIter overrides those methods because it often results in vectorizable code while the default impls don't.
E.g. I suspect next_chunk might benefit less than Vec does because it'll see non-const-length copies even on the Ok
branch while Vec only does that for Err
.
I don't think there are currently any benchmarks for |
Yes, please do. If there aren't significant wins we might as well stick with the defaults. @rustbot author |
@the8472 I added some benchmarks for
Seems like manually implementing |
@rustbot ready |
Hrrm, I don't think this makes sense. fold shouldn't be slower than try_fold. The issue is that fold takes ownership of the iterator and drops it, so you can't reuse the allocation when benchmarking In that case we just have to bite the bullet and measure the allocation costs together with the iterator performance. |
Right, that makes sense. Makes me think, would it maybe make sense to implement the default |
There have been several attempts to do that. In fact there currently is an open PR to try that again. |
461d0f7
to
ccba6c5
Compare
I changed the
|
@the8472 friendly reminder :) |
@bors r+ rollup=never |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
Finished benchmarking commit (4507fda): comparison URL. Overall result: ✅ improvements - 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.
CyclesThis benchmark run did not return any relevant results for this metric. |
This implements a couple
Iterator
methods onvec_deque::IntoIter
((try_)fold
,(try_)rfold
advance_(back_)by
,next_chunk
,count
andlast
) to allow these to be more efficient than their default implementations, also allowing many otherIterator
methods that use these under the hood to take advantage of these manual implementations.vec::IntoIter
has similar implementations for many of these methods. This PR does not yet implementTrustedRandomAccess
and friends, as I'm not very familiar with the required safety guarantees.r? @the8472 (since you also took over my last PR)