cache ArchetypeId in QueryManyIter and QuerySortedIter

[Victoronz]

Objective

When querying for a list of entities, QueryManyIter and QuerySortedIter call D::set_archetype and F::set_archetype with each item fetch (every next() call). Because the vast majority of queries do not have as many archetypes as entities, this is a lot of repeated work. For most QueryData, set_archetype is pretty cheap, but for EntityRef-style types it is implemented as a clone of Access.

Solution

Cache the previous archetype id in the iteration struct, and compare the current archetype id to decide whether to run archetype fetch setup logic.

Consequentially, ìter_many can now perform better than a series of gets. (This was already true, but to a much lesser degree)

Testing

Existing iteration tests.

Potential Future Work

Query::get() is even worse: For EntityRef types, each get() not only constructs an Access with D::init_fetch, but then also clones it twice! By giving Query both a fetch and filter field, D::init_fetch could be avoided in all forms of get() and iter(), and we could perform the same caching trick in all get()s!
However, because we do not have dedicated structs like with iteration, the latter runs into mutability issues: fetch/filter` need to be updated for "last archetype id" caching.

[hymm]

shouldn't this check be outside the new if statement?

[Victoronz]

We only cache valid archetype ids, so if an entity's archetype id matches the last, it must match the query.

However this depends on us initializing the cache with an archetype id that cannot be returned by Entities::get for any possible input entity.

I was under the impression that this held for ArchetypeId::INVALID, but I realize now that this id can be returned by pending entities in QueryManyIter.

Does ArchetypeId::EMPTY work for initialization instead?

[Victoronz]

Actually, it seems that the note on Entities::get about pending entities is incorrect? Which means no need to change anything here.
I hope nothing depends on that noted behavior...

[hymm]

That makes sense. It'd be nice if we could test this, but I'm not sure if it's even possible to create an entity in the invalid archetype. Someone should probably update the docs on Entities::get in another pr.

[hymm]

Hmm, you might be able too between a despawn and a flush.

[Victoronz]

To be clear, this is the implementation of Entities::get:

/// Returns the location of an [`Entity`].
    /// Note: for pending entities, returns `Some(EntityLocation::INVALID)`.
    #[inline]
    pub fn get(&self, entity: Entity) -> Option<EntityLocation> {
        if let Some(meta) = self.meta.get(entity.index() as usize) {
            if meta.generation != entity.generation
                || meta.location.archetype_id == ArchetypeId::INVALID
            {
                return None;
            }
            Some(meta.location)
        } else {
            None
        }
    }

Any invalid ArchetypeId is filtered without exception, the note might have been referring to the validity of the other EntityLocation fields.

[Victoronz]

If we can call iter_many while an archetype (or other location fields) is invalid, we'll have to remove some debug_checked_unwraps

[chescock]

debug_checked_unwrap is unsafe, so this should be in an unsafe block with a safety comment.

It's allowed at the moment because it's inside an unsafe fn, but I think we eventually want to turn on the lint to check for these.

[chescock]

Do we need to worry about unwind safety? If set_archetype panics and the caller does a catch_unwind and then calls this again, we'll call fetch without set_archetype having completed.

If we move the assignment after the call to set_archetype, then there's the risk that we'll call fetch for an entity in the previous archetype without a fresh set_archetype call, which could also be bad.

So maybe we have to set previous_archetype_id to INVALID here and then to location.archetype_id after the call to set_archetype?

Or is something in here already completely unwind-unsafe and we don't need to worry about making it worse?

[Victoronz]

I assume not? Going by unwind safety, QueryIterationCursor can skip over storage ids, which would make several iterator types behave incorrectly.

[chescock]

Yeah, but I don't think skipping entities can cause any unsoundness, since we don't impl TrustedLen anywhere.

We definitely have QueryData where it's unsound to call fetch without calling set_table or set_archetype first. So if you have some EvilQueryData that panics on one table and then works on the next, then a Query<(EvilQueryData, &C)> could cause UB when <&C>::fetch indexes into the wrong table column.

We might be able to blame the unsafe impl WorldQuery for EvilQueryData, but WorldQuery doesn't currently require that set_table and set_archetype not panic.

I'd be gently in favor of setting it to INVALID during the calls just to be safe, but I admit it's a crazy edge case that's unlikely to come up, so I'm not going to push too hard. :)

[Victoronz]

hmm, Query::iter explicitly states:
"This iterator is always guaranteed to return results from each matching entity once and only once." (No # Panic section)
Breaking this could cause UB in code that relies on this.
Besides, storage id skipping could allow EvilQueryData to falsify query emptiness, single and Populated checks, which are also present in get_param/validate_param. It could break wide swaths of bevy machinery!
Maybe we want to adjust the Safety section on WorldQuery?
I'd be in favor of making a note comment for now, and if we want unwind safety, adding it later.

[hymm]

under what conditions does set_archetype panic?

[Victoronz]

It is a trait function (with no Safety requirement regarding panicking), so there is no guarantee that it won't when implemented by some library/user. For in-engine impls set_archetype shouldn't panic.