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 map_with and fold_with #318

Merged
merged 1 commit into from
May 2, 2017
Merged

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Apr 16, 2017

These provide a means to use additional values with a parallel iterator,
without excessive cloning nor requiring Sync. This sort of need has
most often come up for using mpsc::Sender.

    fn map_with<F, T, R>(self, init: T, map_op: F) -> MapWith<Self, T, F>
        where F: Fn(&mut T, Self::Item) -> R + Sync,
              T: Send + Clone,
              R: Send;

    fn fold_with<F, T>(self, init: T, fold_op: F) -> FoldWith<Self, T, F>
        where F: Fn(T, Self::Item) -> T + Sync,
              T: Send + Clone;

These provide a means to use additional values with a parallel iterator,
without excessive cloning nor requiring `Sync`.  This sort of need has
most often come up for using `mpsc::Sender`.
@cuviper
Copy link
Member Author

cuviper commented Apr 16, 2017

FWIW, I don't love these names myself, especially since we have producer/consumer fold_with that's totally different. Suggestions welcome.

@nikomatsakis
Copy link
Member

OK, so let me try to rephrase what these do:

  • You provide an initial (cloneable) value T.
  • When we get to the sequential phase, you get mutable access to this T so you can (e.g.) accumulate values in it or whatever.
  • This is then a (better) alternative to e.g. threading a Mutex<Sender<T>> around.

Man. I see why this is handy, but I agree the name is not intuitive.

I wonder if we could do this in a more universal way. I don't quite see how to make this work yet, but the rough idea is that each time we split, instead of just copying a &F (where F is the closure type) to two places, we would "split" the closure. For normal Fn closures, that would be a no-op, but one could also supply a ThreadedClosure<'a> { mut_state: X, f: &'a Fn(&mut X, T) } (which would clone the state.

(Certainly we could do this as an implementation strategy, but I had sort of hoped to make a "drop-in" thing that we can use with combinators, so that we do foo.map(threaded(x, |x, e| ...)), but I don't see how to do that yet. Part of the problem are some limitations in Rust's current inference engine, which I am feeling increasingly motivated to try and solve.)

@cuviper
Copy link
Member Author

cuviper commented Apr 19, 2017

Ooh, the "drop-in" idea is interesting! I was already struggling with how far to extend this, whether to add for_each_with etc., and decided that map_with and fold_with were the fundamental primitives. But if we can make this work more generically for almost everything that takes a closure, that would be great!

I think this is roughly similar to the sort of abstraction we already use in the string par_split, which uses a trait to accept either char or Fn patterns. Here we'd want a sort of ThreadableFn trait implemented by any F: Fn + Sync, and also by a custom type returned from your threaded wrapper.

I'll play with it...

@cuviper
Copy link
Member Author

cuviper commented Apr 19, 2017

Part of the problem are some limitations in Rust's current inference engine

OK, I think I see what you mean. I got it basically working, and I get new errors like:

error: the type of this value must be known in this context
   --> src/iter/test.rs:976:35
    |
976 |     a.par_iter_mut().for_each(|x| *x = -*x);
    |                                   ^^

... but that line works if I annotate |x: &mut i32|. That's no fun.

@nikomatsakis
Copy link
Member

OK, I think I see what you mean. I got it basically working, and I get new errors like:

Yeah. That said, I think we can fix this in rustc.

@nikomatsakis
Copy link
Member

@cuviper do you have that work on a branch somewhere? I want to investigate how hard it would be to fix this inference problem in rustc; but I guess even if we did so, we'd have to wait until that hit stable etc.

@cuviper
Copy link
Member Author

cuviper commented Apr 25, 2017

I just pushed it to cuviper:threadable, with the general pieces in place but only for_each converted so far. cargo build is fine, then cargo test reveals the inference problem.

@nikomatsakis
Copy link
Member

@cuviper so I think we should consider landing these APIs for now, but I wouldn't want to go adding a bunch more, and I would hope we can deprecate them in the future in favor of a new kind of closure (I'll have to carve out some time to experiment on the rustc side). Thoughts?

@nikomatsakis
Copy link
Member

I was also debating about making them unstable, but I'm not sure that's worthwhile in the rayon crate anymore.

@cuviper
Copy link
Member Author

cuviper commented Apr 30, 2017

I'm good with merging. And I agree we should avoid adding more, unless we find a case that's really not possible to extend from these two. But e.g. someone who wants a similar filter_with can use map_with into Option<Item> and then filter_map that.

@nikomatsakis
Copy link
Member

BTW this is the rustc issue that blocks inference: rust-lang/rust#23012

@nikomatsakis nikomatsakis merged commit 8b4bca8 into rayon-rs:master May 2, 2017
@cuviper cuviper deleted the fold-map_with branch September 23, 2017 04:56
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.

2 participants