Skip to content

Commit

Permalink
Split Component Ticks (bevyengine#6547)
Browse files Browse the repository at this point in the history
# Objective
Fixes bevyengine#4884. `ComponentTicks` stores both added and changed ticks contiguously in the same 8 bytes. This is convenient when passing around both together, but causes half the bytes fetched from memory for the purposes of change detection to effectively go unused. This is inefficient when most queries (no filter, mutating *something*) only write out to the changed ticks.

## Solution
Split the storage for change detection ticks into two separate `Vec`s inside `Column`. Fetch only what is needed during iteration.

This also potentially also removes one blocker from autovectorization of dense queries.

EDIT: This is confirmed to enable autovectorization of dense queries in `for_each` and `par_for_each`  where possible.  Unfortunately `iter` has other blockers that prevent it.

### TODO

 - [x] Microbenchmark
 - [x] Check if this allows query iteration to autovectorize simple loops.
 - [x] Clean up all of the spurious tuples now littered throughout the API

### Open Questions

 - ~~Is `Mut::is_added` absolutely necessary? Can we not just use `Added` or `ChangeTrackers`?~~ It's optimized out if unused.
 - ~~Does the fetch of the added ticks get optimized out if not used?~~ Yes it is.

---

## Changelog
Added: `Tick`, a wrapper around a single change detection tick.
Added: `Column::get_added_ticks`
Added: `Column::get_column_ticks`
Added: `SparseSet::get_added_ticks`
Added: `SparseSet::get_column_ticks`
Changed: `Column` now stores added and changed ticks separately internally.
Changed: Most APIs returning `&UnsafeCell<ComponentTicks>` now returns `TickCells` instead, which contains two separate `&UnsafeCell<Tick>` for either component ticks.
Changed: `Query::for_each(_mut)`, `Query::par_for_each(_mut)` will now leverage autovectorization to speed up query iteration where possible.

## Migration Guide
TODO
  • Loading branch information
james7132 authored and alradish committed Jan 22, 2023
1 parent 3e134b0 commit 59a312b
Show file tree
Hide file tree
Showing 11 changed files with 385 additions and 257 deletions.
8 changes: 2 additions & 6 deletions crates/bevy_ecs/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
Archetype, ArchetypeId, Archetypes, BundleComponentStatus, ComponentStatus,
SpawnBundleStatus,
},
component::{Component, ComponentId, ComponentTicks, Components, StorageType},
component::{Component, ComponentId, Components, StorageType, Tick},
entity::{Entities, Entity, EntityLocation},
storage::{SparseSetIndex, SparseSets, Storages, Table},
};
Expand Down Expand Up @@ -394,11 +394,7 @@ impl BundleInfo {
// SAFETY: bundle_component is a valid index for this bundle
match bundle_component_status.get_status(bundle_component) {
ComponentStatus::Added => {
column.initialize(
table_row,
component_ptr,
ComponentTicks::new(change_tick),
);
column.initialize(table_row, component_ptr, Tick::new(change_tick));
}
ComponentStatus::Mutated => {
column.replace(table_row, component_ptr, change_tick);
Expand Down
95 changes: 59 additions & 36 deletions crates/bevy_ecs/src/change_detection.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
//! Types that detect when their internal data mutate.

use crate::{component::ComponentTicks, ptr::PtrMut, system::Resource};
use crate::{
component::{Tick, TickCells},
ptr::PtrMut,
system::Resource,
};
use bevy_ptr::UnsafeCellDeref;
use std::ops::{Deref, DerefMut};

/// The (arbitrarily chosen) minimum number of world tick increments between `check_tick` scans.
Expand Down Expand Up @@ -95,21 +100,21 @@ macro_rules! change_detection_impl {
#[inline]
fn is_added(&self) -> bool {
self.ticks
.component_ticks
.is_added(self.ticks.last_change_tick, self.ticks.change_tick)
.added
.is_older_than(self.ticks.last_change_tick, self.ticks.change_tick)
}

#[inline]
fn is_changed(&self) -> bool {
self.ticks
.component_ticks
.is_changed(self.ticks.last_change_tick, self.ticks.change_tick)
.changed
.is_older_than(self.ticks.last_change_tick, self.ticks.change_tick)
}

#[inline]
fn set_changed(&mut self) {
self.ticks
.component_ticks
.changed
.set_changed(self.ticks.change_tick);
}

Expand Down Expand Up @@ -224,11 +229,30 @@ macro_rules! impl_debug {
}

pub(crate) struct Ticks<'a> {
pub(crate) component_ticks: &'a mut ComponentTicks,
pub(crate) added: &'a mut Tick,
pub(crate) changed: &'a mut Tick,
pub(crate) last_change_tick: u32,
pub(crate) change_tick: u32,
}

impl<'a> Ticks<'a> {
/// # Safety
/// This should never alias the underlying ticks. All access must be unique.
#[inline]
pub(crate) unsafe fn from_tick_cells(
cells: TickCells<'a>,
last_change_tick: u32,
change_tick: u32,
) -> Self {
Self {
added: cells.added.deref_mut(),
changed: cells.changed.deref_mut(),
last_change_tick,
change_tick,
}
}
}

/// Unique mutable borrow of a [`Resource`].
///
/// See the [`Resource`] documentation for usage.
Expand Down Expand Up @@ -381,22 +405,20 @@ impl<'a> DetectChanges for MutUntyped<'a> {
#[inline]
fn is_added(&self) -> bool {
self.ticks
.component_ticks
.is_added(self.ticks.last_change_tick, self.ticks.change_tick)
.added
.is_older_than(self.ticks.last_change_tick, self.ticks.change_tick)
}

#[inline]
fn is_changed(&self) -> bool {
self.ticks
.component_ticks
.is_changed(self.ticks.last_change_tick, self.ticks.change_tick)
.changed
.is_older_than(self.ticks.last_change_tick, self.ticks.change_tick)
}

#[inline]
fn set_changed(&mut self) {
self.ticks
.component_ticks
.set_changed(self.ticks.change_tick);
self.ticks.changed.set_changed(self.ticks.change_tick);
}

#[inline]
Expand Down Expand Up @@ -429,10 +451,8 @@ mod tests {

use crate::{
self as bevy_ecs,
change_detection::{
ComponentTicks, Mut, NonSendMut, ResMut, Ticks, CHECK_TICK_THRESHOLD, MAX_CHANGE_AGE,
},
component::Component,
change_detection::{Mut, NonSendMut, ResMut, Ticks, CHECK_TICK_THRESHOLD, MAX_CHANGE_AGE},
component::{Component, ComponentTicks, Tick},
query::ChangeTrackers,
system::{IntoSystem, Query, System},
world::World,
Expand Down Expand Up @@ -514,8 +534,8 @@ mod tests {

let mut query = world.query::<ChangeTrackers<C>>();
for tracker in query.iter(&world) {
let ticks_since_insert = change_tick.wrapping_sub(tracker.component_ticks.added);
let ticks_since_change = change_tick.wrapping_sub(tracker.component_ticks.changed);
let ticks_since_insert = change_tick.wrapping_sub(tracker.component_ticks.added.tick);
let ticks_since_change = change_tick.wrapping_sub(tracker.component_ticks.changed.tick);
assert!(ticks_since_insert > MAX_CHANGE_AGE);
assert!(ticks_since_change > MAX_CHANGE_AGE);
}
Expand All @@ -524,8 +544,8 @@ mod tests {
world.check_change_ticks();

for tracker in query.iter(&world) {
let ticks_since_insert = change_tick.wrapping_sub(tracker.component_ticks.added);
let ticks_since_change = change_tick.wrapping_sub(tracker.component_ticks.changed);
let ticks_since_insert = change_tick.wrapping_sub(tracker.component_ticks.added.tick);
let ticks_since_change = change_tick.wrapping_sub(tracker.component_ticks.changed.tick);
assert!(ticks_since_insert == MAX_CHANGE_AGE);
assert!(ticks_since_change == MAX_CHANGE_AGE);
}
Expand All @@ -534,11 +554,12 @@ mod tests {
#[test]
fn mut_from_res_mut() {
let mut component_ticks = ComponentTicks {
added: 1,
changed: 2,
added: Tick::new(1),
changed: Tick::new(2),
};
let ticks = Ticks {
component_ticks: &mut component_ticks,
added: &mut component_ticks.added,
changed: &mut component_ticks.changed,
last_change_tick: 3,
change_tick: 4,
};
Expand All @@ -549,20 +570,21 @@ mod tests {
};

let into_mut: Mut<R> = res_mut.into();
assert_eq!(1, into_mut.ticks.component_ticks.added);
assert_eq!(2, into_mut.ticks.component_ticks.changed);
assert_eq!(1, into_mut.ticks.added.tick);
assert_eq!(2, into_mut.ticks.changed.tick);
assert_eq!(3, into_mut.ticks.last_change_tick);
assert_eq!(4, into_mut.ticks.change_tick);
}

#[test]
fn mut_from_non_send_mut() {
let mut component_ticks = ComponentTicks {
added: 1,
changed: 2,
added: Tick::new(1),
changed: Tick::new(2),
};
let ticks = Ticks {
component_ticks: &mut component_ticks,
added: &mut component_ticks.added,
changed: &mut component_ticks.changed,
last_change_tick: 3,
change_tick: 4,
};
Expand All @@ -573,8 +595,8 @@ mod tests {
};

let into_mut: Mut<R> = non_send_mut.into();
assert_eq!(1, into_mut.ticks.component_ticks.added);
assert_eq!(2, into_mut.ticks.component_ticks.changed);
assert_eq!(1, into_mut.ticks.added.tick);
assert_eq!(2, into_mut.ticks.changed.tick);
assert_eq!(3, into_mut.ticks.last_change_tick);
assert_eq!(4, into_mut.ticks.change_tick);
}
Expand All @@ -584,13 +606,14 @@ mod tests {
use super::*;
struct Outer(i64);

let (last_change_tick, change_tick) = (2, 3);
let mut component_ticks = ComponentTicks {
added: 1,
changed: 2,
added: Tick::new(1),
changed: Tick::new(2),
};
let (last_change_tick, change_tick) = (2, 3);
let ticks = Ticks {
component_ticks: &mut component_ticks,
added: &mut component_ticks.added,
changed: &mut component_ticks.changed,
last_change_tick,
change_tick,
};
Expand Down
118 changes: 80 additions & 38 deletions crates/bevy_ecs/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ use crate::{
system::Resource,
};
pub use bevy_ecs_macros::Component;
use bevy_ptr::OwningPtr;
use bevy_ptr::{OwningPtr, UnsafeCellDeref};
use std::cell::UnsafeCell;
use std::{
alloc::Layout,
any::{Any, TypeId},
Expand Down Expand Up @@ -517,58 +518,108 @@ impl Components {
}
}

/// Records when a component was added and when it was last mutably dereferenced (or added).
/// Used to track changes in state between system runs, e.g. components being added or accessed mutably.
#[derive(Copy, Clone, Debug)]
pub struct ComponentTicks {
pub(crate) added: u32,
pub(crate) changed: u32,
pub struct Tick {
pub(crate) tick: u32,
}

impl ComponentTicks {
impl Tick {
pub const fn new(tick: u32) -> Self {
Self { tick }
}

#[inline]
/// Returns `true` if the component was added after the system last ran.
pub fn is_added(&self, last_change_tick: u32, change_tick: u32) -> bool {
/// Returns `true` if the tick is older than the system last's run.
pub fn is_older_than(&self, last_change_tick: u32, change_tick: u32) -> bool {
// This works even with wraparound because the world tick (`change_tick`) is always "newer" than
// `last_change_tick` and `self.added`, and we scan periodically to clamp `ComponentTicks` values
// `last_change_tick` and `self.tick`, and we scan periodically to clamp `ComponentTicks` values
// so they never get older than `u32::MAX` (the difference would overflow).
//
// The clamp here ensures determinism (since scans could differ between app runs).
let ticks_since_insert = change_tick.wrapping_sub(self.added).min(MAX_CHANGE_AGE);
let ticks_since_insert = change_tick.wrapping_sub(self.tick).min(MAX_CHANGE_AGE);
let ticks_since_system = change_tick
.wrapping_sub(last_change_tick)
.min(MAX_CHANGE_AGE);

ticks_since_system > ticks_since_insert
}

pub(crate) fn check_tick(&mut self, change_tick: u32) {
let age = change_tick.wrapping_sub(self.tick);
// This comparison assumes that `age` has not overflowed `u32::MAX` before, which will be true
// so long as this check always runs before that can happen.
if age > MAX_CHANGE_AGE {
self.tick = change_tick.wrapping_sub(MAX_CHANGE_AGE);
}
}

/// Manually sets the change tick.
///
/// This is normally done automatically via the [`DerefMut`](std::ops::DerefMut) implementation
/// on [`Mut<T>`](crate::change_detection::Mut), [`ResMut<T>`](crate::change_detection::ResMut), etc.
/// However, components and resources that make use of interior mutability might require manual updates.
///
/// # Example
/// ```rust,no_run
/// # use bevy_ecs::{world::World, component::ComponentTicks};
/// let world: World = unimplemented!();
/// let component_ticks: ComponentTicks = unimplemented!();
///
/// component_ticks.set_changed(world.read_change_tick());
/// ```
#[inline]
pub fn set_changed(&mut self, change_tick: u32) {
self.tick = change_tick;
}
}

/// Wrapper around [`Tick`]s for a single component
#[derive(Copy, Clone, Debug)]
pub struct TickCells<'a> {
pub added: &'a UnsafeCell<Tick>,
pub changed: &'a UnsafeCell<Tick>,
}

impl<'a> TickCells<'a> {
/// # Safety
/// All cells contained within must uphold the safety invariants of [`UnsafeCellDeref::read`].
#[inline]
pub(crate) unsafe fn read(&self) -> ComponentTicks {
ComponentTicks {
added: self.added.read(),
changed: self.changed.read(),
}
}
}

/// Records when a component was added and when it was last mutably dereferenced (or added).
#[derive(Copy, Clone, Debug)]
pub struct ComponentTicks {
pub(crate) added: Tick,
pub(crate) changed: Tick,
}

impl ComponentTicks {
#[inline]
/// Returns `true` if the component was added after the system last ran.
pub fn is_added(&self, last_change_tick: u32, change_tick: u32) -> bool {
self.added.is_older_than(last_change_tick, change_tick)
}

#[inline]
/// Returns `true` if the component was added or mutably dereferenced after the system last ran.
pub fn is_changed(&self, last_change_tick: u32, change_tick: u32) -> bool {
// This works even with wraparound because the world tick (`change_tick`) is always "newer" than
// `last_change_tick` and `self.changed`, and we scan periodically to clamp `ComponentTicks` values
// so they never get older than `u32::MAX` (the difference would overflow).
//
// The clamp here ensures determinism (since scans could differ between app runs).
let ticks_since_change = change_tick.wrapping_sub(self.changed).min(MAX_CHANGE_AGE);
let ticks_since_system = change_tick
.wrapping_sub(last_change_tick)
.min(MAX_CHANGE_AGE);

ticks_since_system > ticks_since_change
self.changed.is_older_than(last_change_tick, change_tick)
}

pub(crate) fn new(change_tick: u32) -> Self {
Self {
added: change_tick,
changed: change_tick,
added: Tick::new(change_tick),
changed: Tick::new(change_tick),
}
}

pub(crate) fn check_ticks(&mut self, change_tick: u32) {
check_tick(&mut self.added, change_tick);
check_tick(&mut self.changed, change_tick);
}

/// Manually sets the change tick.
///
/// This is normally done automatically via the [`DerefMut`](std::ops::DerefMut) implementation
Expand All @@ -585,15 +636,6 @@ impl ComponentTicks {
/// ```
#[inline]
pub fn set_changed(&mut self, change_tick: u32) {
self.changed = change_tick;
}
}

fn check_tick(last_change_tick: &mut u32, change_tick: u32) {
let age = change_tick.wrapping_sub(*last_change_tick);
// This comparison assumes that `age` has not overflowed `u32::MAX` before, which will be true
// so long as this check always runs before that can happen.
if age > MAX_CHANGE_AGE {
*last_change_tick = change_tick.wrapping_sub(MAX_CHANGE_AGE);
self.changed.set_changed(change_tick);
}
}
Loading

0 comments on commit 59a312b

Please sign in to comment.