Skip to content

Commit

Permalink
Inline more ECS functions (#8083)
Browse files Browse the repository at this point in the history
# Objective
Upon closer inspection, there are a few functions in the ECS that are
not being inlined, even with the highest optimizations and LTO enabled:

- Almost all
[WorldQuery::init_fetch](https://github.com/james7132/bevy_asm_tests/blob/9fd5f20e252f6e4401d352a0b675098d7e010aff/results/query_get.s#L57)
calls. Affects `Query::get` calls in hot loops. In particular, the
`WorldQuery` implementation for `()` is used *everywhere* as the default
filter and is effectively a no-op.
-
[Entities::get](https://github.com/james7132/bevy_asm_test/blob/9fd5f20e252f6e4401d352a0b675098d7e010aff/results/query_get.s#L39).
Affects `Query::get`, `World::get`, and any component insertion or
removal.
-
[Entities::set](https://github.com/james7132/bevy_asm_tests/blob/9fd5f20e252f6e4401d352a0b675098d7e010aff/results/entity_remove.s#L2487).
Affects any component insertion or removal.
-
[Tick::new](https://github.com/james7132/bevy_asm_tests/blob/9fd5f20e252f6e4401d352a0b675098d7e010aff/results/entity_insert.s#L1368).
I've only seen this in component insertion and spawning.
 - ArchetypeRow::new
 - BlobVec::set_len

Almost all of these have trivial or even empty implementations or have
significant opportunity to be optimized into surrounding code when
inlined with LTO enabled.

## Solution
Inline them
  • Loading branch information
james7132 committed Apr 12, 2023
1 parent f3c7cce commit 2ec38d1
Show file tree
Hide file tree
Showing 11 changed files with 40 additions and 1 deletion.
4 changes: 4 additions & 0 deletions crates/bevy_ecs/src/archetype.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ impl ArchetypeRow {
pub const INVALID: ArchetypeRow = ArchetypeRow(u32::MAX);

/// Creates a `ArchetypeRow`.
#[inline]
pub const fn new(index: usize) -> Self {
Self(index as u32)
}
Expand Down Expand Up @@ -433,6 +434,7 @@ impl Archetype {
/// # Safety
/// valid component values must be immediately written to the relevant storages
/// `table_row` must be valid
#[inline]
pub(crate) unsafe fn allocate(
&mut self,
entity: Entity,
Expand All @@ -449,6 +451,7 @@ impl Archetype {
}
}

#[inline]
pub(crate) fn reserve(&mut self, additional: usize) {
self.entities.reserve(additional);
}
Expand All @@ -458,6 +461,7 @@ impl Archetype {
///
/// # Panics
/// This function will panic if `index >= self.len()`
#[inline]
pub(crate) fn swap_remove(&mut self, row: ArchetypeRow) -> ArchetypeSwapRemoveResult {
let is_last = row.index() == self.entities.len() - 1;
let entity = self.entities.swap_remove(row.index());
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ impl SparseSetIndex for BundleId {
self.index()
}

#[inline]
fn get_sparse_set_index(value: usize) -> Self {
Self(value)
}
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_ecs/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ impl SparseSetIndex for ComponentId {
self.index()
}

#[inline]
fn get_sparse_set_index(value: usize) -> Self {
Self(value)
}
Expand Down Expand Up @@ -601,6 +602,7 @@ impl Tick {
/// ticks are periodically scanned to ensure their relative values are below this.
pub const MAX: Self = Self::new(MAX_CHANGE_AGE);

#[inline]
pub const fn new(tick: u32) -> Self {
Self { tick }
}
Expand All @@ -617,10 +619,10 @@ impl Tick {
self.tick = tick;
}

#[inline]
/// Returns `true` if this `Tick` occurred since the system's `last_run`.
///
/// `this_run` is the current tick of the system, used as a reference to help deal with wraparound.
#[inline]
pub fn is_newer_than(self, last_run: Tick, this_run: Tick) -> bool {
// This works even with wraparound because the world tick (`this_run`) is always "newer" than
// `last_run` and `self.tick`, and we scan periodically to clamp `ComponentTicks` values
Expand All @@ -634,6 +636,7 @@ impl Tick {
}

/// Returns a change tick representing the relationship between `self` and `other`.
#[inline]
pub(crate) fn relative_to(self, other: Self) -> Self {
let tick = self.tick.wrapping_sub(other.tick);
Self { tick }
Expand All @@ -642,6 +645,7 @@ impl Tick {
/// Wraps this change tick's value if it exceeds [`Tick::MAX`].
///
/// Returns `true` if wrapping was performed. Otherwise, returns `false`.
#[inline]
pub(crate) fn check_tick(&mut self, tick: Tick) -> bool {
let age = tick.relative_to(*self);
// This comparison assumes that `age` has not overflowed `u32::MAX` before, which will be true
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_ecs/src/entity/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -234,10 +234,12 @@ impl fmt::Debug for Entity {
}

impl SparseSetIndex for Entity {
#[inline]
fn sparse_set_index(&self) -> usize {
self.index() as usize
}

#[inline]
fn get_sparse_set_index(value: usize) -> Self {
Entity::from_raw(value as u32)
}
Expand Down Expand Up @@ -570,6 +572,7 @@ impl Entities {

/// 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
Expand All @@ -590,6 +593,7 @@ impl Entities {
/// - `index` must be a valid entity index.
/// - `location` must be valid for the entity at `index` or immediately made valid afterwards
/// before handing control to unknown code.
#[inline]
pub(crate) unsafe fn set(&mut self, index: u32, location: EntityLocation) {
// SAFETY: Caller guarantees that `index` a valid entity index
self.meta.get_unchecked_mut(index as usize).location = location;
Expand Down
6 changes: 6 additions & 0 deletions crates/bevy_ecs/src/query/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -550,6 +550,7 @@ unsafe impl<T: Component> WorldQuery for &T {

const IS_ARCHETYPAL: bool = true;

#[inline]
unsafe fn init_fetch<'w>(
world: &'w World,
&component_id: &ComponentId,
Expand Down Expand Up @@ -695,6 +696,7 @@ unsafe impl<'__w, T: Component> WorldQuery for Ref<'__w, T> {

const IS_ARCHETYPAL: bool = true;

#[inline]
unsafe fn init_fetch<'w>(
world: &'w World,
&component_id: &ComponentId,
Expand Down Expand Up @@ -856,6 +858,7 @@ unsafe impl<'__w, T: Component> WorldQuery for &'__w mut T {

const IS_ARCHETYPAL: bool = true;

#[inline]
unsafe fn init_fetch<'w>(
world: &'w World,
&component_id: &ComponentId,
Expand Down Expand Up @@ -1000,6 +1003,7 @@ unsafe impl<T: WorldQuery> WorldQuery for Option<T> {

const IS_ARCHETYPAL: bool = T::IS_ARCHETYPAL;

#[inline]
unsafe fn init_fetch<'w>(
world: &'w World,
state: &T::State,
Expand Down Expand Up @@ -1104,6 +1108,7 @@ macro_rules! impl_tuple_fetch {
)*)
}

#[inline]
#[allow(clippy::unused_unit)]
unsafe fn init_fetch<'w>(_world: &'w World, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> {
let ($($name,)*) = state;
Expand Down Expand Up @@ -1213,6 +1218,7 @@ macro_rules! impl_anytuple_fetch {
)*)
}

#[inline]
#[allow(clippy::unused_unit)]
unsafe fn init_fetch<'w>(_world: &'w World, state: &Self::State, _last_run: Tick, _this_run: Tick) -> Self::Fetch<'w> {
let ($($name,)*) = state;
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_ecs/src/query/filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ unsafe impl<T: Component> WorldQuery for With<T> {

fn shrink<'wlong: 'wshort, 'wshort>(_: Self::Item<'wlong>) -> Self::Item<'wshort> {}

#[inline]
unsafe fn init_fetch(_world: &World, _state: &ComponentId, _last_run: Tick, _this_run: Tick) {}

unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}
Expand Down Expand Up @@ -146,6 +147,7 @@ unsafe impl<T: Component> WorldQuery for Without<T> {

fn shrink<'wlong: 'wshort, 'wshort>(_: Self::Item<'wlong>) -> Self::Item<'wshort> {}

#[inline]
unsafe fn init_fetch(_world: &World, _state: &ComponentId, _last_run: Tick, _this_run: Tick) {}

unsafe fn clone_fetch<'w>(_fetch: &Self::Fetch<'w>) -> Self::Fetch<'w> {}
Expand Down Expand Up @@ -265,6 +267,7 @@ macro_rules! impl_query_filter_tuple {

const IS_ARCHETYPAL: bool = true $(&& $filter::IS_ARCHETYPAL)*;

#[inline]
unsafe fn init_fetch<'w>(world: &'w World, state: &Self::State, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> {
let ($($filter,)*) = state;
($(OrFetch {
Expand Down Expand Up @@ -420,6 +423,7 @@ macro_rules! impl_tick_filter {
item
}

#[inline]
unsafe fn init_fetch<'w>(world: &'w World, &id: &ComponentId, last_run: Tick, this_run: Tick) -> Self::Fetch<'w> {
Self::Fetch::<'w> {
table_ticks: None,
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/storage/blob_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,7 @@ impl BlobVec {
/// Newly added items must be immediately populated with valid values and length must be
/// increased. For better unwind safety, call [`BlobVec::set_len`] _after_ populating a new
/// value.
#[inline]
pub unsafe fn set_len(&mut self, len: usize) {
debug_assert!(len <= self.capacity());
self.len = len;
Expand Down
3 changes: 3 additions & 0 deletions crates/bevy_ecs/src/storage/sparse_set.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,10 +546,12 @@ pub trait SparseSetIndex: Clone + PartialEq + Eq + Hash {
macro_rules! impl_sparse_set_index {
($($ty:ty),+) => {
$(impl SparseSetIndex for $ty {
#[inline]
fn sparse_set_index(&self) -> usize {
*self as usize
}

#[inline]
fn get_sparse_set_index(value: usize) -> Self {
value as $ty
}
Expand Down Expand Up @@ -587,6 +589,7 @@ impl SparseSets {
}

/// Gets a reference to the [`ComponentSparseSet`] of a [`ComponentId`].
#[inline]
pub fn get(&self, component_id: ComponentId) -> Option<&ComponentSparseSet> {
self.sets.get(component_id)
}
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/world/identifier.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ impl SparseSetIndex for WorldId {
self.0
}

#[inline]
fn get_sparse_set_index(value: usize) -> Self {
Self(value)
}
Expand Down
3 changes: 3 additions & 0 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,18 +109,21 @@ impl World {
}

/// Creates a new [`UnsafeWorldCell`] view with complete read+write access.
#[inline]
pub fn as_unsafe_world_cell(&mut self) -> UnsafeWorldCell<'_> {
UnsafeWorldCell::new_mutable(self)
}

/// Creates a new [`UnsafeWorldCell`] view with only read access to everything.
#[inline]
pub fn as_unsafe_world_cell_readonly(&self) -> UnsafeWorldCell<'_> {
UnsafeWorldCell::new_readonly(self)
}

/// Creates a new [`UnsafeWorldCell`] with read+write access from a [&World](World).
/// This is only a temporary measure until every `&World` that is semantically a [`UnsafeWorldCell`]
/// has been replaced.
#[inline]
pub(crate) fn as_unsafe_world_cell_migration_internal(&self) -> UnsafeWorldCell<'_> {
UnsafeWorldCell::new_readonly(self)
}
Expand Down
8 changes: 8 additions & 0 deletions crates/bevy_ecs/src/world/unsafe_world_cell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,13 @@ unsafe impl Sync for UnsafeWorldCell<'_> {}

impl<'w> UnsafeWorldCell<'w> {
/// Creates a [`UnsafeWorldCell`] that can be used to access everything immutably
#[inline]
pub(crate) fn new_readonly(world: &'w World) -> Self {
UnsafeWorldCell(world as *const World as *mut World, PhantomData)
}

/// Creates [`UnsafeWorldCell`] that can be used to access everything mutably
#[inline]
pub(crate) fn new_mutable(world: &'w mut World) -> Self {
Self(world as *mut World, PhantomData)
}
Expand All @@ -99,6 +101,7 @@ impl<'w> UnsafeWorldCell<'w> {
/// - there must be no other borrows on world
/// - returned borrow must not be used in any way if the world is accessed
/// through other means than the `&mut World` after this call.
#[inline]
pub unsafe fn world_mut(self) -> &'w mut World {
// SAFETY:
// - caller ensures the created `&mut World` is the only borrow of world
Expand All @@ -112,6 +115,7 @@ impl<'w> UnsafeWorldCell<'w> {
/// - must have permission to access the whole world immutably
/// - there must be no live exclusive borrows on world data
/// - there must be no live exclusive borrow of world
#[inline]
pub unsafe fn world(self) -> &'w World {
// SAFETY:
// - caller ensures there is no `&mut World` this makes it okay to make a `&World`
Expand All @@ -128,6 +132,7 @@ impl<'w> UnsafeWorldCell<'w> {
///
/// # Safety
/// - must only be used to access world metadata
#[inline]
pub unsafe fn world_metadata(self) -> &'w World {
// SAFETY: caller ensures that returned reference is not used to violate aliasing rules
unsafe { self.unsafe_world() }
Expand All @@ -144,6 +149,7 @@ impl<'w> UnsafeWorldCell<'w> {
/// # Safety
/// - must not be used in a way that would conflict with any
/// live exclusive borrows on world data
#[inline]
pub(crate) unsafe fn unsafe_world(self) -> &'w World {
// SAFETY:
// - caller ensures that the returned `&World` is not used in a way that would conflict
Expand Down Expand Up @@ -240,6 +246,7 @@ impl<'w> UnsafeWorldCell<'w> {

/// Retrieves an [`UnsafeEntityCell`] that exposes read and write operations for the given `entity`.
/// Similar to the [`UnsafeWorldCell`], you are in charge of making sure that no aliasing rules are violated.
#[inline]
pub fn get_entity(self, entity: Entity) -> Option<UnsafeEntityCell<'w>> {
let location = self.entities().get(entity)?;
Some(UnsafeEntityCell::new(self, entity, location))
Expand Down Expand Up @@ -502,6 +509,7 @@ pub struct UnsafeEntityCell<'w> {
}

impl<'w> UnsafeEntityCell<'w> {
#[inline]
pub(crate) fn new(
world: UnsafeWorldCell<'w>,
entity: Entity,
Expand Down

0 comments on commit 2ec38d1

Please sign in to comment.