-
Notifications
You must be signed in to change notification settings - Fork 501
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
Add ParallelIterator::collect_vec_list #1128
Conversation
src/iter/collect/mod.rs
Outdated
// Pseudo-specialization. See impl of ParallelExtend for Vec for more details. | ||
let mut v = Vec::new(); | ||
super::collect::special_extend(pi, len, &mut v); | ||
LinkedList::from([v]) |
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.
Wasn't sure if I should re-use pi.collect::<Vec<_>>()
here or not; theoretically that'd have an unnecessary second check of opt_len() but that would get optimized in almost all cases.
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 way you've done it is fine, especially since that internal method is already shared pub(super)
.
The other thing I noticed is that the unindexed case avoids pushing empty vecs to the list, but here it will always create that one node. I'm not sure that matters though -- it's potentially more meaningful when there could end up being many empty list nodes, instead of just this one in a degenerate case.
src/iter/collect/mod.rs
Outdated
match pi.opt_len() { | ||
Some(len) => { | ||
// Pseudo-specialization. See impl of ParallelExtend for Vec for more details. | ||
let mut v = Vec::new(); |
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.
let mut v = Vec::new(); | |
let mut v = Vec::with_capacity(len); |
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 suggested change is fine, but I think it won't really matter since special_extend
-> collect_with_consumer
will call reserve
anyway. Allocate now or later, but we're still only doing it once.
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.
Right, that was my thinking, and the ParallelExtend impl doesn't reserve here either.
I think that bit of code is short enough that it could just be implemented directly in the trait method. |
Co-authored-by: Josh Stone <[email protected]>
Something I was thinking of including in the docs but wasn't sure about - mentioning the pattern of calling |
057541d
to
c8ab421
Compare
Also, added a check for length of 0 returning an empty LL, though yeah I'm not sure if that's necessary. |
d8e9d1c
to
b5a9a0e
Compare
62a6129
to
3805762
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.
Looks good, thanks!
Would it be possible to cut a release with this? |
This is now published in rayon 1.9.0. |
Thank you so much! |
I wasn't exactly sure where the
collect_vec_list
implementation should go, since the necessary internals are inextend
but it's logically acollect
function. Also, I feel like the documentation could be better, I'd appreciate feedback on that.Resolves #1127