Add earlier early-outs to bundle and component registration - #25295
Add earlier early-outs to bundle and component registration#25295JaySpruce wants to merge 5 commits into
Conversation
JaySpruce
added
A-ECS
| // Use `register_component_checked` if that's a concern. | ||
| #[inline] | ||
| pub fn register_component<T: Component>(&mut self) -> ComponentId { | ||
| if let Some(&id) = self.indices.get(&TypeId::of::<T>()) { |
There was a problem hiding this comment.
Oh good. This has annoyed me before; I should have thought to check if it's a perf improvement.
| constructor: impl Fn() -> C + 'static, | ||
| ) { | ||
| let id = components.register_component::<C>(); | ||
| let id = components.register_component_checked( |
There was a problem hiding this comment.
Can you please add a comment for why this was changed? I can't immediately tell by looking at it, which means that future contributors will be confused too.
There was a problem hiding this comment.
I'm just gonna revert that part, it doesn't really affect performance
alice-i-cecile
added
X-Uncontroversial
|
Still the same performance (besides noise) after last commit: |
JaySpruce
added
S-Needs-Review
| if let Some(&id) = self.indices.get(&TypeId::of::<T>()) { | ||
| enforce_no_required_components_recursion(self, &self.recursion_check_stack, id); | ||
| return id; | ||
| } |
There was a problem hiding this comment.
This should not be needed, as register_component_checked already does this in its first line, and it only takes constants as inputs (note that here we're inside a ComponentsRegistrator so the Vec was already created.
There was a problem hiding this comment.
In this case I was trying to avoid the ComponentDescriptor::new because it's a big type, but I've just realized it's passing the function pointer and not actually calling it lol. I'll remove this one
| if let Some(id) = self.component_id::<T>() { | ||
| return id; | ||
| } | ||
|
|
||
| self.components_registrator().register_component::<T>() |
There was a problem hiding this comment.
Small note here: I believe in this and the other cases we should be able to skip the same check in the register_component/register_bundle_info/etc etc. However I would not give it too much weight since it will only happen very rarely (when we actually register components) and increase the code complexity unnecessarily.
chescock
left a comment
chescock
left a comment
There was a problem hiding this comment.
Looks good! This is a clear and simple change with a measured performance improvement.
I'm surprised that this helped that much! Creating an empty Vec is pretty cheap since it doesn't allocate, so I would have thought that the cost of the extra HashMap lookup would make it not worth it. It's a good thing we have benchmarks to listen to instead of me :).
| return id; | ||
| } | ||
|
|
||
| self.register_component_checked( |
There was a problem hiding this comment.
Would it help to use #[cold] or cold_path() somewhere?
The pattern I've seen before, like in Vec::reserve, is to have a quick check that gets #[inline]d into the caller, and then the slow complex part in a #[cold] function. Then the common case has just the quick check and a branch not taken, as even the code to set up arguments for the expensive call gets pushed out of the hot path.
There was a problem hiding this comment.
Doesn't seem to make a difference unfortunately
JaySpruce
added
S-Ready-For-Final-Review
4 participants
Objective
World::register_bundleis called near the beginning of every bundle-related operation (i.e.insertorremove) to get theBundleIdof the bundle, or register the bundle if it's new. However, the check for whether it already exists only happens after aComponentsRegistratoris created, which happens to contain aVec. The registrator isn't necessary to check if the bundle exists, so theoretically we're creating an emptyVecfor no reason at every insertion or similar operation.Solution
Add earlier outs, including other similar situations.
A little bit of performance:
