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

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

Merged

Conversation

matheus-consoli
Copy link
Collaborator

This mimics the implementation for array::merge.

I used that hack I introduced on #74 in a more generalized form, we can rewrite the gen_conditions in terms of this new macro (basically the same way I do here)

Ref #21

@matheus-consoli
Copy link
Collaborator Author

oh, I almost forgot the perf result

>> critcmp main patch -f tuple::merge
group              main                                   patch
-----              ----                                   -----
tuple::merge 10    1.03    763.3±1.40ns        ? ?/sec    1.00    738.1±2.47ns        ? ?/sec

@yoshuawuyts yoshuawuyts changed the title Implement "perfect" wakining for tuple::merge Implement "perfect" waking for tuple::merge Nov 18, 2022
Copy link
Owner

@yoshuawuyts yoshuawuyts left a comment

Choose a reason for hiding this comment

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

This looks great! -- I'm adding you as a reviewer, so feel free to merge this PR yoursel!

Comment on lines +144 to +145
// poll the `streams.{index}` stream
utils::tuple_for_each!(poll_stream (index, this, streams, cx, LEN) $($F)*);
Copy link
Owner

Choose a reason for hiding this comment

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

Oh this is really nice!

impl_merge_tuple! { merge9 Merge9 A B C D E F G H I }
impl_merge_tuple! { merge10 Merge10 A B C D E F G H I J }
impl_merge_tuple! { merge11 Merge11 A B C D E F G H I J K }
impl_merge_tuple! { merge12 Merge12 A B C D E F G H I J K L }
Copy link
Contributor

Choose a reason for hiding this comment

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

A little nit: macro-highlighting is basically impossible to do well in Rust, but to simplify it for treesitter (the library used by github and lots of editors), we could make the input to this and other macros something like this:

$mod_name: ident :: $StructName: ident <$($F: ident),+>

That's not a blocker at all though, I just like the idea of using common Rust syntax so that macros are both easier to read and highlights.

Of course the issue here would be that MergeX is not really in the mergeX module, just wanted to get the idea out here :)

@poliorcetics
Copy link
Contributor

poliorcetics commented Nov 18, 2022

I have another idea to improve getting the index from a tuple value, let me try it, I think it will fail statically (e.g. tuple.0 but will translate really well dynamically (matching tuple.0 to array[0])

@poliorcetics
Copy link
Contributor

poliorcetics commented Nov 18, 2022

The main idea is using repr(u*) to ensure the tuple indexes are always correct, not letting the user do the mistake of specifying the same twice for example.

macro_rules! tuple_indexes {
    ($mod_name:ident :: $StructName:ident < $($F:ident),+ >) => {
        mod $mod_name {
            // Ensure indexes are in increasing order and start at 0 while being
            // very cheap to generate for the macro.
            //
            // If you want to ensure there are 256 elements or less, use repr(u8)
            #[repr(usize)]
            pub enum Indexes {
                $($F),+
            }

            pub const TUPLE_LEN: usize = match [$(Indexes::$F),+] {
                [.., last] => last as usize + 1,
            };
        }

        #[derive(Default)]
        pub struct $StructName {
            state: [bool; $mod_name::TUPLE_LEN],
        }

        impl $StructName {
            $(
                #[allow(non_snake_case)]
                pub fn $F(&self) -> (usize, bool) {
                    // For this simple example could be inlined, but if used multiple times,
                    // having a separate branch for clarity is nice
                    let index = tuple_indexes!(@index of $mod_name :: $F);
                    (index, self.state[index])
                }
            )+
        }
    };
    (@index of $mod_name: ident :: $F: ident) => {
        $mod_name::Indexes::$F as usize
    };
}

tuple_indexes!(test_1 :: Test1 < A >);
tuple_indexes!(test_2 :: Test2 < A, B >);
tuple_indexes!(test_3 :: Test3 < A, B, C >);

#[test]
fn expected_indexes() {
    let t = Test1::default();
    assert_eq!(t.A().0, 0);

    let t = Test2::default();
    assert_eq!(t.A().0, 0);
    assert_eq!(t.B().0, 1);

    let t = Test3::default();
    assert_eq!(t.A().0, 0);
    assert_eq!(t.B().0, 1);
    assert_eq!(t.C().0, 2);
}

This could be used to simplify the macros, making rustc do less work ^^

@matheus-consoli
Copy link
Collaborator Author

that's a very interesting approach, @poliorcetics!

I'll merge this PR as it is now, but I would like to see a scratch PR integrating your ideas - I think it would be very fair to see you in the contributors list!

@matheus-consoli matheus-consoli merged commit 00de15d into yoshuawuyts:main Nov 18, 2022
@matheus-consoli matheus-consoli deleted the perfect-waker-tuple-merge branch November 18, 2022 23:45
@poliorcetics
Copy link
Contributor

I'll merge this PR as it is now, but I would like to see a scratch PR integrating your ideas - I think it would be very fair to see you in the contributors list!

Done in #99 :) I'll try to run the benchmarks on my side to see if it helped

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.

3 participants