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 the join! macro #406

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add the join! macro #406

wants to merge 1 commit into from

Conversation

fitzgen
Copy link

@fitzgen fitzgen commented Sep 3, 2017

The join! macro forks and joins many expressions at once. This is easier to use than deeply nesting calls to rayon::join.

join! {
    let x = fork || 1;
    let y = fork || 2;
    let z = fork || 3;
}

assert_eq!(x, 1);
assert_eq!(y, 2);
assert_eq!(z, 3);

Fixes #402


Note that I wasn't able to create a macro that could be used like:

let (x, y, z) = join!(|| 1, || 2, || 3);

Because there is no way to create gensyms for the temporaries that would help go from the deeply nesting tuples resulting from the nesting rayon::join calls to a single flat tuple. Rust ends up using the same variable for every tuple element in that case.

Second note: I couldn't figure out how to allow let <irrefutable pattern> = fork ..., so it only allows idents there.

Perhaps someone with better macro fu can resolve these issues.

r? @cuviper

@cuviper
Copy link
Member

cuviper commented Sep 6, 2017

You can let macro hygiene deal with the temporaries like this: (playground)

macro_rules! join {
...
    
    ( @ $( let $lhs:ident = $rhs:expr ; )+ ) => {
        { 
            let join!( < $( $lhs , )+ ) = join!( > $( $rhs , )+ );
            ($( $lhs ),+) // flattened tuple
        }
    };
    ( @ $( let $lhs:ident = $rhs:expr ; )* $x:expr $( , $xs:expr )*) => {
        join! { @
            $( let $lhs = $rhs ; )*
            let lhs = $x;
            $($xs),*
        }
    };

    ( $x:expr $( , $xs:expr )* ) => {
        join! { @ $x $( , $xs )* }
    };
}

They're all called lhs, but they are unique as far as the compiler is concerned, and still usable in the same scope when they're matched and flattened.

Might want to use a different sigil than < though, because that's a legitimate start of an expr. That's why I had to put the plain expr match at the end of the macro patterns.

@fitzgen
Copy link
Author

fitzgen commented Sep 11, 2017

@cuviper updated!

@cuviper
Copy link
Member

cuviper commented Sep 12, 2017

Second note: I couldn't figure out how to allow let = fork ..., so it only allows idents there.

Can you elaborate the problem with that? I just tried changing let...fork and @left to take :pat, and it seems to work fine!

@fitzgen
Copy link
Author

fitzgen commented Sep 12, 2017

Can you elaborate the problem with that? I just tried changing let...fork and @left to take :pat, and it seems to work fine!

I think I was trying to do it without the let in the hopes of somehow making arbitrary left hand sides work, eg self.whatever = fork || ...; too.

Anyways, I've added let <pattern> support and a test for it.

@cuviper
Copy link
Member

cuviper commented Sep 17, 2017

Can you rebase this for the CI fixes?

@nikomatsakis I'd also like to know what you think of this interface. I think the plain expr form is straightforward, but I'm unsure about the let pat = fork expr form, if only for the odd fork "keyword".

The `join!` macro forks and joins many expressions at once. This is easier to
use than deeply nesting calls to `rayon::join`.
@fitzgen
Copy link
Author

fitzgen commented Sep 18, 2017

Rebased.

Note that the fork doesn't need to be there, I just found it made it easier to understand that the x in let x = ... was being bound to the result of the closure, not the closure itself. Happy to remove fork if others don't like it.

@cuviper
Copy link
Member

cuviper commented Sep 18, 2017

I just found it made it easier to understand that the x in let x = ... was being bound to the result of the closure, not the closure itself.

That's a fair point! I'm not opposed to using fork, just want to bikeshed it a little. :) It does invoke the idea of "fork-join" parallelism though, which is nice.

We could just take plain expr instead of closures, and call it as join(|| expr, ...) ourselves.

join! {
    let x = 1;
    let y = 2;
    let z = 3;
}
// becomes:
let (x, (y, z)) = join(|| 1, join(|| 2, || 3));

But that's less flexible than taking a user's closure, and also implies a sequence that doesn't really exist -- e.g. z can't use x or y in its expression.

I don't have any great alternatives.

@fitzgen
Copy link
Author

fitzgen commented Sep 18, 2017

But that's less flexible than taking a user's closure

Yeah, and I think defining the closures is actually necessary to support, so people can move if needed.

and also implies a sequence that doesn't really exist -- e.g. z can't use x or y in its expression.

I didn't think of that. Not sure how to address it.

@nikomatsakis
Copy link
Member

Yeah, and I think defining the closures is actually necessary to support, so people can move if needed.

Mmm -- is there ever a time that they need to move (modulo compiler bugs)? I don't think there should be (and if there really was you can always force a move for individual variables).

The main reason that one needs to write move is to tell the compiler that the closure is going to be escaping the current function, but that certainly shouldn't be happening in this case!

@nikomatsakis
Copy link
Member

Usually these kinds of macros are called something like "parallel blocks" -- is the name join! the right one? I could imagine something like parallel! { ... } or in_parallel! { ... } as well.

@cuviper
Copy link
Member

cuviper commented Sep 22, 2017

is the name join! the right one?

At least in the case of join!(|| 1, || 2, || 3), I was considering it just a variadic form of fn join. Naming it wildly different would seem odd to me.

@nikomatsakis
Copy link
Member

@cuviper does it support that form, or do you have to use let?

@nikomatsakis
Copy link
Member

Ah, I see from the macro it supports both. Seems like the docs should mention that more prominently, or maybe I just missed it.

