Double the capacity when BlobVec is full #11167
Conversation
mockersf
added
A-ECS
|
This solution won't fix most of the issues. In many cases, bevy calls Proper fix should be (basically, copy
|
|
Sure. I will leave |
| let extra_space = self.capacity.max(additional - available_space); | ||
| // SAFETY: `additional - available_space > 0` so `extra_space` is non-zero | ||
| let increment = unsafe { NonZeroUsize::new_unchecked(extra_space) }; |
There was a problem hiding this comment.
- Let's call it
extra_capacitybecause "space" can mean both "len" and "capacity" - Also let's name both variables
extra_spaceandincrementthe same, because they are the same - Also
new_uncheckedis not really needed here, safenew()+unwrapshould work equally fine- compiler is able to get rid of this trivial check
- but even if it doesn't, this code is executed rarely anyway
|
Overall, looks good. |
|
If we want to make this code more perfect, further change might be this: split
The idea is this. If we mark Splitting |
There was a problem hiding this comment.
The status quo seems to have been an artifact of #1525 when we forked from hecs. It seems like hecs does also use a doubling strategy as well, as of July 2021: Ralith/hecs@a8545a2, which was roughly 4-5 months after ECS V2. Checked this with @Ralith and @cart on Discord: https://discord.com/channels/691052431525675048/749335865876021248/1192580492164341820.
With the history here established, I think this is a good idea in general, but if you check the usage of BlobVec::push, it's only used in Column::push, which is only used in ComponentSparseSet::insert, Resource::insert, and their variants. This won't impact any of the table based storage, which is what almost all components use, but this should improve performance when mass inserting/spawning SparseSet components.
Code generally looks good, though there are some things I want addressed.
| /// Similar to `reserve_exact`. This method ensures that the capacity will grow at least `self.capacity()` if there is no | ||
| /// enough space to hold `additional` more elements. | ||
| #[cold] | ||
| fn do_reserve(&mut self, additional: usize) { |
There was a problem hiding this comment.
I'd follow the style the Rust project uses for their Vec implementation, and scope the cold function within the reserve function itself: https://github.com/rust-lang/rust/blob/6bc08a725f888a06ea3c6844f3d0cc2d2ebc5142/library/alloc/src/raw_vec.rs#L294.
| #[cold] | ||
| fn do_reserve(&mut self, additional: usize) { | ||
| let available_space = self.capacity - self.len; | ||
| if available_space < additional && self.item_layout.size() > 0 { |
There was a problem hiding this comment.
If we are already doing the checks in reserve, we don't need to be repeating them here. We can just assume we have already met the requisite conditions.
| if slf.item_layout.size() > 0 { | ||
| let increment = slf.capacity.max(additional - (slf.capacity - slf.len)); | ||
| let increment = NonZeroUsize::new(increment).unwrap(); | ||
| // SAFETY: not called for ZSTs | ||
| unsafe { slf.grow_exact(increment) }; | ||
| } |
There was a problem hiding this comment.
After #10799 merged, this code is actually meant to call grow_exact for ZST.
There was a problem hiding this comment.
Does it mean we no longer need that check and just call grow_exact immediately?
There was a problem hiding this comment.
Yes, we don't need check if slf.item_layout.size() > 0.
|
benchmarks are a bit noisy but we do win in the big sparse_set case one run was particularly harsh on these two benches: growing by 1.5x instead of 2x looks pretty much the same |
alice-i-cecile
added
the
S-Ready-For-Final-Review
Objective
Solution
BlobVecbefore pushing a new element.