Add `par_extend`, `partition`, `unzip`, and `unzip_into` #326

cuviper · 2017-04-26T23:31:52Z

ParallelExtend mirrors std::iter::Extend, with one method par_extend. It works basically like FromParallelIterator, except it adds to an existing container instead of creating a new one. In fact, most of the FromParallelIterators we implement now just create an empty container and then par_extend it.
ParallelIterator::partition mirrors Iterator::partition, separating the iterator into two ParallelExtend containers based on the user's predicate function.
ParallelIterator::unzip mirrors Iterator::unzip, separating an iterator over (A, B) into two ParallelExtend containers, one with just A and the other with just B.
IndexedParallelIterator::unzip_into is analogous to collect_into, writing directly into the target vectors.

cuviper · 2017-04-26T23:34:40Z

@nikomatsakis In the wiki you said that Iterator::partition really ought to allow two distinct output types. It wouldn't be very hard for me to do that here if you want.

cuviper · 2017-04-26T23:37:24Z

The nightly failures are rust-lang/rust#41479. It does work with nightly-2017-04-21.

cuviper · 2017-04-27T00:24:22Z

src/iter/unzip.rs

+
+    fn opt_len(&mut self) -> Option<usize> {
+        self.base.opt_len()
+    }


This one got me thinking. It's fine to leak the source-iterator's index-ness through opt_len() here, and it should result in the optimization we want. However, with an eye on moving that mechanism to specialization, I can't actually implement a true IndexedParallelIterator for this!

I could implement IndexedParallelIterator::drive(), but not with_producer() because of the iterator requirement in Producer. We need to stay in "push" mode here to feed values into multiple consumers, but once we flip to an iterator we're left pulling. And that means we can't support zip et al. on this -- which I wouldn't expect in a par_extend() implementation anyway, but with specialization one could encounter that.

Maybe this is a distinction which deserves to split up the traits again. An IndexedParallelIterator that can drive(), enumerate(), etc., and a subset ProducableParallelIterator that supports with_producer(), zip(), etc.

Ugh.

(Incidentally, this "push vs. pull" is basically why Iterator::unzip requires Extend instead of FromIterator.)

Interesting. It's also worth considering whether using specialization is worth it versus the current opt_len() trick.

cuviper · 2017-04-27T17:34:15Z

There's also Itertools::partition_map we could emulate, which can map to different item types and allows different collection types. That would be yet another full implementation, where unzip.rs and partition.rs are already very similar... maybe they can be merged and abstracted so that only the Folder::consume is really different. Kind of like we used to with MapOp, but this code is trickier and also hidden from the API, so it's more favorable to sharing.

cuviper · 2017-04-27T19:38:22Z

I merged that common functionality, generalized partition, and added partition_map.

cuviper · 2017-04-28T21:56:41Z

Added FromParallelIterator<&char> for String since Rust 1.17 now has it -- rust-lang/rust#40028.
(not directly related to this PR, but it touches the same code as extend, so consider it a bonus...)

nikomatsakis

This is awesome. I had plumb forgot that we still had a few things unchecked (and that they included such generally useful things as unzip() and partition()!). We should open issues for the remainder (which just appears to be scan() and cycle() -- scan() may just not make sense, cycle() seems achievable though).

nikomatsakis · 2017-04-30T11:04:57Z

src/iter/unzip.rs

+
+
+/// Unzip an `IndexedParallelIterator` into two arbitrary `Consumer`s.
+pub fn unzip_indexed<I, A, B, CA, CB>(pi: I, left: CA, right: CB)


Are these intended to be Rayon public APIs? (I'd appreciate a comment if not.)

nikomatsakis · 2017-04-30T11:05:01Z

src/iter/collect/mod.rs

@@ -39,6 +40,26 @@ fn special_extend<I, T>(pi: I, len: usize, v: &mut Vec<T>)
    collect.complete();
 }

+/// Unzips the results of the exact iterator into the specified vectors.
+pub fn unzip_into<I, A, B>(mut pi: I, left: &mut Vec<A>, right: &mut Vec<B>)


