Use macros for tuple implementations of various types #2
Conversation
Signed-off-by: Luca Della Vedova <[email protected]>
…red_macros Signed-off-by: Luca Della Vedova <[email protected]>
…_macros Signed-off-by: Luca Della Vedova <[email protected]>
…red_macros Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
| ( | ||
| ( | ||
| $( | ||
| $T.0, | ||
| )* | ||
| ), | ||
| ( | ||
| $( | ||
| $T.1, | ||
| )* | ||
| ) | ||
| ) |
There was a problem hiding this comment.
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 <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
luca-della-vedova
force-pushed
the
luca/buffered_macros
branch
from
a5776d1 to
af7b768
Compare
| // 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>; |
There was a problem hiding this comment.
This is a "hack" I had to create to allow iterating over all the types passed as an input without actually using them
src/chain/unzip.rs
Outdated
| // 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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I found a trick using |
luca-della-vedova
changed the title
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
src/chain/fork_clone_builder.rs
Outdated
| } | ||
| // Implements the `ForkCloneBUilder` trait for all tuples between size 2 and 15 | ||
| // (inclusive) | ||
| all_tuples!(impl_forkclonebuilder_for_tuple, 2, 15, F, U); |
There was a problem hiding this comment.
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.
Signed-off-by: Luca Della Vedova <[email protected]>
Signed-off-by: Luca Della Vedova <[email protected]>
New feature implementation
Implemented feature
As noted in the description. Formatting is quite opinionated but I couldn't run
cargo fmtsince 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,UnzipBuilderandStreamPack.