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.

Sign up for GitHub

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

Jump to bottom

Add method fold_with to Producer and UnindexedProducer #184

Merged
merged 2 commits into from
Dec 21, 2016
Merged

Add method fold_with to Producer and UnindexedProducer #184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7fa8b8d
Add method fold_with to Producer and UnindexedProducer
bluss Dec 19, 2016
31449c8
Implement fold_with for ChainProducer
bluss Dec 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/par_iter/chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,17 @@ impl<A, B> Producer for ChainProducer<A, B>
ChainProducer::new(0, a_right, b_right))
}
}

fn fold_with<F>(self, mut folder: F) -> F
where F: Folder<A::Item>,
{
folder = self.a.fold_with(folder);
if folder.full() {
folder
} else {
self.b.fold_with(folder)
}
}
}

impl<A, B> IntoIterator for ChainProducer<A, B>
Expand Down
50 changes: 34 additions & 16 deletions src/par_iter/internal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,22 @@ pub trait Producer: IntoIterator + Send + Sized {
/// Split into two producers; one produces items `0..index`, the
/// other `index..N`. Index must be less than `N`.
fn split_at(self, index: usize) -> (Self, Self);

/// Iterate the producer, feeding each element to `folder`, and
/// stop when the folder is full (or all elements have been consumed).
///
/// The provided implementation is sufficient for most iterables.
fn fold_with<F>(self, mut folder: F) -> F
where F: Folder<Self::Item>,
{
for item in self {
folder = folder.consume(item);
if folder.full() {
break;
}
}
folder
}
}

