From 2966d5af46a2f9e927a268a9d608fe050a406ded Mon Sep 17 00:00:00 2001 From: James Liu Date: Wed, 18 May 2022 18:34:52 +0000 Subject: [PATCH] Reduce code duplication by using QueryIterationCursor in QueryIter (#4733) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective We have duplicated code between `QueryIter` and `QueryIterationCursor`. Reuse that code. ## Solution - Reuse `QueryIterationCursor` inside `QueryIter`. - Slim down `QueryIter` by removing the `&'w World`. It was only being used by the `size_hint` and `ExactSizeIterator` impls, which can use the QueryState and &Archetypes in the type already. - Benchmark to make sure there is no significant regression. Relevant benchmark results seem to show that there is no tangible difference between the two. Everything seems to be either identical or within a workable margin of error here. ``` group embed-cursor main ----- ------------ ---- fragmented_iter/base 1.00 387.4±19.70ns ? ?/sec 1.07 413.1±27.95ns ? ?/sec many_maps_iter 1.00 27.3±0.22ms ? ?/sec 1.00 27.4±0.10ms ? ?/sec simple_iter/base 1.00 13.8±0.07µs ? ?/sec 1.00 13.7±0.17µs ? ?/sec simple_iter/sparse 1.00 61.9±0.37µs ? ?/sec 1.00 62.2±0.64µs ? ?/sec simple_iter/system 1.00 13.7±0.34µs ? ?/sec 1.00 13.7±0.10µs ? ?/sec sparse_fragmented_iter/base 1.00 11.0±0.54ns ? ?/sec 1.03 11.3±0.48ns ? ?/sec world_query_iter/50000_entities_sparse 1.08 105.0±2.68µs ? ?/sec 1.00 97.5±2.18µs ? ?/sec world_query_iter/50000_entities_table 1.00 27.3±0.13µs ? ?/sec 1.00 27.3±0.37µs ? ?/sec ``` --- crates/bevy_ecs/src/query/iter.rs | 115 +++++------------------------- 1 file changed, 18 insertions(+), 97 deletions(-) diff --git a/crates/bevy_ecs/src/query/iter.rs b/crates/bevy_ecs/src/query/iter.rs index 25bc9720b2984e..5b5094f647bcca 100644 --- a/crates/bevy_ecs/src/query/iter.rs +++ b/crates/bevy_ecs/src/query/iter.rs @@ -16,13 +16,7 @@ pub struct QueryIter<'w, 's, Q: WorldQuery, QF: Fetch<'w, State = Q::State>, F: tables: &'w Tables, archetypes: &'w Archetypes, query_state: &'s QueryState, - world: &'w World, - table_id_iter: std::slice::Iter<'s, TableId>, - archetype_id_iter: std::slice::Iter<'s, ArchetypeId>, - fetch: QF, - filter: QueryFetch<'w, F>, - current_len: usize, - current_index: usize, + cursor: QueryIterationCursor<'w, 's, Q, QF, F>, } impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> QueryIter<'w, 's, Q, QF, F> @@ -40,30 +34,11 @@ where last_change_tick: u32, change_tick: u32, ) -> Self { - let fetch = QF::init( - world, - &query_state.fetch_state, - last_change_tick, - change_tick, - ); - let filter = QueryFetch::::init( - world, - &query_state.filter_state, - last_change_tick, - change_tick, - ); - QueryIter { - world, query_state, tables: &world.storages().tables, archetypes: &world.archetypes, - fetch, - filter, - table_id_iter: query_state.matched_table_ids.iter(), - archetype_id_iter: query_state.matched_archetype_ids.iter(), - current_len: 0, - current_index: 0, + cursor: QueryIterationCursor::init(world, query_state, last_change_tick, change_tick), } } } @@ -74,73 +49,20 @@ where { type Item = QF::Item; - // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: - // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual - // We can't currently reuse QueryIterationCursor in QueryIter for performance reasons. See #1763 for context. #[inline(always)] fn next(&mut self) -> Option { unsafe { - if QF::IS_DENSE && >::IS_DENSE { - loop { - if self.current_index == self.current_len { - let table_id = self.table_id_iter.next()?; - let table = &self.tables[*table_id]; - self.fetch.set_table(&self.query_state.fetch_state, table); - self.filter.set_table(&self.query_state.filter_state, table); - self.current_len = table.len(); - self.current_index = 0; - continue; - } - - if !self.filter.table_filter_fetch(self.current_index) { - self.current_index += 1; - continue; - } - - let item = self.fetch.table_fetch(self.current_index); - - self.current_index += 1; - return Some(item); - } - } else { - loop { - if self.current_index == self.current_len { - let archetype_id = self.archetype_id_iter.next()?; - let archetype = &self.archetypes[*archetype_id]; - self.fetch.set_archetype( - &self.query_state.fetch_state, - archetype, - self.tables, - ); - self.filter.set_archetype( - &self.query_state.filter_state, - archetype, - self.tables, - ); - self.current_len = archetype.len(); - self.current_index = 0; - continue; - } - - if !self.filter.archetype_filter_fetch(self.current_index) { - self.current_index += 1; - continue; - } - - let item = self.fetch.archetype_fetch(self.current_index); - self.current_index += 1; - return Some(item); - } - } + self.cursor + .next(self.tables, self.archetypes, self.query_state) } } fn size_hint(&self) -> (usize, Option) { let max_size = self .query_state - .matched_archetypes - .ones() - .map(|index| self.world.archetypes[ArchetypeId::new(index)].len()) + .matched_archetype_ids + .iter() + .map(|id| self.archetypes[*id].len()) .sum(); let archetype_query = F::Fetch::IS_ARCHETYPAL && QF::IS_ARCHETYPAL; @@ -153,7 +75,6 @@ pub struct QueryCombinationIter<'w, 's, Q: WorldQuery, F: WorldQuery, const K: u tables: &'w Tables, archetypes: &'w Archetypes, query_state: &'s QueryState, - world: &'w World, cursors: [QueryIterationCursor<'w, 's, Q, QueryFetch<'w, Q>, F>; K], } @@ -195,7 +116,6 @@ impl<'w, 's, Q: WorldQuery, F: WorldQuery, const K: usize> QueryCombinationIter< } QueryCombinationIter { - world, query_state, tables: &world.storages().tables, archetypes: &world.archetypes, @@ -289,9 +209,9 @@ where let max_size: usize = self .query_state - .matched_archetypes - .ones() - .map(|index| self.world.archetypes[ArchetypeId::new(index)].len()) + .matched_archetype_ids + .iter() + .map(|id| self.archetypes[*id].len()) .sum(); if max_size < K { @@ -324,9 +244,9 @@ where { fn len(&self) -> usize { self.query_state - .matched_archetypes - .ones() - .map(|index| self.world.archetypes[ArchetypeId::new(index)].len()) + .matched_archetype_ids + .iter() + .map(|id| self.archetypes[*id].len()) .sum() } } @@ -363,6 +283,8 @@ impl<'w, 's, Q: WorldQuery, QF, F: WorldQuery> QueryIterationCursor<'w, 's, Q, Q where QF: Fetch<'w, State = Q::State>, { + const IS_DENSE: bool = QF::IS_DENSE && >::IS_DENSE; + unsafe fn init_empty( world: &'w World, query_state: &'s QueryState, @@ -409,7 +331,7 @@ where #[inline] unsafe fn peek_last(&mut self) -> Option { if self.current_index > 0 { - if QF::IS_DENSE && >::IS_DENSE { + if Self::IS_DENSE { Some(self.fetch.table_fetch(self.current_index - 1)) } else { Some(self.fetch.archetype_fetch(self.current_index - 1)) @@ -420,8 +342,7 @@ where } // NOTE: If you are changing query iteration code, remember to update the following places, where relevant: - // QueryIter, QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual - // We can't currently reuse QueryIterationCursor in QueryIter for performance reasons. See #1763 for context. + // QueryIterationCursor, QueryState::for_each_unchecked_manual, QueryState::par_for_each_unchecked_manual #[inline(always)] unsafe fn next( &mut self, @@ -429,7 +350,7 @@ where archetypes: &'w Archetypes, query_state: &'s QueryState, ) -> Option { - if QF::IS_DENSE && >::IS_DENSE { + if Self::IS_DENSE { loop { if self.current_index == self.current_len { let table_id = self.table_id_iter.next()?;