Fix fishy spawn batch implementation#23344
Conversation
ElliottjPierce
added
C-Bug
chescock
left a comment
chescock
left a comment
There was a problem hiding this comment.
Good catch!
Should we really be allocating at the high iterator bound? It's probably helpful in most cases, but it could brick the ecs and panic if the high bound is more than u32::MAX or something, and if it is significantly higher than the truth, it would cause real slow downs allocating and then freeing all those entities.
Wait, what do we do when the iterator is actually unbounded? ... Huh, it uses upper.unwrap_or(lower);. Ah, and it falls back to doing spawn instead of spawn_at if it runs out of preallocated entities.
Given that we allocate at the lower bound sometimes, that might be an argument for allocating at the lower bound always.
And I suppose another option is to require ExactSizeIterator.
But the change you have here seems like the right first step regardless, since it only changes the behavior in places where we were leaking before!
Yeah, I agree. I think we either always use the lower bound or at least cap the upper bound to 2x the lower or something. |
I don't think it's common to have an upper bound as well as a non-zero lower bound? I guess it sometimes happens when |
It also happens when doing any kind of filtering, which is probably more common. |
|
Filters have to use 0 as the lower bound. |
Ah, right. I misread. Thanks. |
kfc35
added
S-Ready-For-Final-Review
# Objective Current spawn batch implementation leaks entity allocations. For iterators that give a fixed upper bound that is greater than the lower bound, the current implementation over-allocates entities and never frees them. I checked and this has been a bug for a very long time, so impact is probably low. ## Solution Free the over allocated entities. We could also use the lower bound of the iterator, but that is not enough since the iterator doesn't always meet its lower bound. ## Testing I added a test that works here but fails on main. ## Other things to look at Should we really be allocating at the high iterator bound? It's probably helpful in most cases, but it could brick the ecs and panic if the high bound is more than u32::MAX or something, and if it is significantly higher than the truth, it would cause real slow downs allocating and then freeing all those entities. Should we restructure this to work without needing a `Drop` implementation. That would let us implement `fold` and such, which could improve performance where the iterator is the bottleneck. This would either come with a breaking change (like needing to call `spawn_rest` instead of just letting the spawn iterator drop) or a lot of unsafe code that would need to properly handle unwinds etc. Probably not too bad, but more complexity to maintain.
6 participants
Objective
Current spawn batch implementation leaks entity allocations.
For iterators that give a fixed upper bound that is greater than the lower bound,
the current implementation over-allocates entities and never frees them.
I checked and this has been a bug for a very long time, so impact is probably low.
Solution
Free the over allocated entities.
We could also use the lower bound of the iterator, but that is not enough since the iterator doesn't always meet its lower bound.
Testing
I added a test that works here but fails on main.
Other things to look at
Should we really be allocating at the high iterator bound? It's probably helpful in most cases, but it could brick the ecs and panic if the high bound is more than u32::MAX or something, and if it is significantly higher than the truth, it would cause real slow downs allocating and then freeing all those entities.
Should we restructure this to work without needing a
Dropimplementation. That would let us implementfoldand such, which could improve performance where the iterator is the bottleneck. This would either come with a breaking change (like needing to callspawn_restinstead of just letting the spawn iterator drop) or a lot of unsafe code that would need to properly handle unwinds etc. Probably not too bad, but more complexity to maintain.