Conversation
|
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.
| } | ||
| } | ||
| } 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.
# 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.
|
Updating the branch to see if miri behaves :) It would be nice to land this in 0.18: it seems very close. |
alice-i-cecile
left a comment
There was a problem hiding this comment.
I think this should exist, and I like the implementation strategy.
I think that there's a fair bit of context needed in the release notes to explain why this is cool (it enables OOP-y patterns), but I'll add that later during editing :)
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