Componentize system-internal QueryState.

[re0312]

Objective

• Part 2 of #19454
• Still split from #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

[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.

[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.

[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.