Are these intended to be Rayon public APIs? (I'd appreciate a comment if not.)

nikomatsakis · 2017-04-30T11:08:41Z

src/iter/unzip.rs

+
+    fn opt_len(&mut self) -> Option<usize> {
+        self.base.opt_len()
+    }


Interesting. It's also worth considering whether using specialization is worth it versus the current opt_len() trick.

nikomatsakis · 2017-04-30T11:14:30Z

src/iter/partition.rs

+    {
+        let mut result = None;
+        {
+            // Now it's time to find the consumer for non-matching items


Clever trick, I will say. I wonder if we can use some internal traits + generics to "package up" this pattern rather than repeating it. It would probably make things less clear though, although it would provide a good place to document what is going on.

nikomatsakis · 2017-04-30T11:16:15Z

src/iter/unzip.rs

@@ -1,15 +1,24 @@
 use super::internal::*;
 use super::*;

+trait UnzipOp<T>: Sync {


I think perhaps you were a step ahead of me. Good place for comments, though!

nikomatsakis · 2017-04-30T11:19:31Z

src/iter/mod.rs

-        where C: Default + ParallelExtend<Self::Item>,
+    fn partition<A, B, P>(self, predicate: P) -> (A, B)
+        where A: Default + ParallelExtend<Self::Item>,
+              B: Default + ParallelExtend<Self::Item>,


Yay! My only hesitation is that it means that transition to rayon may require more type annotations. e.g., with seq iterators, people can do:

let (x, y): (Vec<_>, _) = iter.partition();

maybe we should add another method for this case? (We could then propose the same method for seq iterators, I suppose). But I don't know what would be a good name.

nikomatsakis · 2017-04-30T11:22:04Z

src/iter/mod.rs

+    /// Partitions and maps the items of a parallel iterator into a pair of
+    /// arbitrary `ParallelExtend` containers.  `Either::Left` items go into
+    /// the first container, and `Either::Right` items go into the second.
+    fn partition_map<A, B, P, L, R>(self, predicate: P) -> (A, B)


This is meant for parity with itertools, I guess? Makes sense. I was wondering if we should link directly to itertools, so we can use the same Either type, but it's problematic that it is at only 0.6 I suppose, and it seems a bit silly to add an itertools dependency just for Either.

(cc @bluss -- as an aside, any reason itertools can't be 1.0?)

nikomatsakis · 2017-04-30T11:25:35Z

So the only real question that came up in my review is whether to have partition() return two collections or not. It depends how much we care about compatibility. We could include a note in the docs that our signature is more flexible that the iterator one and hence may require an additional type annotation. I think I'm inclined to do that for now, and just leave it the way you have it.

(I have definitely needed two distinct collection types in the past and been annoyed that I can't have it.)

cuviper · 2017-04-30T16:44:52Z

Yeah, I'm ok with the possible ambiguity, since it would only bite at compile time, not a runtime surprise, and it's simple to correct.

I'll add more comments where requested.

nikomatsakis · 2017-05-02T20:41:23Z

r=me, after rebase

bluss · 2017-08-04T13:25:43Z

Sorry for the very late reply 😄. Itertools is working towards 1.0, it doesn't feel very far off.

bluss · 2017-08-04T13:26:51Z

That said we don't have a "tracking issue" for it. One reason might be that if std adopts anything from itertools using the same method name, we have a problem.

cuviper commented Apr 27, 2017

View reviewed changes

cuviper mentioned this pull request Apr 27, 2017

Need an unzip() operator. #323

Closed

nikomatsakis approved these changes Apr 30, 2017

View reviewed changes

cuviper added 11 commits May 2, 2017 13:44

Add a ParallelExtend trait

5a86ddd

Add IndexedParallelIterator::unzip_into

cd838ed

Add ParallelIterator::unzip

9a5ba40

Add ParallelIterator::partition

0182458

Abstract the unzip implementation

3319da5

Use the unzip abstraction for partition

99651cd

Add ParallelIterator::partition_map

004b463

Let partition output different collection types

612c4ef

Add FromParallelIterator<&char> for String

a6fd1c8

Document more of the unzip implementation

a591d70

Note partition's ability to use distinct types

82ca764

cuviper force-pushed the extend-unzip-partition branch from de19391 to 82ca764 Compare May 2, 2017 21:01

cuviper merged commit 81e3d2c into rayon-rs:master May 3, 2017

cuviper deleted the extend-unzip-partition branch September 23, 2017 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `par_extend`, `partition`, `unzip`, and `unzip_into` #326

Add `par_extend`, `partition`, `unzip`, and `unzip_into` #326

cuviper commented Apr 26, 2017

cuviper commented Apr 26, 2017

cuviper commented Apr 26, 2017

cuviper Apr 27, 2017

nikomatsakis Apr 30, 2017

cuviper commented Apr 27, 2017

cuviper commented Apr 27, 2017

cuviper commented Apr 28, 2017

nikomatsakis left a comment

nikomatsakis Apr 30, 2017

nikomatsakis Apr 30, 2017

nikomatsakis Apr 30, 2017

nikomatsakis Apr 30, 2017

nikomatsakis Apr 30, 2017

nikomatsakis Apr 30, 2017

nikomatsakis Apr 30, 2017

nikomatsakis commented Apr 30, 2017

cuviper commented Apr 30, 2017

nikomatsakis commented May 2, 2017

bluss commented Aug 4, 2017

bluss commented Aug 4, 2017



		/// Unzip an `IndexedParallelIterator` into two arbitrary `Consumer`s.
		pub fn unzip_indexed<I, A, B, CA, CB>(pi: I, left: CA, right: CB)

Add par_extend, partition, unzip, and unzip_into #326

Add par_extend, partition, unzip, and unzip_into #326

Conversation

cuviper commented Apr 26, 2017

cuviper commented Apr 26, 2017

cuviper commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cuviper commented Apr 27, 2017

cuviper commented Apr 27, 2017

cuviper commented Apr 28, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Apr 30, 2017

cuviper commented Apr 30, 2017

nikomatsakis commented May 2, 2017

bluss commented Aug 4, 2017

bluss commented Aug 4, 2017

Add `par_extend`, `partition`, `unzip`, and `unzip_into` #326

Add `par_extend`, `partition`, `unzip`, and `unzip_into` #326