Remove `MaybeDone` from `tuple::join` #74

matheus-consoli · 2022-11-14T19:55:55Z

Remove the need for MaybeDone on tuple::join by creating for each future three fields: the Future itself; its result; its state.

I had to add a dependency on paste to create the $F_fields.

Bench:

>> critcmp main patch -f tuple::join
group             main                                   patch
-----             ----                                   -----
tuple::join 10    1.00    232.6±9.13ns        ? ?/sec    1.14    266.0±3.09ns        ? ?/sec

yoshuawuyts

This looks great, thank you so much!

yoshuawuyts

Actually, on second thought: I just noticed the perf regression and I think it might be because we're doing extra copies at the end. Meaning we've added extra machinery, but don't quite reap the benefits we hoped we would.

I've added some in-line comments explaining what's going on. Do you think you could update it to work that way?

yoshuawuyts · 2022-11-14T23:10:59Z

src/future/join/tuple.rs

+                $(
+                    #[pin] $F: $F,
+                    [<$F _out>]: MaybeUninit<$F::Output>,
+                    [<$F _state>]: PollState,
+                )*


We may want to create named fields here instead. I'm not sure about the exact syntax, but I'm thinking something along these lines?

Suggested change

$(

#[pin] $F: $F,

[<$F _out>]: MaybeUninit<$F::Output>,

[<$F _state>]: PollState,

)*

futures: ($(#[pin]$F,)*),

output: ($(MaybeUninit<$F::Output>,)*),

state: ($(PollState,)*),

yoshuawuyts · 2022-11-14T23:16:57Z

src/future/join/tuple.rs

+                    paste! {
+                        Poll::Ready(($( unsafe { this.[<$F _out>].assume_init_read() }),*))
+                    }


The regression we're seeing is probably because of this. The main performance benefit of #22 is that we prevent an extra copy at the end. Say we have a tuple (A, B), the output would always be (A::Output, B::Output) unless the future is cancelled.

Because we know this we can then store a field in-line of: (MaybeUninit<A::Output>, MaybeUninit<B::Output>), which once we know all fields have been initialized we can just return from the future as-is. This patch unfortunately doesn't quite do that; in order to mark the fields as "initialized" here, it does a move.

That's why I'm suggesting we instead find a way to store futures, output, and state as separate fields of tuples - and find a way to index into them.

yoshuawuyts · 2022-11-14T23:20:47Z

src/future/join/tuple.rs

+                                *this.[<$F _state>] = PollState::Done;
+                            }
+                        }
+                        all_done &= this.[<$F _state>].is_done();


Oof haha, I forgot we had this. We should probably initialize a counter set to len: utils::tuple_len!() inside the struct, and then with each completed future count down. That will also set us up to enable #21 to work, since we'll want to stop calling all wakers on each iteration anyway. But that doesn't have to be inside this patch.

matheus-consoli · 2022-11-16T00:22:00Z

I've reworked this a bit:

no paste dep
no tuple copy from (MaybeUninit<A>,..) to (A,)

but it got somewhat worst hahaha

I'll try to figure out a better way to impl this later, but let me know if you have any ideas :)

>> critcmp main patch -f tuple::join
group             main                                   patch
-----             ----                                   -----
tuple::join 10    1.00    231.6±0.73ns        ? ?/sec    1.19    275.7±1.31ns        ? ?/sec

for reference, the macro is desugaring `Join3` as follows:

#[pin_project]
#[must_use = "futures do nothing unless you `.await` or poll them"]
#[allow(non_snake_case)]
pub struct Join3<A: Future, B: Future, C: Future> {
    len: u32,
    #[pin] A: A,
    #[pin] B: B,
    #[pin] C: C,
    outputs: (
        MaybeUninit<A::Output>,
        MaybeUninit<B::Output>,
        MaybeUninit<C::Output>,
    ),
    states: PollStates,
}

