Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Merged by Bors] - Remove unnecesary branches/panics from Query accesses #6461

Closed
wants to merge 6 commits into from
Closed

[Merged by Bors] - Remove unnecesary branches/panics from Query accesses #6461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5f55919
Macro debug_checked_unreachable
james7132 Nov 3, 2022
55bff60
More debug_checked_unreachable
james7132 Nov 4, 2022
1959388
Fix CI
james7132 Nov 4, 2022
df56859
Use a trait instead of a macro
james7132 Nov 4, 2022
96057d5
Revert changes to empty example
james7132 Nov 4, 2022
76533d7
semicolon
james7132 Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.toml
View file
Open in desktop
Expand Up @@ -1600,6 +1600,9 @@ target_sdk_version = 31
icon = "@mipmap/ic_launcher"
label = "Bevy Example"

[profile.release]
lto = true

[profile.wasm-release]
inherits = "release"
opt-level = "z"
Expand Down
4 changes: 4 additions & 0 deletions benches/Cargo.toml
View file
Open in desktop
Expand Up @@ -17,6 +17,10 @@ bevy_reflect = { path = "../crates/bevy_reflect" }
bevy_tasks = { path = "../crates/bevy_tasks" }
bevy_utils = { path = "../crates/bevy_utils" }

[profile.release]
opt-level = 3
lto = true

