Queries as Entities #14668

Trashtalk217 · 2024-08-08T22:24:14Z

Objective

Query's caches of matching tables and archetypes are updated each time the query is accessed by iterating through the archetypes that have been created since it was last accessed. As the number of archetypes in bevy is expected to stabilize over the runtime of a program this isn't currently an issue. However with the increased archetype fragmentation caused by fragmenting relationships and the new wrinkle of archetype deletion the updating of the query caches is poised to become a performance bottleneck.

An appropriate solution would allow us to expose new/deleted archetypes to only the queries that may be interested in iterating them. Luckily observers #10839 could cleanly model this pattern. By sending ECS triggers each time an archetype is created and creating an observer for each query we can target the updates so that we no longer need to iterate every new archetype. Similarly we can fire archetype deletion events to cause queries to remove the archetype from their cache.

Solution

Continuing the quote:

In order to implement such a mechanism we need some way for the observer to have access to the query state. Currently QueryState is stored in the system state, so in order to update it we would need to expose several additional APIs, even then it's quite difficult to reach into the system state tuple to access the correct query without some macros to handle tuple indexing. An alternative would be to move the query state into a component on an associated entity (WIP branch here: queries-as-entities) this provides additional benefits in making it easier manage dynamic queries, but they are largely tangential to this RFC.

Migration Guide

TBD

Trashtalk217 · 2024-08-08T22:31:01Z

This is currently blocked by #12144 because all of our nice query entities are nuked in the render world. After #14449 is merged this should be resolved.

JMS55 · 2024-08-08T23:38:23Z

Can we not add a market component to query-entities and other internal stuff, and only nuke entities without that component?

james-j-obrien · 2024-08-08T23:43:02Z

Can we not add a market component to query-entities and other internal stuff, and only nuke entities without that component?

It's more complicated than that. Currently we clear all entities by just clearing the storage so to only clear a selection of entities would be a change in itself and has perf implications. The more complex aspect is currently entities in the render world have the same entity id as the main world, this breaks if, for example, a main world entity gets extracted and tries to take the same id as a query that's been retained.

SkiFire13 · 2024-08-09T11:34:31Z

crates/bevy_ecs/src/system/query.rs

        last_run: Tick,
        this_run: Tick,
    ) -> Self {
-        state.validate_world(world.id());
+        let query_state = world.world_mut().get::<QueryState<D, F>>(state).unwrap();


This is unsound since there may be other system parameters that e.g. immutably borrow metadata from the world. This should use UnsafeWorldCell::get_entity instead. Also, to make it completly safe, queries should register access to their state components.

It feels like transforming this to use EntityRef shouldn't be too difficult since the Query should own the Entity.

EntityRef requires read access to all other components of that Entity as well, which will make it conflict with any other mutable query.

I missed that this didn't namespace the entities it used at all...

I suspect this kind of work will require name-spacing of some kind to avoid the conflict.

After all if you have a Query that returns all Queries (assuming they are type erased) what sense does that make?

It doesn't really make sense, but the way system params access currently works you can only request access for all components with a given id/type on entities that have or not some other components.

This will definitely need something like namespacing otherwise Query<EntityMut> would not work.

I am just saying that caution should be taken to avoid expanding existing "everything" definitions ala EntityRef/EntityMut to suddenly block infrastructure bits from working. Those things didn't overlap before they would only overlap in the entity backed world if we didn't keep them distinct.

Solutions could be a separate world for an extreme example or segregating things so that the system knows that a query entity doesn't have a component on it that a user would add.

The main reason I brought up EntityRef is because it seems to more closely match what was written here. I guess it is technically a hypothetical combination of EntityRef and a Query<&QueryState<D, F>> (a bit recursive there) but I didn't think that type existed so referenced the other one.

Mostly it was "if Query can share the entity how does that work" which I thought was what EntityRef was about "shared ownership of an Entity"

SkiFire13 · 2024-08-09T11:34:54Z

crates/bevy_ecs/src/system/query.rs

+    fn get_query_state(&self) -> &QueryState<D, F> {
+        return unsafe { self.world.world_mut().get::<QueryState<D, F>>(self.state).unwrap() }
+    }


SkiFire13 · 2024-08-09T11:35:42Z

crates/bevy_ecs/src/system/query.rs

+    fn get_query_state(&self) -> &QueryState<D, F> {
+        return unsafe { self.world.world_mut().get::<QueryState<D, F>>(self.state).unwrap() }
+    }


Also, couldn't this access be done only once in Query::new (thus going back to Query holding a &QueryState, except now it's a &'w QueryState)?

SkiFire13 · 2024-08-09T11:36:09Z

crates/bevy_ecs/src/system/query.rs

-        let new_state = self.state.as_readonly();
+        let mut query_state = self.get_query_state();
+        let new_state = query_state.as_readonly().clone();
+        let read_only_state = unsafe { self.world.world_mut() }.spawn(*new_state).id();


This is very unsound not only due to the creation of the &mut World, but also because spawn actually needs exclusive access to the world to mutate its metadata.

IMO this is a big blocker because spawning entities is something that you just cannot do without exclusive access to the world, but obviously we don't want each query to require exclusive access to do so.

We don't need another entity here. as_readonly calls pointer::cast and so doesn't need to be copied.

I am not sure the ideal solution but I think type erasing QueryState<D, F> and "transmuting" that type erased form into a QueryState<D, F> should work here as the underlying bytes are the same between the two forms.

However another reason to call clone here is ownership, specifically how do we share ownership of the original QueryState<D, F> since hypothetically both Query should hold a immutable reference to the QueryState<D, F> if it is shared like that. (Of course one would treat it as a QueryState<D::ReadOnly, F> but that is a pointer cast thing)

With how Query is defined right now you need another entity, because the Query<D::ReadOnly, F> will try to access a component of type QueryState<D::ReadOnly, F> on that Entity, but that may only have a component of type QueryState<D, F>.

Query would have to be changed to store a &'w QueryState<D, F> to do what you're proposing.

That is why I said it needs to be type erased, aka store the QueryState in a way that isn't type specific.

Trashtalk217 added 2 commits August 7, 2024 01:46

implement Component for QueryState

8c5b269

made Query to use Entity QueryState

7fb5a9b

Trashtalk217 added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times S-Blocked This cannot move forward until something else changes labels Aug 8, 2024

Trashtalk217 requested a review from james-j-obrien August 8, 2024 22:35

SkiFire13 reviewed Aug 10, 2024

View reviewed changes

Trashtalk217 added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Queries as Entities #14668

Queries as Entities #14668

Trashtalk217 commented Aug 8, 2024

Trashtalk217 commented Aug 8, 2024

JMS55 commented Aug 8, 2024

james-j-obrien commented Aug 8, 2024 •

edited

Loading

SkiFire13 Aug 9, 2024

Guvante Sep 27, 2024

SkiFire13 Sep 27, 2024

Guvante Sep 27, 2024

SkiFire13 Sep 27, 2024

Guvante Sep 27, 2024

SkiFire13 Aug 9, 2024

SkiFire13 Aug 9, 2024

SkiFire13 Aug 9, 2024

Guvante Sep 27, 2024

SkiFire13 Sep 27, 2024

Guvante Sep 27, 2024

Queries as Entities #14668

Are you sure you want to change the base?

Queries as Entities #14668

Conversation

Trashtalk217 commented Aug 8, 2024

Objective

Solution

Migration Guide

Trashtalk217 commented Aug 8, 2024

JMS55 commented Aug 8, 2024

james-j-obrien commented Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-j-obrien commented Aug 8, 2024 •

edited

Loading