Skip to content

Commit

Permalink
Reduce code duplication by using QueryIterationCursor in QueryIter (b…
Browse files Browse the repository at this point in the history 
…evyengine#4733)

# 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
```
  • Loading branch information
@james7132 @ItsDoot
james7132 authored and ItsDoot committed Feb 1, 2023
1 parent 1cc12e4 commit 2966d5a
Showing 1 changed file with 18 additions and 97 deletions.
115 changes: 18 additions & 97 deletions crates/bevy_ecs/src/query/iter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Q, F>,
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>
Expand All @@ -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::<F>::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),
}
}
}
Expand All @@ -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<Self::Item> {
unsafe {
if QF::IS_DENSE && <QueryFetch<'static, F>>::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<usize>) {
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;
Expand All @@ -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<Q, F>,
world: &'w World,
cursors: [QueryIterationCursor<'w, 's, Q, QueryFetch<'w, Q>, F>; K],
}

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
}
}
Expand Down Expand Up @@ -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 && <QueryFetch<'static, F>>::IS_DENSE;

unsafe fn init_empty(
world: &'w World,
query_state: &'s QueryState<Q, F>,
Expand Down Expand Up @@ -409,7 +331,7 @@ where
#[inline]
unsafe fn peek_last(&mut self) -> Option<QF::Item> {
if self.current_index > 0 {
if QF::IS_DENSE && <QueryFetch<'static, F>>::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))
Expand All @@ -420,16 +342,15 @@ 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,
tables: &'w Tables,
archetypes: &'w Archetypes,
query_state: &'s QueryState<Q, F>,
) -> Option<QF::Item> {
if QF::IS_DENSE && <QueryFetch<'static, F>>::IS_DENSE {
if Self::IS_DENSE {
loop {
if self.current_index == self.current_len {
let table_id = self.table_id_iter.next()?;
Expand Down

0 comments on commit 2966d5a

Please sign in to comment.