impl<A: Future, B: Future, C: Future> Future for Join3<A, B, C> {
    type Output = (A::Output, B::Output, C::Output);
    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
        let mut this = self.project();
        if this.states[0].is_pending() {
            if let Poll::Ready(out) = this.A.poll(cx) {
                this.outputs.0 = MaybeUninit::new(out);
                this.states[0] = PollState::Done;
                *this.len -= 1;
            }
        };
        if this.states[1].is_pending() {
            if let Poll::Ready(out) = this.B.poll(cx) {
                this.outputs.1 = MaybeUninit::new(out);
                this.states[1] = PollState::Done;
                *this.len -= 1;
            }
        };
        if this.states[2].is_pending() {
            if let Poll::Ready(out) = this.C.poll(cx) {
                this.outputs.2 = MaybeUninit::new(out);
                this.states[2] = PollState::Done;
                *this.len -= 1;
            }
        };
        if *this.len <= 0 {
            let out = unsafe {
                (this.outputs as *const _ as *const (A::Output, B::Output, C::Output)).read()
            };
            Poll::Ready(out)
        } else {
            Poll::Pending
        }
    }
}

src/future/join/tuple.rs

yoshuawuyts · 2022-11-16T17:45:21Z

src/future/join/tuple.rs

-            done: bool,
-            $(#[pin] $F: MaybeDone<$F>,)*
+            len: u32,
+            $(#[pin] $F: $F,)*


Would it be possible to instead do:

Suggested change

$(#[pin] $F: $F,)*

tuple: ($(#[pin] $F: $F,)*),

That way we can move the tuple fields into here basically in-place.

I'm currently trying to figure out how to do this!

tuple: ($(#[pin] $F: $F,)*) doesn't work because pin_project doesn't support pinning this way, like (#[pin] T, #[pin] S)

play

@yoshuawuyts, I got something!

It's on a second branch, here.

I took some inspiration from #84 to use the pinned futures inside a tuple(struct), and it kinda works.

We have some performance gain, but it's kinda negligible 😞 (especially thinking about the macro-heavy impl orientation)

>> critcmp main patch -f tuple::join group main patch ----- ---- ----- tuple::join 10 1.03 239.3±1.06ns ? ?/sec 1.00 231.3±0.79ns ? ?/sec

Oh yeah, I like the intermediate struct approach!

The reason why there's a small perf cost might be because you're using ptr::read there which ends up doing an extra copy? Instead I think if it's at all possible it might be worth attempting to perform a direct transmute? This might require adding a #[repr(transparent)] to the Futures struct so that the layout is guaranteed to be the same as the tuples it stores?

oh, I don't think #[repr(transparent)] Futures is possible, transparent only allows one field to have a non-zero size, the Futures have many.

oh, I don't think #[repr(transparent)] Futures is possible, transparent only allows one field to have a non-zero size, the Futures have many.

oof, TIL :')

oook, it seems to me that we cannot transmute (MaybeUninit<T>, MaybeUninit<U>) to (T, U).

I'm not sure why, but I guess it's the same underlying problem from rust-lang/rust#61956, so casting the tuples appears to be the best option so far

Transmuting is not what you want here I think ?

let t = (MaybeUninit::<usize>::new(42), MaybeUninit::<u8>::new(12)); // source, can come from anywhere /* ... */ // Where you wanted to transmute: let (a, b) = t; (a.assume_init(), b.assume_init())

This can be macro-ified using $F as the variable name: it won't even shadow the type names because that's two different namespaces (let usize: usize = 4_usize; is valid)

Assuming the t is owned, this should not do any copy anywhere

ohh, thank you for pointing it out! I'll try that

yoshuawuyts

I've just approved #96. It seems this PR has merge conflicts anyway; if those can be resolved I think this should be good to go as well.

Either as part of this PR, or as part of a follow-up PR we should switch this implementation over to using tuple_for_each! as well, so the underlying implementation can be shared between all tuples.

poliorcetics · 2022-11-19T17:39:05Z

src/future/join/tuple.rs

+                    len: LEN,
+                    $($F: $F.into_future(),)*
+                    outputs: ($(MaybeUninit::<$F::Output>::uninit(),)*),
+                    states: PollStates::new(LEN as usize),


PollArray is usable here

matheus-consoli · 2022-11-20T03:11:13Z

I'm closing this PR in favor of #103

I opted to start from a clean new branch instead of dealing with this branch conflicts 😅

Thank you all!

yoshuawuyts approved these changes Nov 14, 2022

View reviewed changes

yoshuawuyts mentioned this pull request Nov 14, 2022

Replace MaybeDone/Fuse with separate input/output/state fields #22

Open

13 tasks

yoshuawuyts requested changes Nov 14, 2022

View reviewed changes

Remove MaybeDone from tuple::join

be1d966

poliorcetics mentioned this pull request Nov 15, 2022

feat: Remove MaybeDone for tuple::join (without additional deps) #84

Closed

Rework tuple::join impl

32a5233

matheus-consoli force-pushed the remove-maybedone-from-join-tuple branch from 5bd8446 to 32a5233 Compare November 16, 2022 00:01

yoshuawuyts reviewed Nov 16, 2022

View reviewed changes

yoshuawuyts mentioned this pull request Nov 17, 2022

Pin-projecting in-line tuples taiki-e/pin-project#349

Open

matheus-consoli mentioned this pull request Nov 18, 2022

Implement "perfect" waking for tuple::merge #96

Merged

yoshuawuyts approved these changes Nov 18, 2022

View reviewed changes

poliorcetics reviewed Nov 19, 2022

View reviewed changes

matheus-consoli mentioned this pull request Nov 20, 2022

Remove MaybeDone from tuple::join #103

Merged

matheus-consoli closed this Nov 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `MaybeDone` from `tuple::join` #74

Remove `MaybeDone` from `tuple::join` #74

matheus-consoli commented Nov 14, 2022 •

edited

Loading

yoshuawuyts left a comment

yoshuawuyts left a comment •

edited

Loading

yoshuawuyts Nov 14, 2022 •

edited

Loading

yoshuawuyts Nov 14, 2022

yoshuawuyts Nov 14, 2022

matheus-consoli commented Nov 16, 2022

yoshuawuyts Nov 16, 2022

matheus-consoli Nov 16, 2022 •

edited

Loading

matheus-consoli Nov 17, 2022 •

edited

Loading

yoshuawuyts Nov 17, 2022

matheus-consoli Nov 17, 2022

yoshuawuyts Nov 17, 2022

matheus-consoli Nov 17, 2022

poliorcetics Nov 18, 2022

poliorcetics Nov 18, 2022

matheus-consoli Nov 18, 2022

yoshuawuyts left a comment

poliorcetics Nov 19, 2022

matheus-consoli commented Nov 20, 2022

Remove MaybeDone from tuple::join #74

Remove MaybeDone from tuple::join #74

Conversation

matheus-consoli commented Nov 14, 2022 • edited Loading

yoshuawuyts left a comment

Choose a reason for hiding this comment

yoshuawuyts left a comment • edited Loading

Choose a reason for hiding this comment

yoshuawuyts Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matheus-consoli commented Nov 16, 2022

Choose a reason for hiding this comment

matheus-consoli Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

matheus-consoli Nov 17, 2022 • edited Loading

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

yoshuawuyts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matheus-consoli commented Nov 20, 2022

Remove `MaybeDone` from `tuple::join` #74

Remove `MaybeDone` from `tuple::join` #74

matheus-consoli commented Nov 14, 2022 •

edited

Loading

yoshuawuyts left a comment •

edited

Loading

yoshuawuyts Nov 14, 2022 •

edited

Loading

matheus-consoli Nov 16, 2022 •

edited

Loading

matheus-consoli Nov 17, 2022 •

edited

Loading