Uncached Queries

[cBournhonesque]

Objective

Adds uncached queries: queries that don't cache in memory the list of archetypes that match. These queries can be contrasted with the current cached Queries: every time a query operation is done on cached queries, they start by iterating through new archetypes to check which of these new archetypes match the query.

When we do an operation on a cached query (iter/get/etc.), we use the cached list of sorted archetypes. But for uncached queries, we instead go through world archetypes from scratch to check which archetypes match the query. (Note that we do not go through all the archetypes because we can use the ComponentIndex to reduce the number of archetypes that we have to go through.)

Solution

• For cached queries, the cache is populated by iterating through new archetypes and checking with the uncached version of the query if those archetypes match. iterate_archetypes for uncached queries does the job of checking if an archetype matches the query. In contrast, iterate_archetypes for cached queries just goes through the cached list of matching archetypes.

• Implements uncached queries by adding a 3rd generic QueryCache to QueryState. That generic can essentially have 2 values: CacheState or Uncached.

I opted for this so that:

• we statically know if the query is cached or uncached so we don't pay the cost of if is_cached() calls

• we don't need to write new implementations for all the QueryState functions, only a couple of functions change and are implemented differently for QueryState<CacheState> vs QueryState<Uncached>

• Added a DefaultCache which can be CacheState (cached) or Uncached depending on a feature. This lets us run all existing query tests in both cached or uncached mode.

• Added some type aliases UncachedQuery and UncachedQueryState, as well as methods on World (query_uncached, etc.)

Open questions

• For simplification I removed the check of DefaultQueryFilters::is_dense to check if a query is dense. I believe that it doesn't make sense for a DefulatQueryFilter to not be dense, since we need it to be dense to efficiently filter disabled vs non-disabled queries. We should probably add some error (panic?) if a user tries to register a non-dense component as a DefaultQueryFilter.

• Some operations are slower than necessary for uncached queries. For instance query.iter().sort() will go through all archetypes twice: once for the iter() and once for the sort(). This might be avoidable but I think it can be punted to a future PR.

[james-j-obrien]

Ignoring the WIP nature of the PR, this seems like broadly the correct approach to me i.e. splitting up the query state such that the "cache" portion is responsible for managing a list of storages to iterate.

Having an additional generic on all these query related structs and traits is an ergonomic hit, the bigger these definitions get the "scarier" they get in the sense that they become harder and harder to grok, but none of the alternatives are free either.

[github-actions]

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

[alice-i-cecile]

As SME-ECS, I'm broadly on board with this direction and excite to see this evolve!

[cBournhonesque]

Updated the PR to make the tests pass.

Things i'd like to change:

• ideally not add a 3rd generic to QueryState
• we realistically only have 2 implementations for QueryCache, so maybe we don't need a trait
• split_cache is very unsafe and miri errors on it
• maybe we can just have a UncachedQuery and a CachedQuery that contains it + the cache?

[alice-i-cecile]

maybe we can just have a UncachedQuery and a CachedQuery that contains it + the cache?

This seems like a simpler design. Explore that and report back?

[chescock]

I haven't read through all the code here yet, but I wanted to brainstorm some possible connections:

Could this supersede #18162 and #19787? The goal there was to make QueryLens usable without needing to store it in a local, and to make it possible for a world.query() method that you can immediately iterate. But I think those use cases really want uncached queries!

I've also started suspecting that we'll want to split the typed WorldQuery::States out of QueryState somehow. Queries as entities (#19454) is going to want an untyped QueryState component, so it might need to store the WorldQuery::States elsewhere. And Query::transmute_lens actually wants to re-use everything except the WorldQuery::States in the returned lens, and I think could share the cache from the original query if there were a way to re-use a borrow to the rest of the QueryState. So we might want to chop QueryState into three pieces instead of just two.

Hmm, I see you're adding a type parameter to QueryState. Given that you have to add a type parameter to Query either way, what would you think of an approach like #18162 where we put QueryState itself behind a trait? The cached query state would be a struct that contains the uncached state.

[cBournhonesque]

Could this supersede #18162 and #19787? The goal there was to make QueryLens usable without needing to store it in a local, and to make it possible for a world.query() method that you can immediately iterate. But I think those use cases really want uncached queries!

Maybe? I'm less familiar with QueryLens; if I understand correctly they are just a place to store the new QueryState when we are transmuting a Query (because Query only borrows QueryState).
I think it's kind of independent from uncached queries, you could want to transmute a cached Query into another Query that still remains cached! I believe your approach of making it possible for the Query to own its QueryState is correct. Maybe instead of a new type parameter, the Query can own a Cow<QueryState> ?

I've also started suspecting that we'll want to split the typed WorldQuery::States out of QueryState somehow. Queries as entities (#19454) is going to want an untyped QueryState component, so it might need to store the WorldQuery::States elsewhere. And Query::transmute_lens actually wants to re-use everything except the WorldQuery::States in the returned lens, and I think could share the cache from the original query if there were a way to re-use a borrow to the rest of the QueryState. So we might want to chop QueryState into three pieces instead of just two.

My plan here (I talked a bit about it in #ecs-dev) was to have the query logic be entirely dynamic; the QueryState would just store an untyped QueryPlan that would help you get the correct ECS data.
A typed Query<D, F> would start by creating a QueryPlan from D and F, use the plan to identify the correct entities to match, and then call D::from_state(DynamicState) to fetch the correct data. This plan would also allow multi-source queries (etc.).

Hmm, I see you're adding a type parameter to QueryState. Given that you have to add a type parameter to Query either way, what would you think of an approach like #18162 where we put QueryState itself behind a trait? The cached query state would be a struct that contains the uncached state.

Yes I was thinking of doing this, I think that's the correct approach!
Currently my iteration-related traits are on the Cache structs, but it's a bit weird, they should probably be in a QueryState-related trait. Oh I see that your PR #18162 does exactly this!
Your

fn storage_ids(&self) -> Self::StorageIter {
        // Query iteration holds both the state and the storage iterator,
        // so the iterator cannot borrow from the state,
        // which requires us to `clone` the `Vec` here.
        self.matched_storage_ids.clone().into_iter()
    }

is a better version of my

pub trait QueryCache: Debug + Clone + Sync {
    /// Returns the data needed to iterate through the archetypes that match the query.
    /// Usually used to populate a `QueryIterationCursor`
    fn iteration_data<'s, 'a: 's, D: QueryData, F: QueryFilter>(
        &'a self,
        query: &QueryState<D, F, Self>,
        world: UnsafeWorldCell,
    ) -> IterationData<'s>;
 }