[[bench]]
name = "ecs"
path = "benches/bevy_ecs/benches.rs"
Expand Down
38 changes: 16 additions & 22 deletions crates/bevy_ecs/src/query/fetch.rs
View file
Open in desktop
Expand Up @@ -3,7 +3,7 @@ use crate::{
change_detection::Ticks,
component::{Component, ComponentId, ComponentStorage, ComponentTicks, StorageType},
entity::Entity,
query::{debug_checked_unreachable, Access, FilteredAccess},
query::{Access, DebugCheckedUnwrap, FilteredAccess},
storage::{ComponentSparseSet, Table},
world::{Mut, World},
};
Expand Down Expand Up @@ -552,7 +552,7 @@ unsafe impl<T: Component> WorldQuery for &T {
.storages()
.sparse_sets
.get(component_id)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
}),
}
}
Expand Down Expand Up @@ -585,7 +585,7 @@ unsafe impl<T: Component> WorldQuery for &T {
fetch.table_components = Some(
table
.get_column(component_id)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get_data_slice()
.into(),
);
Expand All @@ -600,14 +600,14 @@ unsafe impl<T: Component> WorldQuery for &T {
match T::Storage::STORAGE_TYPE {
StorageType::Table => fetch
.table_components
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get(table_row)
.deref(),
StorageType::SparseSet => fetch
.sparse_set
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get(entity)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.deref(),
}
}
Expand Down Expand Up @@ -696,7 +696,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
.storages()
.sparse_sets
.get(component_id)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
}),
last_change_tick,
change_tick,
Expand Down Expand Up @@ -730,9 +730,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
&component_id: &ComponentId,
table: &'w Table,
) {
let column = table
.get_column(component_id)
.unwrap_or_else(|| debug_checked_unreachable());
let column = table.get_column(component_id).debug_checked_unwrap();
fetch.table_data = Some((
column.get_data_slice().into(),
column.get_ticks_slice().into(),
Expand All @@ -747,9 +745,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
) -> Self::Item<'w> {
match T::Storage::STORAGE_TYPE {
StorageType::Table => {
let (table_components, table_ticks) = fetch
.table_data
.unwrap_or_else(|| debug_checked_unreachable());
let (table_components, table_ticks) = fetch.table_data.debug_checked_unwrap();
Mut {
value: table_components.get(table_row).deref_mut(),
ticks: Ticks {
Expand All @@ -762,9 +758,9 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {
StorageType::SparseSet => {
let (component, component_ticks) = fetch
.sparse_set
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get_with_ticks(entity)
.unwrap_or_else(|| debug_checked_unreachable());
.debug_checked_unwrap();
Mut {
value: component.assert_unique().deref_mut(),
ticks: Ticks {
Expand Down Expand Up @@ -1038,7 +1034,7 @@ unsafe impl<T: Component> WorldQuery for ChangeTrackers<T> {
.storages()
.sparse_sets
.get(component_id)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
}),
marker: PhantomData,
last_change_tick,
Expand Down Expand Up @@ -1077,7 +1073,7 @@ unsafe impl<T: Component> WorldQuery for ChangeTrackers<T> {
fetch.table_ticks = Some(
table
.get_column(id)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get_ticks_slice()
.into(),
);
Expand All @@ -1092,9 +1088,7 @@ unsafe impl<T: Component> WorldQuery for ChangeTrackers<T> {
match T::Storage::STORAGE_TYPE {
StorageType::Table => ChangeTrackers {
component_ticks: {
let table_ticks = fetch
.table_ticks
.unwrap_or_else(|| debug_checked_unreachable());
let table_ticks = fetch.table_ticks.debug_checked_unwrap();
table_ticks.get(table_row).read()
},
marker: PhantomData,
Expand All @@ -1104,9 +1098,9 @@ unsafe impl<T: Component> WorldQuery for ChangeTrackers<T> {
StorageType::SparseSet => ChangeTrackers {
component_ticks: *fetch
.sparse_set
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get_ticks(entity)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get(),
marker: PhantomData,
last_change_tick: fetch.last_change_tick,
Expand Down
12 changes: 6 additions & 6 deletions crates/bevy_ecs/src/query/filter.rs
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ use crate::{
archetype::{Archetype, ArchetypeComponentId},
component::{Component, ComponentId, ComponentStorage, ComponentTicks, StorageType},
entity::Entity,
query::{debug_checked_unreachable, Access, FilteredAccess, WorldQuery},
query::{Access, DebugCheckedUnwrap, FilteredAccess, WorldQuery},
storage::{ComponentSparseSet, Table},
world::World,
};
Expand Down Expand Up @@ -439,7 +439,7 @@ macro_rules! impl_tick_filter {
world.storages()
.sparse_sets
.get(id)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
}),
marker: PhantomData,
last_change_tick,
Expand Down Expand Up @@ -476,7 +476,7 @@ macro_rules! impl_tick_filter {
) {
fetch.table_ticks = Some(
table.get_column(component_id)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get_ticks_slice()
.into()
);
Expand Down Expand Up @@ -504,7 +504,7 @@ macro_rules! impl_tick_filter {
StorageType::Table => {
$is_detected(&*(
fetch.table_ticks
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get(table_row))
.deref(),
fetch.last_change_tick,
Expand All @@ -514,9 +514,9 @@ macro_rules! impl_tick_filter {
StorageType::SparseSet => {
let ticks = &*fetch
.sparse_set
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get_ticks(entity)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.get();
$is_detected(ticks, fetch.last_change_tick, fetch.change_tick)
}
Expand Down
15 changes: 9 additions & 6 deletions crates/bevy_ecs/src/query/iter.rs
View file
Open in desktop
Expand Up @@ -2,7 +2,7 @@ use crate::{
archetype::{ArchetypeEntity, ArchetypeId, Archetypes},
entity::{Entities, Entity},
prelude::World,
query::{ArchetypeFilter, QueryState, WorldQuery},
query::{ArchetypeFilter, DebugCheckedUnwrap, QueryState, WorldQuery},
storage::{TableId, Tables},
};
use std::{borrow::Borrow, iter::FusedIterator, marker::PhantomData, mem::MaybeUninit};
Expand Down Expand Up @@ -153,8 +153,11 @@ where
continue;
}

let archetype = &self.archetypes[location.archetype_id];
let table = &self.tables[archetype.table_id()];
let archetype = self
.archetypes
.get(location.archetype_id)
.debug_checked_unwrap();
let table = self.tables.get(archetype.table_id()).debug_checked_unwrap();

// SAFETY: `archetype` is from the world that `fetch/filter` were created for,
// `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with
Expand Down Expand Up @@ -586,7 +589,7 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's,
// we are on the beginning of the query, or finished processing a table, so skip to the next
if self.current_index == self.current_len {
let table_id = self.table_id_iter.next()?;
let table = &tables[*table_id];
let table = tables.get(*table_id).debug_checked_unwrap();
// SAFETY: `table` is from the world that `fetch/filter` were created for,
// `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with
Q::set_table(&mut self.fetch, &query_state.fetch_state, table);
Expand Down Expand Up @@ -616,10 +619,10 @@ impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> QueryIterationCursor<'w, 's,
loop {
if self.current_index == self.current_len {
let archetype_id = self.archetype_id_iter.next()?;
let archetype = &archetypes[*archetype_id];
let archetype = archetypes.get(*archetype_id).debug_checked_unwrap();
// SAFETY: `archetype` and `tables` are from the world that `fetch/filter` were created for,
// `fetch_state`/`filter_state` are the states that `fetch/filter` were initialized with
let table = &tables[archetype.table_id()];
let table = tables.get(archetype.table_id()).debug_checked_unwrap();
Q::set_archetype(&mut self.fetch, &query_state.fetch_state, archetype, table);
F::set_archetype(
&mut self.filter,
Expand Down
48 changes: 43 additions & 5 deletions crates/bevy_ecs/src/query/mod.rs
View file
Open in desktop
Expand Up @@ -10,11 +10,49 @@ pub use filter::*;
pub use iter::*;
pub use state::*;

#[allow(unreachable_code)]
pub(crate) unsafe fn debug_checked_unreachable() -> ! {
#[cfg(debug_assertions)]
unreachable!();
std::hint::unreachable_unchecked();
/// A debug checked version of [`Option::unwrap_unchecked`]. Will panic in
/// debug modes if unwrapping a `None` or `Err` value in debug mode, but is
/// equivalent to `Option::unwrap_uncheched` or `Result::unwrap_unchecked`
/// in release mode.
pub(crate) trait DebugCheckedUnwrap {
type Item;
/// # Panics
/// Panics if the value is `None` or `Err`, only in debug mode.
///
/// # Safety
/// This must never be called on a `None` or `Err` value. This can
/// only be called on `Some` or `Ok` values.
unsafe fn debug_checked_unwrap(self) -> Self::Item;
}

// Thes two impls are explicitly split to ensure that the unreachable! macro
// does not cause inlining to fail when compiling in release mode.
#[cfg(debug_assertions)]
impl<T> DebugCheckedUnwrap for Option<T> {
type Item = T;

#[inline(always)]
unsafe fn debug_checked_unwrap(self) -> Self::Item {
if let Some(inner) = self {
inner
} else {
unreachable!()
}
}
}

#[cfg(not(debug_assertions))]
impl<T> DebugCheckedUnwrap for Option<T> {
type Item = T;

#[inline(always)]
unsafe fn debug_checked_unwrap(self) -> Self::Item {
if let Some(inner) = self {
inner
} else {
std::hint::unreachable_unchecked()
}
}
}

#[cfg(test)]
Expand Down
28 changes: 19 additions & 9 deletions crates/bevy_ecs/src/query/state.rs
View file
Open in desktop
Expand Up @@ -3,7 +3,9 @@ use crate::{
component::ComponentId,
entity::Entity,
prelude::FromWorld,
query::{Access, FilteredAccess, QueryCombinationIter, QueryIter, WorldQuery},
query::{
Access, DebugCheckedUnwrap, FilteredAccess, QueryCombinationIter, QueryIter, WorldQuery,
},
storage::TableId,
world::{World, WorldId},
};
Expand Down Expand Up @@ -409,11 +411,18 @@ impl<Q: WorldQuery, F: ReadOnlyWorldQuery> QueryState<Q, F> {
{
return Err(QueryEntityError::QueryDoesNotMatch(entity));
}
let archetype = &world.archetypes[location.archetype_id];
let archetype = world
.archetypes
.get(location.archetype_id)
.debug_checked_unwrap();
let mut fetch = Q::init_fetch(world, &self.fetch_state, last_change_tick, change_tick);
let mut filter = F::init_fetch(world, &self.filter_state, last_change_tick, change_tick);

let table = &world.storages().tables[archetype.table_id()];
let table = world
.storages()
.tables
.get(archetype.table_id())
.debug_checked_unwrap();
Q::set_archetype(&mut fetch, &self.fetch_state, archetype, table);
F::set_archetype(&mut filter, &self.filter_state, archetype, table);

Expand Down Expand Up @@ -930,7 +939,7 @@ impl<Q: WorldQuery, F: ReadOnlyWorldQuery> QueryState<Q, F> {
let tables = &world.storages().tables;
if Q::IS_DENSE && F::IS_DENSE {
for table_id in &self.matched_table_ids {
let table = &tables[*table_id];
let table = tables.get(*table_id).debug_checked_unwrap();
Q::set_table(&mut fetch, &self.fetch_state, table);
F::set_table(&mut filter, &self.filter_state, table);

Expand All @@ -946,8 +955,8 @@ impl<Q: WorldQuery, F: ReadOnlyWorldQuery> QueryState<Q, F> {
} else {
let archetypes = &world.archetypes;
for archetype_id in &self.matched_archetype_ids {
let archetype = &archetypes[*archetype_id];
let table = &tables[archetype.table_id()];
let archetype = archetypes.get(*archetype_id).debug_checked_unwrap();
let table = tables.get(archetype.table_id()).debug_checked_unwrap();
Q::set_archetype(&mut fetch, &self.fetch_state, archetype, table);
F::set_archetype(&mut filter, &self.filter_state, archetype, table);

Expand Down Expand Up @@ -1025,7 +1034,7 @@ impl<Q: WorldQuery, F: ReadOnlyWorldQuery> QueryState<Q, F> {
change_tick,
);
let tables = &world.storages().tables;
let table = &tables[*table_id];
let table = tables.get(*table_id).debug_checked_unwrap();
let entities = table.entities();
Q::set_table(&mut fetch, &self.fetch_state, table);
F::set_table(&mut filter, &self.filter_state, table);
Expand Down Expand Up @@ -1076,8 +1085,9 @@ impl<Q: WorldQuery, F: ReadOnlyWorldQuery> QueryState<Q, F> {
change_tick,
);
let tables = &world.storages().tables;
let archetype = &world.archetypes[*archetype_id];
let table = &tables[archetype.table_id()];
let archetype =
world.archetypes.get(*archetype_id).debug_checked_unwrap();
let table = tables.get(archetype.table_id()).debug_checked_unwrap();
Q::set_archetype(&mut fetch, &self.fetch_state, archetype, table);
F::set_archetype(&mut filter, &self.filter_state, archetype, table);

Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/storage/table.rs
View file
Open in desktop
@@ -1,7 +1,7 @@
use crate::{
component::{ComponentId, ComponentInfo, ComponentTicks, Components},
entity::Entity,
query::debug_checked_unreachable,
query::DebugCheckedUnwrap,
storage::{blob_vec::BlobVec, SparseSet},
};
use bevy_ptr::{OwningPtr, Ptr, PtrMut};
Expand Down Expand Up @@ -386,7 +386,7 @@ impl Table {
for (component_id, column) in self.columns.iter_mut() {
new_table
.get_column_mut(*component_id)
.unwrap_or_else(|| debug_checked_unreachable())
.debug_checked_unwrap()
.initialize_from_unchecked(column, row, new_row);
}
TableMoveResult {
Expand Down