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.

Sign up for GitHub

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

Remove MaybeDone from tuple::join #74

Closed
wants to merge 2 commits into from
Closed

Remove MaybeDone from tuple::join #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be1d966
Remove `MaybeDone` from `tuple::join`
matheus-consoli Nov 14, 2022
32a5233
Rework tuple::join impl
matheus-consoli Nov 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
133 changes: 104 additions & 29 deletions src/future/join/tuple.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,85 @@
use super::Join as JoinTrait;
use crate::utils::MaybeDone;
use crate::utils::{self, PollState, PollStates};

use core::fmt::{self, Debug};
use core::future::{Future, IntoFuture};
use core::mem::MaybeUninit;
use core::pin::Pin;
use core::task::{Context, Poll};

use pin_project::pin_project;

macro_rules! impl_merge_tuple {
macro_rules! maybe_poll {
($idx:tt, $len:ident, $this:ident, $fut:ident, $cx:ident) => {
if $this.states[$idx].is_pending() {
if let Poll::Ready(out) = $this.$fut.poll($cx) {
$this.outputs.$idx = MaybeUninit::new(out);
$this.states[$idx] = PollState::Done;
*$this.len -= 1;
}
}
};
}

macro_rules! poll_all_pending {
(@inner 0, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(0, $len, $this, $fut, $cx);
poll_all_pending!(@inner 1, $len, $this, $cx, ($($rest,)*));
};
(@inner 1, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(1, $len, $this, $fut, $cx);
poll_all_pending!(@inner 2, $len, $this, $cx, ($($rest,)*));
};
(@inner 2, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(2, $len, $this, $fut, $cx);
poll_all_pending!(@inner 3, $len, $this, $cx, ($($rest,)*));
};
(@inner 3, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(3, $len, $this, $fut, $cx);
poll_all_pending!(@inner 4, $len, $this, $cx, ($($rest,)*));
};
(@inner 4, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(4, $len, $this, $fut, $cx);
poll_all_pending!(@inner 5, $len, $this, $cx, ($($rest,)*));
};
(@inner 5, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(5, $len, $this, $fut, $cx);
poll_all_pending!(@inner 6, $len, $this, $cx, ($($rest,)*));
};
(@inner 6, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(6, $len, $this, $fut, $cx);
poll_all_pending!(@inner 7, $len, $this, $cx, ($($rest,)*));
};
(@inner 7, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(7, $len, $this, $fut, $cx);
poll_all_pending!(@inner 8, $len, $this, $cx, ($($rest,)*));
};
(@inner 8, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(8, $len, $this, $fut, $cx);
poll_all_pending!(@inner 9, $len, $this, $cx, ($($rest,)*));
};
(@inner 9, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(9, $len, $this, $fut, $cx);
poll_all_pending!(@inner 10, $len, $this, $cx, ($($rest,)*));
};
(@inner 10, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(10, $len, $this, $fut, $cx);
poll_all_pending!(@inner 11, $len, $this, $cx, ($($rest,)*));
};
(@inner 11, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(11, $len, $this, $fut, $cx);
poll_all_pending!(@inner 12, $len, $this, $cx, ($($rest,)*));
};
(@inner 12, $len:ident, $this:ident, $cx:ident, ($fut:ident, $($rest:ident,)*)) => {
maybe_poll!(12, $len, $this, $fut, $cx);
};
(@inner $ignore:literal, $len:ident, $this:ident, $cx:ident, ()) => { };
($len:ident, $this:ident, $cx:ident, $($F:ident,)*) => {
poll_all_pending!(@inner 0, $len, $this, $cx, ($($F,)*));
};
}
yoshuawuyts marked this conversation as resolved.
Show resolved Hide resolved

macro_rules! impl_join_tuple {
($StructName:ident $($F:ident)*) => {
/// Waits for two similarly-typed futures to complete.
///
Expand All @@ -21,8 +92,10 @@ macro_rules! impl_merge_tuple {
#[must_use = "futures do nothing unless you `.await` or poll them"]
#[allow(non_snake_case)]
pub struct $StructName<$($F: Future),*> {
done: bool,
$(#[pin] $F: MaybeDone<$F>,)*
len: u32,
$(#[pin] $F: $F,)*
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@matheus-consoli matheus-consoli Nov 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@matheus-consoli matheus-consoli Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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 :')

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

outputs: ($(MaybeUninit<$F::Output>,)*),
states: PollStates,
}

impl<$($F),*> Debug for $StructName<$($F),*>
Expand All @@ -32,7 +105,8 @@ macro_rules! impl_merge_tuple {
)* {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_tuple("Join")
$(.field(&self.$F))*
.field(&($(&self.$F,)*))
.field(&self.states)
.finish()
}
}
Expand All @@ -41,20 +115,18 @@ macro_rules! impl_merge_tuple {
#[allow(unused_parens)]
#[allow(unused_variables)]
impl<$($F: Future),*> Future for $StructName<$($F),*> {
type Output = ($($F::Output),*);
type Output = ($($F::Output,)*);

fn poll(
self: Pin<&mut Self>, cx: &mut Context<'_>
) -> Poll<Self::Output> {
let mut all_done = true;
let mut this = self.project();
assert!(!*this.done, "Futures must not be polled after completing");

$(all_done &= this.$F.as_mut().poll(cx).is_ready();)*
poll_all_pending!(LEN, this, cx, $($F,)*);

if all_done {
*this.done = true;
Poll::Ready(($(this.$F.take().unwrap()),*))
if *this.len <= 0 {
let out = unsafe {(this.outputs as *const _ as *const ($($F::Output,)*)).read()};
Poll::Ready(out)
} else {
Poll::Pending
}
Expand All @@ -66,33 +138,36 @@ macro_rules! impl_merge_tuple {
where $(
$F: IntoFuture,
)* {
type Output = ($($F::Output),*);
type Output = ($($F::Output,)*);
type Future = $StructName<$($F::IntoFuture),*>;

fn join(self) -> Self::Future {
let ($($F,)*): ($($F,)*) = self;
const LEN: u32 = utils::tuple_len!($($F,)*);
$StructName {
done: false,
$($F: MaybeDone::new($F.into_future())),*
len: LEN,
$($F: $F.into_future(),)*
outputs: ($(MaybeUninit::<$F::Output>::uninit(),)*),
states: PollStates::new(LEN as usize),
Copy link
Contributor

Choose a reason for hiding this comment

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

PollArray is usable here

}
}
}
};
}

impl_merge_tuple! { Join0 }
impl_merge_tuple! { Join1 A }
impl_merge_tuple! { Join2 A B }
impl_merge_tuple! { Join3 A B C }
impl_merge_tuple! { Join4 A B C D }
impl_merge_tuple! { Join5 A B C D E }
impl_merge_tuple! { Join6 A B C D E F }
impl_merge_tuple! { Join7 A B C D E F G }
impl_merge_tuple! { Join8 A B C D E F G H }
impl_merge_tuple! { Join9 A B C D E F G H I }
impl_merge_tuple! { Join10 A B C D E F G H I J }
impl_merge_tuple! { Join11 A B C D E F G H I J K }
impl_merge_tuple! { Join12 A B C D E F G H I J K L }
impl_join_tuple! { Join0 }
impl_join_tuple! { Join1 A }
impl_join_tuple! { Join2 A B }
impl_join_tuple! { Join3 A B C }
impl_join_tuple! { Join4 A B C D }
impl_join_tuple! { Join5 A B C D E }
impl_join_tuple! { Join6 A B C D E F }
impl_join_tuple! { Join7 A B C D E F G }
impl_join_tuple! { Join8 A B C D E F G H }
impl_join_tuple! { Join9 A B C D E F G H I }
impl_join_tuple! { Join10 A B C D E F G H I J }
impl_join_tuple! { Join11 A B C D E F G H I J K }
impl_join_tuple! { Join12 A B C D E F G H I J K L }

#[cfg(test)]
mod test {
Expand All @@ -110,7 +185,7 @@ mod test {
fn join_1() {
futures_lite::future::block_on(async {
let a = future::ready("hello");
assert_eq!((a,).join().await, ("hello"));
assert_eq!((a,).join().await, ("hello",));
});
}

Expand Down
13 changes: 13 additions & 0 deletions src/utils/poll_state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,19 @@ pub(crate) enum PollStates {
Boxed(Box<[PollState]>),
}

impl core::fmt::Debug for PollStates {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
Self::Inline(len, states) => f
// .debug_tuple("Inline")
.debug_list()
.entries(&states[..(*len as usize)])
.finish(),
Self::Boxed(states) => f.debug_list().entries(&**states).finish(),
}
}
}

impl PollStates {
pub(crate) fn new(len: usize) -> Self {
assert!(MAX_INLINE_ENTRIES <= u8::MAX as usize);
Expand Down
4 changes: 4 additions & 0 deletions src/utils/tuple.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/// Compute the number of permutations for a number
/// during compilation.
pub(crate) const fn permutations(mut num: u32) -> u32 {
if num == 0 {
return 0;
}

let mut total = 1;
loop {
total *= num;
Expand Down