Store only the IDs needed for Query iteration

[james7132]

Objective

Other than the exposed functions for reading matched tables and archetypes, a QueryState does not actually need both internal Vecs for storing matched archetypes and tables. In practice, it will only use one of the two depending on if it uses dense or archetypal iteration.

Same vein as #12474. The goal is to reduce the memory overhead of using queries, which Bevy itself, ecosystem plugins, and end users are already fairly liberally using.

Solution

Add StorageId, which is a union over TableId and ArchetypeId, and store only one of the two at runtime. Read the slice as if it was one ID depending on whether the query is dense or not.

This follows in the same vein as #5085; however, this one directly impacts heap memory usage at runtime, while #5085 primarily targeted transient pointers that might not actually exist at runtime.

---

Changelog

Changed: QueryState::matched_tables now returns an iterator instead of a reference to a slice.
Changed: QueryState::matched_archetypes now returns an iterator instead of a reference to a slice.

Migration Guide

QueryState::matched_tables and QueryState::matched_archetypes does not return a reference to a slice, but an iterator instead. You may need to use iterator combinators or collect them into a Vec to use it as a slice.

[alice-i-cecile]

Okay, I can see the motivation here. I don't think this is a meaningful regression in terms of readability, and saving memory here is a useful application.

To confirm my understanding:

1. Whether or not a query uses dense or sparse iteration can be determined at constant time, based on whether or not both the data and filter are dense (aka don't contain any sparse-set components).
2. We can reuse the same space to store the table (dense) and archetype (sparse) IDs, since we always care about exactly one of the two.
3. We don't want to use an enum here, because the discriminant would take an extra byte, which we don't need since we can always just reconstruct this based on the constants already stored in our QueryState.

[james7132]

We don't want to use an enum here, because the discriminant would take an extra byte, which we don't need since we can always just reconstruct this based on the constants already stored in our QueryState.

It's actually an extra 4 bytes due to padding, which isn't a huge deal, but the branch on something that should be an invariant is potentially worse in hot loops. It could be an enum and then use DebugCheckedUnwrap, but that might negatively impact readability, may add some optimization blockers to the compiler if widespread enough, and it would always negate any memory savings from this PR.

[cBournhonesque]

I read through this code recently to try to finally understand ECS internals, so i challenged myself to review this

[cBournhonesque]

Do we still also need separate self.matched_tables and self.matched_archetypes bitsets?
It looks like only one of the two will be used, depending on the value of D::IS_DENSE and F::IS_DENSE

Maybe it's still needed for things like join?

[james7132]

Yep exactly, join needs them, and get_unchecked_manual needs specifically matched_archetypes (right now).

[cBournhonesque]

I don't know if it would be useful to specify that this is used for optimizing query iteration; in the case where all components are in tables, we can iterate through table_ids directly instead of archetypes

[cBournhonesque]

I would also maybe add a comment on why this being a union is ok here, as unions are rarer than enums. It's because we know which variant to use depending on QueryState::IS_DENSE so we can skip storing the variant type?

[cBournhonesque]

Does that mean that the table_entities and archetype_entities below could have a similar optiimization as storage_entities: &'w [StorageEntities]?

[james7132]

This is what #5085 does, however the benefit may be limited as this struct doesn't really exist at runtime: it's never formally materialized to the stack or the heap under normal use cases and any inlined iteration will decompose the fetches and updates.

Compare this with the Vec in QueryState, which requires both stack and heap space due to being a persisted heap allocated backing for Query. There's real memory savings by using the union.

Doable but I'm not sure if the savings are worth the introduction of more unsafe and readability impact.