/// A consumer which consumes items that are fed to it.
Expand Down Expand Up @@ -95,6 +111,22 @@ pub trait UnindexedConsumer<ITEM>: Consumer<ITEM> {
pub trait UnindexedProducer: IntoIterator + Send + Sized {
fn can_split(&self) -> bool;
fn split(self) -> (Self, Self);

/// Iterate the producer, feeding each element to `folder`, and
/// stop when the folder is full (or all elements have been consumed).
///
/// The provided implementation is sufficient for most iterables.
fn fold_with<F>(self, mut folder: F) -> F
where F: Folder<Self::Item>,
{
for item in self {
folder = folder.consume(item);
Copy link
Member

Choose a reason for hiding this comment

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

At some point, we discussed whether we should have Producer: UnindexedProducer, but we didn't have a good reason for it (even though it logically makes sense). The existence of this duplicate code might provide such a reason.

Copy link
Contributor Author

@bluss bluss Dec 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I'm new to the internals, maybe split_at can be removed and only split is left?

bridge_producer_consumer (indexed case) is this today:

            let mid = len / 2;
            let (left_producer, right_producer) = producer.split_at(mid);
            let (left_consumer, right_consumer, reducer) = consumer.split_at(mid);

It could change to something like:

            let (left_producer, right_producer) = producer.split();
            let mid = left_producer.len();
            let (left_consumer, right_consumer, reducer) = consumer.split_at(mid);

so the difference is that an indexed producer can tell its length (and that it has a meaningful length or before / after ordering of the halves).

ndarray's unindexed producer can tell the length of each part perfectly. What sets it apart is that (1) it can't split exactly at len / 2 since it must split in chunks of some axis' size (2) It doesn't want to split so that it has a "before" and "after" half. It prefers an unordered split for performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

About order:

Take a 3 × 4 array

[[1, 2, 3, 4],
 [5, 6, 7, 8],
 [9, 0, 1, 2]];

The logical order of the regular .iter() is defined to be "logical" row major, which means the elements in the order we read them: 1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2. The memory order of the array is separated from the logical order of .iter().

For parallelism, we guarantee no order and the unindexed producer for an array view will pick either of these two splits, depending on which preserves memory locality:

[[1, 2, | 3, 4],
 - - - - - - - - - v.split_at(Axis(0), 1) // best for row major memory order
 [5, 6, | 7, 8],
 [9, 0, | 1, 2]];
        \ v.split_at(Axis(1), 2)  // best for column major memory order

Copy link
Member

Choose a reason for hiding this comment

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

No, you need split_at in order to handle zip.

Copy link
Member

Choose a reason for hiding this comment

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

(That said, I did want to consider changing the structure so that the producer can offer a suggestion about where to split instead. But I haven't decided on the best way to do it.)

Copy link
Member

Choose a reason for hiding this comment

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

So the key distinctions are:

  • can split at an arbitrary point
    • needed for zip
  • knows exactly how many elements it will produce
    • needed for precise consumers

Today we tie those together, but they could be separated. Something like:

  • UnindexedProducer
    • can split itself at some point
  • KnownLengthProducer: UnindexedProducer
    • can tell you the number of items it will produce
  • Producer: KnownLengthProducer
    • can further be split at an arbitrary point

we might then have two bridge methods:

  • one takes a KnownLengthProducer and a Consumer
    • this is the one that today requires a Producer
    • it would instead invoke producer.split(), get resulting length, and invoke consumer.split_at(), as you suggested
  • one takes a UnindexedProducer and a UnindexedConsumer
    • same as today

We would probably have to tweak the ParallelIterator hierarchy a bit too, but the idea is that zip() would require that its sub-iterators can produce a full Producer (or, potentially, that the LHS can produce a KnownLengthProducer and the RHS can produce a full Producer). It's split() implementation can then call left.split() and right.split_at(left_split.len())`.

Make sense?

Copy link
Contributor Author

@bluss bluss Dec 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yes that makes sense. I'm happy with the two traits that are now; since the UnindexedProducer here didn't want to participate in zip in any way even if it can tell its exact length, the reason is that it doesn't produce elements in any particular order.

Copy link
Member

Choose a reason for hiding this comment

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

At some point, we discussed whether we should have Producer: UnindexedProducer, but we didn't have a good reason for it (even though it logically makes sense). The existence of this duplicate code might provide such a reason.

Yes, although IIRC we were also wanting impl<T: Producer> UnindexedProducer for T to avoid duplicating can_split/split boilerplate, but that would really want specialization. Plus that boilerplate would never actually be invoked.

In the case of just fold_while, perhaps we could make a BaseProducer common to both, so the boilerplate is a one-line empty impl in most cases. Or we could extract this body to a shared free function just to avoid the duplication. But this is only 6 lines, let's not overthink it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd vote for not more trivial traits to implement, unless they do something noticeable.

Copy link
Member

Choose a reason for hiding this comment

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

Yet another option is to add something like:

trait Folder {
    // ...
    fn consume_iter<I:IntoIterator>(self, iterator: I) -> Self {
        // ...
    }
}

... which might be useful to folks specializing fold_with too.

if folder.full() {
break;
}
}
folder
}
}

/// A splitter controls the policy for splitting into smaller work items.
Expand Down Expand Up @@ -207,14 +239,7 @@ pub fn bridge_producer_consumer<P, C>(len: usize, mut producer: P, mut consumer:
|| helper(len - mid, splitter, right_producer, right_consumer));
reducer.reduce(left_result, right_result)
} else {
let mut folder = consumer.into_folder();
for item in producer {
folder = folder.consume(item);
if folder.full() {
break;
}
}
folder.complete()
producer.fold_with(consumer.into_folder()).complete()
}
}
}
Expand Down Expand Up @@ -245,13 +270,6 @@ fn bridge_unindexed_producer_consumer<P, C>(mut splitter: Splitter,
|| bridge_unindexed_producer_consumer(splitter, right_producer, right_consumer));
reducer.reduce(left_result, right_result)
} else {
let mut folder = consumer.into_folder();
for item in producer {
folder = folder.consume(item);
if folder.full() {
break;
}
}
folder.complete()
producer.fold_with(consumer.into_folder()).complete()
}
}