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

Introduce `join_context` which tells whether jobs were stolen #354

Merged
merged 6 commits into from Oct 11, 2017

Conversation

Projects
None yet
3 participants
@cuviper
Copy link
Member

cuviper commented Jun 1, 2017

This removes the need for tracking thread-id or hacky TLS variables in
the adaptive splitter. Instead, with join_context the parallel
iterator internals are directly informed whether a job is executing on
its original thread or stolen elsewhere.

Note that this is insta-stable since we want to use it ourselves. We
could hide docs and/or mark it deprecated to approach stability a little
differently than the usual "unstable" feature.

@leodasvacas

This comment has been minimized.

Copy link
Contributor

leodasvacas commented Jun 5, 2017

Should the name be changed considering that unpark got renamed to notify in futures?

@cuviper

This comment has been minimized.

Copy link
Member Author

cuviper commented Jun 9, 2017

I think the places you'd use this are distinct enough from futures, but I'm open to other names.

@nikomatsakis

This comment has been minimized.

Copy link
Member

nikomatsakis commented Jun 10, 2017

I wonder if we should design this API in a more open-ended way. That is, right now it supplies a simple bool. But maybe it should supply a struct with methods, so that we can add additional information in the future.

@nikomatsakis

This comment has been minimized.

Copy link
Member

nikomatsakis commented Jun 10, 2017

As an example, I think it'd potentially be useful to tell closure B whether it was stolen or not (irrespective of whether we wound up jumping into the Rayon thread-pool). Right now, closure B can't tell the difference between the two.

@nikomatsakis

This comment has been minimized.

Copy link
Member

nikomatsakis commented Jun 10, 2017

Apart from that nit, though, I think this looks pretty nice, and I'm inclined to say I like it better than the current hack of testing which thread you are on. It feels cleaner to me to be told, particularly since it doesn't require a lot of work on our part to track that information.

@cuviper cuviper force-pushed the cuviper:join_notify branch from 2088fe5 to c829f21 Jun 24, 2017

@cuviper

This comment has been minimized.

Copy link
Member Author

cuviper commented Jun 24, 2017

Rebased and renamed to join_context with an opaque FnContext struct, still just a single bool for now. I didn't call it just JoinContext because I thought it might be a useful addition to stuff like spawn too. Alternate name suggestions are welcome.

join_context-points

join_context-points-medians

I think that looks ok? Some gains, some losses -- seem like noise.

/// outside the thread pool to begin with.
pub fn join_context<A, B, RA, RB>(oper_a: A, oper_b: B) -> (RA, RB)
where A: FnOnce(FnContext) -> RA + Send,
B: FnOnce(FnContext) -> RB + Send,

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 7, 2017

Member

Hmm, I think I expected &FnContext, but I guess there's not a ton of reason for that. If we wanted to maximally future-proof, I think we would pass a FnContext<'a>, where 'a is (for the moment) just a phantom-lifetime. This would give us the ability to have FnContext grow some amount of by-reference data later.

e.g.

struct FnContext<'a> {
    migrated: bool,
    dummy: PhantomData<&'a ()>,
}

This comment has been minimized.

@cuviper

cuviper Jul 7, 2017

Author Member

Then its Default seems weird to me, which is used by the bridge functions in their starting state. Should we allow arbitrary FnContext<'a> in that case, or just FnContext<'static>?

Perhaps we should also force this !Send and !Sync if we think it might reference local state.

This comment has been minimized.

@nikomatsakis

nikomatsakis Jul 11, 2017

Member

Hmm I think I didn't notice that it supports Default. Maybe it's overkill to worry about this stuff anyway.

@cuviper cuviper force-pushed the cuviper:join_notify branch from d6b30b5 to c6d3367 Sep 17, 2017

@cuviper

This comment has been minimized.

Copy link
Member Author

cuviper commented Sep 17, 2017

Rebased.

_marker: PhantomData<*mut ()>,
}

impl Default for FnContext {

This comment has been minimized.

@nikomatsakis

nikomatsakis Sep 22, 2017

Member

Do we really want this to support default? That means anybody can make one...?

This comment has been minimized.

@cuviper

cuviper Sep 23, 2017

Author Member

I thought I needed a way for the iterator bridge functions to create one -- which is as good as "anybody" as far as crate privacy goes -- but I just modified those to extract the migrated bool to pass around instead.

@cuviper cuviper force-pushed the cuviper:join_notify branch from 863bc12 to b6d4e2e Sep 23, 2017

@cuviper cuviper force-pushed the cuviper:join_notify branch from b6d4e2e to acf4aad Oct 6, 2017

@cuviper cuviper changed the title Introduce `join_notify` which tells whether jobs were stolen Introduce `join_context` which tells whether jobs were stolen Oct 6, 2017

cuviper added some commits Jun 1, 2017

Introduce `join_notify` which tells whether jobs were stolen
This removes the need for tracking thread-id or hacky TLS variables in
the adaptive splitter.  Instead, with `join_notify` the parallel
iterator internals are directly informed whether a job is executing on
its original thread or stolen elsewhere.

Note that this is insta-stable since we want to use it ourselves.  We
could hide docs and/or mark it deprecated to approach stability a little
differently than the usual "unstable" feature.

@cuviper cuviper force-pushed the cuviper:join_notify branch from acf4aad to a466f66 Oct 7, 2017

@nikomatsakis

This comment has been minimized.

Copy link
Member

nikomatsakis commented Oct 10, 2017

bors r+

bors bot added a commit that referenced this pull request Oct 10, 2017

Merge #354 #454
354: Introduce `join_context` which tells whether jobs were stolen r=nikomatsakis a=cuviper

This removes the need for tracking thread-id or hacky TLS variables in
the adaptive splitter.  Instead, with `join_context` the parallel
iterator internals are directly informed whether a job is executing on
its original thread or stolen elsewhere.

Note that this is insta-stable since we want to use it ourselves.  We
could hide docs and/or mark it deprecated to approach stability a little
differently than the usual "unstable" feature.

454: Remove `rayon::split`, leaving only `rayon::iter::split` r=nikomatsakis a=cuviper

The `split` function creates a `ParallelIterator`, which better
justifies its place in `rayon::iter`.  It's not really used enough to
justify its place in the crate root.

Fixes #432.
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Oct 11, 2017

@bors bors bot merged commit a466f66 into rayon-rs:master Oct 11, 2017

2 checks passed

bors Build succeeded
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cuviper cuviper deleted the cuviper:join_notify branch Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.