Get names of queued components #18451
Conversation
alice-i-cecile
added
A-ECS
| queued | ||
| .components | ||
| .values() | ||
| .find(|queued| queued.id == id) |
There was a problem hiding this comment.
The linear search here looks expensive. Instead of putting the descriptors in the individual queues, would it make sense to have a separate HashMap<ComponentId, Arc<ComponentDescriptor>>? For that matter, the ComponentIds are dense, so I think it could be Vec<Arc<ComponentDescriptor>>.
There was a problem hiding this comment.
Yeah, the linear search is not ideal, but this is a cold path. Ids are almost always correct unless this is in a inter-world context, and queued components aren't queued for long. I genuinely think it would be slower to cache it's location somewhere or change the backing data structure because that slows everything down by a tiny bit, where as this slows almost nothing down (but by a lot).
That's why I did it this way (even though it hurts my eyes too). But if the consensus is different, I can absolutely do otherwise.
There was a problem hiding this comment.
I left some suggestions, but neither of them should be blocking.
The linear scan still makes me nervous! I'd feel better if we instead added a Vec<ComponentDescriptor> to QueuedRegistration. We know that it's a dense range of indices, so we can index it by offsetting ComponentId.
But if we're okay with the linear scan, then this is a solid implementation of that approach and we should merge it!
crates/bevy_ecs/src/component.rs
Outdated
| .components | ||
| .values() | ||
| .find(|queued| queued.id == id) | ||
| .map(|queued| CowArc::Owned(Arc::new(queued.descriptor.clone()))) |
There was a problem hiding this comment.
Did you mean to make queued.descriptor be an Arc, so that this just clones the Arc? Right now, you're cloning the underlying descriptor and allocating a new Arc each time.
If you do want to clone the descriptor, then there's no need for an Arc and you can return an ordinary Cow<ComponentDescriptor> here.
For that matter, if you're willing to just have get_name and not a general get_descriptor, you could return Cow<str> and just have to clone name. It's a Cow::Borrowed in all cases except dynamic components, so that will usually be a cheap reference copy.
There was a problem hiding this comment.
Yeah this was a mess.
Originally it was an arc, which is why we needed a new release of atomicow.
But it being an arc, slowed down other operations because of the extra deref, so we removed it.
So this is basically an artifact. Nice catch.
Now it's removed and we can (ironically) just do normal Cows.
alice-i-cecile
added
S-Ready-For-Final-Review
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
It hurts me to see something unoptimized too, but this is a really slow path, and I think introducing an invariant (even though it's incidentally upheld today), is not worth it. The migration guide has also been updated BTW. |
Head branch was pushed to by a user without write access
Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
# Objective #18173 allows components to be queued without being fully registered. But much of bevy's debug logging contained `components.get_name(id).unwrap()`. However, this panics when the id is queued. This PR fixes this, allowing names to be retrieved for debugging purposes, etc, even while they're still queued. ## Solution We change `ComponentInfo::descriptor` to be `Arc<ComponentDescriptor>` instead of not arc'd. This lets us pass the descriptor around (as a name or otherwise) as needed. The alternative would require some form of `MappedRwLockReadGuard`, which is unstable, and would be terribly blocking. Putting it in an arc also signifies that it doesn't change, which is a nice signal to users. This does mean there's an extra pointer dereference, but I don't think that's an issue here, as almost all paths that use this are for debugging purposes or one-time set ups. ## Testing Existing tests. ## Migration Guide `Components::get_name` now returns `Option<Cow<'_, str>` instead of `Option<&str>`. This is because it now returns results for queued components. If that behavior is not desired, or you know the component is not queued, you can use `components.get_info().map(ComponentInfo::name)` instead. Similarly, `ScheduleGraph::conflicts_to_string` now returns `impl Iterator<Item = (String, String, Vec<Cow<str>>)>` instead of `impl Iterator<Item = (String, String, Vec<&str>)>`. Because `Cow<str>` derefs to `&str`, most use cases can remain unchanged. --------- Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
Objective
#18173 allows components to be queued without being fully registered. But much of bevy's debug logging contained
components.get_name(id).unwrap(). However, this panics when the id is queued. This PR fixes this, allowing names to be retrieved for debugging purposes, etc, even while they're still queued.Solution
We change
ComponentInfo::descriptorto beArc<ComponentDescriptor>instead of not arc'd. This lets us pass the descriptor around (as a name or otherwise) as needed. The alternative would require some form ofMappedRwLockReadGuard, which is unstable, and would be terribly blocking. Putting it in an arc also signifies that it doesn't change, which is a nice signal to users. This does mean there's an extra pointer dereference, but I don't think that's an issue here, as almost all paths that use this are for debugging purposes or one-time set ups.Testing
Existing tests.
Migration Guide
Components::get_namenow returnsOption<Cow<'_, str>instead ofOption<&str>. This is because it now returns results for queued components. If that behavior is not desired, or you know the component is not queued, you can usecomponents.get_info().map(ComponentInfo::name)instead.Similarly,
ScheduleGraph::conflicts_to_stringnow returnsimpl Iterator<Item = (String, String, Vec<Cow<str>>)>instead ofimpl Iterator<Item = (String, String, Vec<&str>)>. BecauseCow<str>derefs to&str, most use cases can remain unchanged.