get_components_mut #21780
get_components_mut #21780
Conversation
alice-i-cecile
added
C-Feature
|
It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note. Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes. |
# Objective - As part of #21780, I need a way to iterate over the component ids of a bundle for `Entity*Except` conflict checking without allocating. Pulled this out as it changes some unrelated code too. ## Solution - Change `Bundle::component_ids` and `Bundle::get_component_ids` to return an iterator instead of taking a closure. In theory I would expect this to compile to the same asm. I would also argue that using an iterator is a more natural api for this than the closure. It probably took a closure before because expressing that the iterator doesn't capture the `&mut ComponentRegistrator` lifetime wasn't possible without the `use` syntax. - Removed some #[allow(deprecated)] in the Bundle macro that was missed. ## Testing - Checked the asm for `hook_on_add` in the observers example for to confirm it was still the same. This is a pretty simple example though, so not sure how good of a check this is. - None of the code touched are in any hot paths, but ran the spawn and insert benches. Any changes seem to be in the noise.
hymm
force-pushed
the
get_components_mut
branch
from
d0fda0c to
caf7606
Compare
# Objective - As part of bevyengine#21780, I need a way to iterate over the component ids of a bundle for `Entity*Except` conflict checking without allocating. Pulled this out as it changes some unrelated code too. ## Solution - Change `Bundle::component_ids` and `Bundle::get_component_ids` to return an iterator instead of taking a closure. In theory I would expect this to compile to the same asm. I would also argue that using an iterator is a more natural api for this than the closure. It probably took a closure before because expressing that the iterator doesn't capture the `&mut ComponentRegistrator` lifetime wasn't possible without the `use` syntax. - Removed some #[allow(deprecated)] in the Bundle macro that was missed. ## Testing - Checked the asm for `hook_on_add` in the observers example for to confirm it was still the same. This is a pretty simple example though, so not sure how good of a check this is. - None of the code touched are in any hot paths, but ran the spawn and insert benches. Any changes seem to be in the noise.
| /// Accesses [`Component`](crate::prelude::Component) data | ||
| Component(EcsAccessLevel), | ||
| /// Accesses [`Resource`](crate::prelude::Resource) data | ||
| Resource(ResourceAccessLevel), |
| #[derive(Clone, Copy, Debug, PartialEq)] | ||
| pub enum QueryAccessError { | ||
| /// Component was not registered on world | ||
| ComponentNotRegistered, |
There was a problem hiding this comment.
Note that since adding a component to an entity registers it, ComponentNotRegistered is just a special case of EntityDoesNotMatch. I don't think a caller can ever really do anything differently in those cases, so I'd be inclined to remove the ComponentNotRegistered variant and just return EntityDoesNotMatch in that case.
There was a problem hiding this comment.
You can technically call this method before the component has been registered. I hit this while I was writing my tests. Is the average user likely to hit this? probably not. But the way you would fix this is by registering the component which is not what you would do for EntityDoesNotMatch.
There was a problem hiding this comment.
You can technically call this method before the component has been registered. I hit this while I was writing my tests. Is the average user likely to hit this? probably not. But the way you would fix this is by registering the component which is not what you would do for
EntityDoesNotMatch.
But if the component hasn't been registered, then it hasn't been inserted onto any entity, which means it hasn't been inserted onto the current entity. So registering it will just change the error to EntityDoesNotMatch. And inserting the component onto that entity will fix either error.
... oh, except for Option<D> or Has<C>, which might fail instead of returning None or false. Blah.
I didn't do exactly that, but I did try the algorithm in this pr with fixedbitsets and it was significantly slower at 2 and 5 components. iirc it was around 30us for both. Around the same at 10 components and faster at 16. |
# Objective - As part of bevyengine#21780, I need a way to iterate over the component ids of a bundle for `Entity*Except` conflict checking without allocating. Pulled this out as it changes some unrelated code too. ## Solution - Change `Bundle::component_ids` and `Bundle::get_component_ids` to return an iterator instead of taking a closure. In theory I would expect this to compile to the same asm. I would also argue that using an iterator is a more natural api for this than the closure. It probably took a closure before because expressing that the iterator doesn't capture the `&mut ComponentRegistrator` lifetime wasn't possible without the `use` syntax. - Removed some #[allow(deprecated)] in the Bundle macro that was missed. ## Testing - Checked the asm for `hook_on_add` in the observers example for to confirm it was still the same. This is a pretty simple example though, so not sure how good of a check this is. - None of the code touched are in any hot paths, but ran the spawn and insert benches. Any changes seem to be in the noise.
# Objective - As part of bevyengine#21780, I need a way to iterate over the component ids of a bundle for `Entity*Except` conflict checking without allocating. Pulled this out as it changes some unrelated code too. ## Solution - Change `Bundle::component_ids` and `Bundle::get_component_ids` to return an iterator instead of taking a closure. In theory I would expect this to compile to the same asm. I would also argue that using an iterator is a more natural api for this than the closure. It probably took a closure before because expressing that the iterator doesn't capture the `&mut ComponentRegistrator` lifetime wasn't possible without the `use` syntax. - Removed some #[allow(deprecated)] in the Bundle macro that was missed. ## Testing - Checked the asm for `hook_on_add` in the observers example for to confirm it was still the same. This is a pretty simple example though, so not sure how good of a check this is. - None of the code touched are in any hot paths, but ran the spawn and insert benches. Any changes seem to be in the noise.
Good call: we should prioritize the performance of the 2-5 component use case. |
|
@hymm, this looks pretty much ready for review :) Want to get this passing CI and then get a full review from me? |
needed it when I was examining the assembly
| } | ||
| } | ||
| } else { | ||
| // we can optimize small sizes by caching the iteration result in an array on the stack |
There was a problem hiding this comment.
Does this actually help? For the really small sizes, I would have expected the compiler to unroll the whole thing anyway.
There was a problem hiding this comment.
yeah, it's like 50% faster at 10 components. I think it's mostly the calls to Components::component_id.
There was a problem hiding this comment.
yeah, it's like 50% faster at 10 components. The call to
Components::component_idis surprisingly expensive.
Yeah, but the component_id call is now done only once in get_state, right? So the ComponentIds are already cached on the stack, and this is just caching the trivial conversions to EcsAccessType.
Anyway, if it's 50% faster then we should definitely keep it!
There was a problem hiding this comment.
looked at the assembly a little. Best I can tell is that it's inlining way more stuff. I see 10 is_compatible checks vs just 3. I see some other changes, but don't fully understand them to know if they're helping the speed.
3 participants
Objective
EntityMut::get_components_mutandEntityWorldMut::get_components_mutthat does not allocateSolution
QueryData. This is then used to iterate over the pairs of access to check if they are compatible or not.Testing
Bench checked vs unchecked (50000 entities)
so at 10 components each call was taking about 0.22us vs 0.03 us
ToDo