Skip to content
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

Merged
merged 5 commits into from
Feb 9, 2024

Conversation

coolreader18
Copy link
Contributor

@coolreader18 coolreader18 commented Feb 7, 2024

I wasn't exactly sure where the collect_vec_list implementation should go, since the necessary internals are in extend but it's logically a collect function. Also, I feel like the documentation could be better, I'd appreciate feedback on that.

Resolves #1127

Comment on lines 78 to 81
// 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])
Copy link
Contributor Author

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.

Copy link
Member

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.

match pi.opt_len() {
Some(len) => {
// Pseudo-specialization. See impl of ParallelExtend for Vec for more details.
let mut v = Vec::new();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut v = Vec::new();
let mut v = Vec::with_capacity(len);

Copy link
Member

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.

Copy link
Contributor Author

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.

@cuviper
Copy link
Member

cuviper commented Feb 7, 2024

I wasn't exactly sure where the collect_vec_list implementation should go, since the necessary internals are in extend but it's logically a collect function.

I think that bit of code is short enough that it could just be implemented directly in the trait method.

src/iter/mod.rs Outdated Show resolved Hide resolved
Co-authored-by: Josh Stone <[email protected]>
@coolreader18
Copy link
Contributor Author

coolreader18 commented Feb 7, 2024

Something I was thinking of including in the docs but wasn't sure about - mentioning the pattern of calling .into_iter().flatten() on the linked list? Like, .collect_vec_list().into_iter().flatten() is now a very straightforward/efficient way of turning a parallel iterator into a serial one, though I'm unsure if that should be encouraged or where in the docs that would go.

@coolreader18
Copy link
Contributor Author

Also, added a check for length of 0 returning an empty LL, though yeah I'm not sure if that's necessary.

@coolreader18 coolreader18 force-pushed the collect_vec_list branch 2 times, most recently from 62a6129 to 3805762 Compare February 7, 2024 20:46
Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@cuviper cuviper added this pull request to the merge queue Feb 9, 2024
Merged via the queue into rayon-rs:master with commit a9f676b Feb 9, 2024
4 checks passed
@coolreader18 coolreader18 deleted the collect_vec_list branch February 9, 2024 18:45
@coolreader18
Copy link
Contributor Author

Would it be possible to cut a release with this?

@cuviper
Copy link
Member

cuviper commented Feb 27, 2024

This is now published in rayon 1.9.0.

@coolreader18
Copy link
Contributor Author

Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for some sort of collect()/extend() into LinkedList<Vec<T>>
3 participants