@nikomatsakis
Copy link
Member

Oh, never mind. I see it now. =)

@cuviper
Copy link
Member

cuviper commented Sep 23, 2017

OK, so I think we agree that the "variadic" join!(|| 1, || 2, || 3) makes sense.

Maybe the let form should use a different macro like parallel! { let... }? On gitter @nikomatsakis also suggested let a = do || or let a = join || instead of fork.

One other thing that occurred to me it looks a little weird that we're not creating a scope for those let names. i.e. it's not an expression like join! { let a = fork ...; let b = fork ...; a + b }. Maybe we should put let in the name instead? e.g. parallel_let! { a = ...; b = ...; };?

Finally, since #[macro_use] isn't namespaced, should we prefix them like rayon_join!? I do usually call the function as rayon::join(...), so this would still feel symmetric to me.

@nikomatsakis
Copy link
Member

@cuviper interesting point about moving let into the name.


// Necessary for the tests using macros that expand to `::rayon::whatever`.
#[cfg(test)]
mod rayon {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use $crate in the macro and remove this helper module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point -- the macro should use $crate anyway.

@nikomatsakis
Copy link
Member

So @cuviper and I revisited this today in our Rayon sync meeting.

Some thoughts:

As far as the bikeshed goes, I'm inclined to this form:

rayon::parallel_let! {
    a = ...;
    b = ...;
}

Alternatives:

  • par_let, to be more consistent with par_iter?
  • commas instead of ;?

Putting let a = ... inside of the {} really suggests to me that the name is scoped to the macro-invocation itself, whereas it in fact is scoped to the containing block.

The new macro system makes namespacing a non-issue. Yay!

As for the process, I'd be ok accepting this PR because of grandfathering. That said, I'd love to see this reframed as an RFC, laying out some of the motivation and getting a bit broader feedback. But I don't want to ask too much of poor @fitzgen. As @cuviper said, "Hey @fitzgen, remember that PR that stalled? please now use this bigger process for it..."...

@ghost
Copy link

ghost commented Nov 15, 2018

@nikomatsakis

* `par_let`, to be more consistent with `par_iter`?

Yes to that! I think par_let would be more appropriate.

* commas instead of `;`?

Possibly, but unsure. This might look strange with semicolons:

rayon::par_let!(a = 10, b = 20, c = 30);

Another thing to take into consideration when gauging the syntax is that there might be pattern matching and types thrown in:

rayon::par_let! {
    a: isize = foo(10),
    (a, b): (u32, String) = bar(20),
    c = baz(30),
}

Not sure how I feel about that. Looks messy and unlike any other Rust syntax I've seen.

The following looks much better, but suffers from the scoping confusion you mentioned:

rayon::parallel! {
    let a: isize = foo(10);
    let (a, b): (u32, String) = bar(20);
    let c = baz(30);
}

@cuviper
Copy link
Member

cuviper commented Nov 28, 2018

there might be pattern matching and types thrown in:

Currently macro_rules! doesn't allow this, though it probably should: playground

error: `$p:pat` is followed by `:`, which is not allowed for `pat` fragments

@nikomatsakis suggested we might be able to hack this by accepting either $i:ident : $t:ty or ($p:pat) : $t:ty, but I think then it gets complicated to match combinations of the two.

Also, I don't know if this was intentional, but your examples have a twice. I expect we'd expand these to a single binding: let (a, ((a, b), c)) = join(|| foo(10), join(|| bar(20), || baz(30))); -- which would then trigger E0416, "identifier a is bound more than once in the same pattern."

@cuviper
Copy link
Member

cuviper commented Nov 28, 2018

As a way forward, how would folks feel about just adding join! for now? That's pretty straightforward, whereas the design of parallel-let seems to need more discussion, probably worthy of an RFC.

@nikomatsakis
Copy link
Member

works for me

@cuviper
Copy link
Member

cuviper commented Nov 28, 2018

we might be able to hack this by accepting either $i:ident : $t:ty or ($p:pat) : $t:ty

I misunderstood, they meant $p:tt : $t:ty, which accepts identifiers or (...) just as well. It just means that the user has to be aware that parens are needed in cases like (Foo { .. }).

@rocallahan
Copy link
Contributor

Mmm -- is there ever a time that they need to move (modulo compiler bugs)?

Yes. The closure may want to take ownership of some object (e.g. to take it apart).

@nikomatsakis
Copy link
Member

I would note that there is no plenty of precedent from futures, too: https://docs.rs/futures/0.3.15/futures/macro.join.html

@cuviper
Copy link
Member

cuviper commented May 27, 2021

@rocallahan since you asked in #865, are you interested in restarting this?

@rocallahan
Copy link
Contributor

I just hacked around it in our code:

macro_rules! join_all {
    ($e1:expr, $e2:expr) => {
        rayon::join($e1, $e2)
    };
    ($e1:expr, $e2:expr, $e3:expr, $e4:expr) => {{
        let ((a, b), (c, d)) = rayon::join(|| join_all!($e1, $e2), || join_all!($e3, $e4));
        (a, b, c, d)
    }};
    ($e1:expr, $e2:expr, $e3:expr, $e4:expr, $e5:expr) => {{
        let ((a, b, c, d), e) = rayon::join(|| join_all!($e1, $e2, $e3, $e4), $e5);
        (a, b, c, d, e)
    }};
}

so I'm not going to sink more work into it at this time.

@QuarticCat
Copy link

Perhaps we can implement it as (a, b, c, ...).join() through a Join trait?

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.

join_slice()
8 participants