-
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
[WIP] Create Scan parallel iterator #1036
base: main
Are you sure you want to change the base?
Conversation
Write a parallel scan function that consumes a parallel iterator, then produces a new one. Add some benchmarking and testing.
src/iter/mod.rs
Outdated
@@ -1384,6 +1387,14 @@ pub trait ParallelIterator: Sized + Send { | |||
sum::sum(self) | |||
} | |||
|
|||
fn scan<F>(self, scan_op: F, id: Self::Item) -> Scan<Self::Item, F> |
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's start here, documenting at the API level, especially for a user who is looking for an equivalent to Iterator::scan
. How would you describe what this does, and importantly how is it different than the sequential version? The signature of F
is quite different, looking more like some flavor of reduce
.
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.
Good question, I wasn't exactly sure what the signature should be, and it could change. But it's similar to the way reduce
compares to fold
. This should have an identical result to sequential scan when the operation doesn't have internal state, and is associative. If not, it won't make sense to run it in parallel.
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.
Hi @cuviper, could you take another look at this?
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 don't feel you addressed my first comment? The signature is part of it, but please make an attempt at adding documentation describing what this does. And frankly, I don't think many people even use Iterator::scan
, so it's not sufficient to just reference that. Suppose this existed independently - describe what it does, and what points are important for the user to think about.
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.
Sorry, I misunderstood you last time. I've added documentation.
I was hoping to get some feedback on the signature and general approach. These are the differences from the other ParallelIterator functions, and my rationale for them:
- The scan operation currently has type
Fn(&Item, &Item) -> Item
, since when we call it iteratively, we need to keep each intermediate result and not consume it. - Since
scan_op
takes in references, we don't need multiple copies ofidentity
. So, I haveidentity
as typeItem
, rather thanFn() -> Item
, since it's simpler. The other functions useFn() -> Item
, though, and it would be easy to change.
Do you have any preferences on those?
Hi, here's a draft of a parallel scan function based on discussion in #329 and #393.
General Approach
Vec<T>
. The reduce step combines them into aLinkedList<Vec<T>>
.Vec<Vec<T>>
so we can operate on it in parallel again.In order to maximize the parallelism, we don't want to split too much, since that creates more sequential work. I've been using
.with_min_len()
in order to prevent that.Happy to take feedback on better ways to do any of these steps.
Benchmarks
I also tried writing some benchmarks. I ran these on my M2 Mac with 12 threads.
Potential Improvements