Componentize system-internal QueryState.

[re0312]

Objective

• Part 2 of Query as Entities #19454
• Still split from Queries as entities #18860(authored) by @notmd) for better review and limit implementation impact. so all credit for this work belongs to @notmd .

Solution

• Componentize system-internal QueryState by wrapping it into an InternalQueryStatem compoent.

Performance

ecs system

ecs iteration

This PR should not significantly impact existing benchmarks. Any observed discrepancies are likely due to noise.

However the potential archetype fragmentation - which isn't covered by current benchmark - may have more substantial effects in real scenario, for example, in many_cubes , the archetype counts increase from 23 to 262.
but the type-erased QueryState implementation will ultimately resolve these fragmentation concerns

[notmd]

Looks good, just small question

[Victoronz]

This function should have a safety contract.

It is not sound to use this in SystemParam::get_param!
That impl passes on a QueryState reference to the resulting Query, meaning any other Query SystemParam with the same QueryState would now create a mutable reference while an aliasing one is still live.

[re0312]

I'm not entirely sure if I fully grasp your meaning, but each query system parameter maintains its own unique entity, so it shouldn't create mutable aliasing.

[notmd]

Systems only need mutable access in this PR. Up coming PR will use observer to update cache, and observer will be the only place need it

[Victoronz]

I'm not entirely sure if I fully grasp your meaning, but each query system parameter maintains its own unique entity, so it shouldn't create mutable aliasing.

Hmm, maybe I've misunderstood, but how I see the issue:
Per my understanding, it is legal to have the exact same Query type twice in the same system, given that access doesn't conflict (when read-only, f.e.). If the unique entities are differentiated by Query type, then that means one Entity will then serve multiple system parameters.
However, if you say that this is not the case, then my question goes somewhere else.
If get_param accesses the ECS, then per the safety contract of get_param, this access must be registered in init_state.
If the access for a Query type is registered in a type-based way, which is what I believe the current API does, then how do we prevent conflicts when multiple instances exist?
It did not seem to me that the current access infrastructure is fine-grained enough for entity-disjoint mutable access.

Does this make more sense?

[re0312]

Even when using the same query type twice in a single system, the implementation creates two distinct querystate entities. As a result, get_param operations will never intersect between these instances.

[Victoronz]

I think the issue I pointed out here is core to why the previous attempts failed, and it seems pretty difficult to solve.
I believe #18162 and its follow-ups can significantly improve on the current messiness of the Query/QueryState implementation, which might alleviate some of the pains here!

[re0312]

I think the issue I pointed out here is core to why the previous attempts failed, and it seems pretty difficult to solve. I believe #18162 and its follow-ups can significantly improve on the current messiness of the Query/QueryState implementation, which might alleviate some of the pains here!

Could you please provide a more detailed example? IMO, this pull request appears unrelated to the issue addressed in #18162.

[Victoronz]

I think the issue I pointed out here is core to why the previous attempts failed, and it seems pretty difficult to solve. I believe #18162 and its follow-ups can significantly improve on the current messiness of the Query/QueryState implementation, which might alleviate some of the pains here!

Could you please provide a more detailed example? IMO, this pull request appears unrelated to the issue addressed in #18162.

In some ways, the current Query/QueryState combination is more complex than it needs to be. #18162 is one of a few future PRs that will unify and simplify behavior there, which might loosen the requirements for what is needed to create QueryState/Query.
In particular, I felt that further freedom in what QueryState pointers are available could be useful for Queries as Entities, in case some reference-counting/mutex-like/custom pointer functionality could help.

[re0312]

In some ways, the current Query/QueryState combination is more complex than it needs to be. #18162 is one of a few future PRs that will unify and simplify behavior there, which might loosen the requirements for what is needed to create QueryState/Query. In particular, I felt that further freedom in what QueryState pointers are available could be useful for Queries as Entities, in case some reference-counting/mutex-like/custom pointer functionality could help.

emm, Could you clarify how exactly? I don’t see how your suggestion relates to this PR. For queries as entities, the goal is to host the system’s built-in querystate into ecs observer. This change has no impact on regular (non-cached) querystate.

[hymm]

This pr is going to "leak" memory when a system is dropped or when World::query is called as the query state entities are spawned, but never removed.

Edit: was the pr for World::query to return a Query never merged?

[hymm]

This should be wrapped in an unsafe block and state how the safety contract is fulfilled.

[re0312]

This pr is going to "leak" memory when a system is dropped or when World::query is called as the query state entities are spawned, but never removed.

Edit: was the pr for World::query to return a Query never merged?

yeah, the current implementation of one-shot systems and observer system may lead to "memory leak" when users unregister them, as proper cleanup mechanisms are not yet implemented.

[re0312]

This pr is going to "leak" memory when a system is dropped or when World::query is called as the query state entities are spawned, but never removed.
Edit: was the pr for World::query to return a Query never merged?

yeah, the current implementation of one-shot systems and observer system may lead to "memory leak" when users unregister them, as proper cleanup mechanisms are not yet implemented.

Unfortunately, it seems that there's no clean solution to this problem at the moment unless systems as entities landed.

[Trashtalk217]

One minor documentation improvement.

And concerning what @hymm said about leaking memory. I do think this should be addressed. There are only a couple places where we currently drop systems:

• In 'unregister_system' (https://docs.rs/bevy_ecs/0.16.1/src/bevy_ecs/system/system_registry.rs.html#187)
• In 'run_system_once_with' (https://docs.rs/bevy_ecs/0.16.1/src/bevy_ecs/system/system.rs.html#359)
• and observers

It's currently not possible to remove systems from schedules, so we don't have to worry about that.

I believe that we should manually deal with it in these places before we have systems as entities (maybe we can also implement a custom Dropped, I haven't messed with that before). I don't think I can approve merging this before it's resolved.

[Trashtalk217]

I'd like a better explanation for what those safety reasons are. From what I can remember it's mostly so that users don't query for QueryState and subsequently mess with it, since it's an internal engine thing. But there may be more reasons.

[re0312]

One minor documentation improvement.

And concerning what @hymm said about leaking memory. I do think this should be addressed. There are only a couple places where we currently drop systems:

• In 'unregister_system' (https://docs.rs/bevy_ecs/0.16.1/src/bevy_ecs/system/system_registry.rs.html#187)
• In 'run_system_once_with' (https://docs.rs/bevy_ecs/0.16.1/src/bevy_ecs/system/system.rs.html#359)
• and observers

It's currently not possible to remove systems from schedules, so we don't have to worry about that.

The situation may be more complex than this ,While our existing system-build API allows users to construct world-unrelated systems, we cannot support query as entities until the system as entities are properly implemented.