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

Use macros for tuple implementations of various types #2

Merged
merged 14 commits into from
Jul 22, 2024

Conversation

luca-della-vedova
Copy link
Member

@luca-della-vedova luca-della-vedova commented Jul 17, 2024

New feature implementation

Implemented feature

As noted in the description. Formatting is quite opinionated but I couldn't run cargo fmt since it seems the codebase is not formatted and it would have blown up the diff.

Implementation description

Bevy provides an all_tuples! macro that facilitates implementing macros for different sizes of tuples. This together with a macro implementation for the traits themselves makes it possible to reduce code duplication when implementing traits for tuples of different sizes.

This has been used for Buffered, Bufferable, ForkCloneBuilder, Unzippable, UnzipBuilder and StreamPack.

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
…red_macros

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
…_macros

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
…red_macros

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Comment on lines +593 to +604
(
(
$(
$T.0,
)*
),
(
$(
$T.1,
)*
)
)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a bit of a symbol soup that I didn't know how to format better, it takes a lot of lines but it would be pretty much unreadable if it was compressed in a single line

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
@luca-della-vedova luca-della-vedova changed the base branch from impulse_builder to main July 18, 2024 07:48
Comment on lines +51 to +55
// Variable is only used to make sure this cycle is repeated once
// for each instance of the $T type, but the type itself is not
// used.
#[allow(unused)]
let $T = std::marker::PhantomData::<$T>;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a "hack" I had to create to allow iterating over all the types passed as an input without actually using them

Comment on lines 78 to 80
// Targets is cloned to avoid borrow checker issues when
// doing a mutable borrow of the world later
let targets = world.get::<ForkTargetStorage>(source).or_broken()?.clone();
Copy link
Member Author

Choose a reason for hiding this comment

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

From my understanding previously the clone wasn't necessary because we would dereference the borrow to Entity which, because Entity implements Copy, would just copy it below the scenes.
Performance wise I believe cloning the SmallVec should be almost the same as copying all the elements one by one

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, the problem here is we can't guarantee that the preallocated SmallVec size is big enough since it's currently 8, and these tuples go up to 12. At that point doing a clone will cause an avoidable heap allocation.

If there's any way to use a pattern here to create N variables with entities or to create a tuple of N entities, copied out of the SmallVec, that would be preferable. Heap allocations once in a while aren't a huge deal if avoiding them is too onerous, but the more we can avoid them inside of hot loops, the more compelling this library is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair, I "found" a trick that by adding a dummy macro element (that shouldn't really cost anything since it's only a compile time cost) we can unpack all the vector elements into a tuple, which also fairly simplifies the code there dc95e40.
This comes at the cost of a dependency on itertools since it has a very convenient method to turn an iterator into a tuple.

I had to change the maximum tuple size down to 12 since that's the maximum that itertools next_tuple() implements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

I had to change the maximum tuple size down to 12 since that's the maximum that itertools next_tuple() implements.

This should be totally fine. Since it supports nested tuples of any depth, there's effectively no limit, but if people are exceeding tuples of 12 elements in one output then they're going pretty wild anyway.

@luca-della-vedova
Copy link
Member Author

luca-della-vedova commented Jul 18, 2024

I found a trick using PhantomData and some minor refactoring that removed the limitations, now all the types have macro implementations.

@luca-della-vedova luca-della-vedova changed the title Use macros for tuple implementations of Bufferable, Buffered and StreamPack Use macros for tuple implementations of various types Jul 18, 2024
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
}
// Implements the `ForkCloneBUilder` trait for all tuples between size 2 and 15
// (inclusive)
all_tuples!(impl_forkclonebuilder_for_tuple, 2, 15, F, U);
Copy link
Contributor

@mxgrey mxgrey Jul 22, 2024

Choose a reason for hiding this comment

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

Very minor nitpick, but let's cap the tuple size to 12 for all of the tuple implementations for the sake of consistency, so users don't get additionally confused by hitting different tuple limits for different functions. If someone wants more than 12 forked clones there are several ways they could still accomplish that with a limit of 12.

Copy link
Member Author

Choose a reason for hiding this comment

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

903663c also found that there was a tuple with size 1 implemented manually and removed that to make the diff even more red c97a7fc

Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai>
Copy link
Contributor

@mxgrey mxgrey left a comment

Choose a reason for hiding this comment

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

This is awesome!! Thanks for figuring out how to cook up the right symbol soup for all these traits 👍

@mxgrey mxgrey merged commit 9b60160 into open-rmf:main Jul 22, 2024
1 check passed
@luca-della-vedova luca-della-vedova deleted the luca/buffered_macros branch July 22, 2024 07:34
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.

2 participants