Switch to using smallvec over tinyvec #100
Conversation
|
See the reasons why tinyvec was adopted: #75 . It's 100% safe. But yeah 52 dependents vs 759 dependents... |
|
I'm not too familiar with the operations being done with the type, but it does seem like it's collecting some kind of stream of bools into a temporary buffer of some kind. Is the number of elements here bounded? If so, I could switch this to use a on-stack bitset instead, eliminating the dependency entirely. Otherwise, I'm still going to insist on using smallvec here. I'm trying to minimize the dependency tree of Bevy, which depends transitively on this crate through rodio. This is the only instance of tinyvec in the entire dependency tree, while there are at least 6 uses of smallvec elsewhere. |
|
Looking again at this, the size of these TinyVecs scale with the number of channels being decoded, which can only go up to 255 due to the limits on u8, and since it's storing bools, using on-stack bitset would only be 32 bytes to cover that entire range. We could skip the branches and the allocations without any unsafe. |
| pub mapping_magnitudes :Vec<u8>, | ||
| pub mapping_angles :Vec<u8>, | ||
| pub mapping_mux :Vec<u8>, | ||
| pub mapping_mux : [u8; 256], |
There was a problem hiding this comment.
why is this change needed? it doesn't seem useful to me.
| let mut no_residue = TinyVec::<[bool; 32]>::new(); | ||
| for fl in &decoded_floor_infos { | ||
| no_residue.push(fl.is_unused()); | ||
| let mut no_residue = [false; 256]; |
There was a problem hiding this comment.
Have you tested the performance of this? This function isn't particularly hot but we do allocate way more stack space now than before.
2 participants
smallvecseems to be in much higher use thantinyvec, so addingtinyvectends to increase the amount of work while usinglewtonas a dependency in a typical build. This PR replacestinyvecwithsmallvecas a drop-in replacement.