From 816325f1d5dab024ab1bd02ebe40a631d4be8a43 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Mon, 10 Mar 2025 15:38:47 -0700 Subject: [PATCH 01/20] migrate SparseSet to ThinColumn still some issues to fix, indices are oob sometimes --- crates/bevy_ecs/src/storage/mod.rs | 21 + crates/bevy_ecs/src/storage/sparse_set.rs | 118 +++++- crates/bevy_ecs/src/storage/table/column.rs | 439 +++----------------- crates/bevy_ecs/src/storage/table/mod.rs | 66 ++- 4 files changed, 216 insertions(+), 428 deletions(-) diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index 2a5a5f184e649..e275c8a41451a 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -60,3 +60,24 @@ impl Storages { } } } + +struct AbortOnPanic; + +impl Drop for AbortOnPanic { + fn drop(&mut self) { + // Panicking while unwinding will force an abort. + panic!("Aborting due to allocator error"); + } +} + +/// Wraps a function in an `AbortOnPanic` guard, which will be dropped if the function unwinds, +/// causing the program to abort. This should be used whenever an operation may cause a panic +/// while an object is still in an invalid state, which could cause UB when dropped. +#[inline(always)] +pub fn abort_on_panic R, R>(f: F) -> R { + let _guard = AbortOnPanic; + let ret = f(); + // the operation was successful, so we don't drop the guard + core::mem::forget(_guard); + ret +} diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index bb79382e06a8d..f0861eb0221fe 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -2,13 +2,15 @@ use crate::{ change_detection::MaybeLocation, component::{ComponentId, ComponentInfo, ComponentTicks, Tick, TickCells}, entity::Entity, - storage::{Column, TableRow}, + storage::TableRow, }; use alloc::{boxed::Box, vec::Vec}; use bevy_ptr::{OwningPtr, Ptr}; -use core::{cell::UnsafeCell, hash::Hash, marker::PhantomData, panic::Location}; +use core::{cell::UnsafeCell, hash::Hash, marker::PhantomData, num::NonZeroUsize, panic::Location}; use nonmax::NonMaxUsize; +use super::{abort_on_panic, ThinColumn}; + type EntityIndex = u32; #[derive(Debug)] @@ -114,9 +116,8 @@ impl SparseArray { /// A sparse data structure of [`Component`](crate::component::Component)s. /// /// Designed for relatively fast insertions and deletions. -#[derive(Debug)] pub struct ComponentSparseSet { - dense: Column, + dense: ThinColumn, // Internally this only relies on the Entity index to keep track of where the component data is // stored for entities that are alive. The generation is not required, but is stored // in debug builds to validate that access is correct. @@ -132,7 +133,7 @@ impl ComponentSparseSet { /// initial `capacity`. pub(crate) fn new(component_info: &ComponentInfo, capacity: usize) -> Self { Self { - dense: Column::with_capacity(component_info, capacity), + dense: ThinColumn::with_capacity(component_info, capacity), entities: Vec::with_capacity(capacity), sparse: Default::default(), } @@ -140,7 +141,10 @@ impl ComponentSparseSet { /// Removes all of the values stored within. pub(crate) fn clear(&mut self) { - self.dense.clear(); + // SAFETY: `self.len()` is the length of the column + unsafe { + self.dense.clear(self.len()); + } self.entities.clear(); self.sparse.clear(); } @@ -148,13 +152,18 @@ impl ComponentSparseSet { /// Returns the number of component values in the sparse set. #[inline] pub fn len(&self) -> usize { - self.dense.len() + self.entities.len() } /// Returns `true` if the sparse set contains no component values. #[inline] pub fn is_empty(&self) -> bool { - self.dense.len() == 0 + self.len() == 0 + } + + #[inline] + fn capacity(&self) -> usize { + self.entities.capacity() } /// Inserts the `entity` key and component `value` pair into this sparse @@ -175,13 +184,16 @@ impl ComponentSparseSet { assert_eq!(entity, self.entities[dense_index.as_usize()]); self.dense.replace(dense_index, value, change_tick, caller); } else { - let dense_index = self.dense.len(); - self.dense - .push(value, ComponentTicks::new(change_tick), caller); - self.sparse - .insert(entity.index(), TableRow::from_usize(dense_index)); + let dense_index = TableRow::from_usize(self.len()); + self.reserve(1); + // SAFETY: `dense_index` is the last element in the column after the call to `reserve` + unsafe { + self.dense + .initialize(dense_index, value, change_tick, caller); + } + self.sparse.insert(entity.index(), dense_index); #[cfg(debug_assertions)] - assert_eq!(self.entities.len(), dense_index); + assert_eq!(self.len(), dense_index.as_usize()); #[cfg(not(debug_assertions))] self.entities.push(entity.index()); #[cfg(debug_assertions)] @@ -308,9 +320,13 @@ impl ComponentSparseSet { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index.as_usize()]); self.entities.swap_remove(dense_index.as_usize()); - let is_last = dense_index.as_usize() == self.dense.len() - 1; + let last_index = self.len() - 1; + let is_last = dense_index.as_usize() == last_index; // SAFETY: dense_index was just removed from `sparse`, which ensures that it is valid - let (value, _, _) = unsafe { self.dense.swap_remove_and_forget_unchecked(dense_index) }; + let (value, _, _) = unsafe { + self.dense + .swap_remove_and_forget_unchecked(last_index, dense_index) + }; if !is_last { let swapped_entity = self.entities[dense_index.as_usize()]; #[cfg(not(debug_assertions))] @@ -331,10 +347,12 @@ impl ComponentSparseSet { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index.as_usize()]); self.entities.swap_remove(dense_index.as_usize()); - let is_last = dense_index.as_usize() == self.dense.len() - 1; + let last_index = self.len() - 1; + let is_last = dense_index.as_usize() == last_index; // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { - self.dense.swap_remove_unchecked(dense_index); + self.dense + .swap_remove_and_drop_unchecked(last_index, dense_index); } if !is_last { let swapped_entity = self.entities[dense_index.as_usize()]; @@ -351,7 +369,69 @@ impl ComponentSparseSet { } pub(crate) fn check_change_ticks(&mut self, change_tick: Tick) { - self.dense.check_change_ticks(change_tick); + // SAFETY: `self.len()` is equal to the length of the column + unsafe { + self.dense.check_change_ticks(self.len(), change_tick); + } + } + + /// Reserves `additional` elements worth of capacity within the table. + pub(crate) fn reserve(&mut self, additional: usize) { + if self.capacity() - self.len() < additional { + let column_cap = self.capacity(); + self.entities.reserve(additional); + + // use entities vector capacity as driving capacity for all related allocations + let new_capacity = self.capacity(); + + if column_cap == 0 { + // SAFETY: the current capacity is 0 + unsafe { self.alloc_dense(NonZeroUsize::new_unchecked(new_capacity)) }; + } else { + // SAFETY: + // - `column_cap` is indeed the columns' capacity + unsafe { + self.realloc_dense( + NonZeroUsize::new_unchecked(column_cap), + NonZeroUsize::new_unchecked(new_capacity), + ); + }; + } + } + } + + /// Allocate memory for the columns in the [`Table`] + /// + /// The current capacity of the columns should be 0, if it's not 0, then the previous data will be overwritten and leaked. + fn alloc_dense(&mut self, new_capacity: NonZeroUsize) { + // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. + // To avoid this, we use `AbortOnPanic`. If the allocation triggered a panic, the `AbortOnPanic`'s Drop impl will be + // called, and abort the program. + abort_on_panic(|| { + self.dense.alloc(new_capacity); + }); + } + + /// Reallocate memory for the columns in the [`Table`] + /// + /// # Safety + /// - `current_column_capacity` is indeed the capacity of the columns + unsafe fn realloc_dense( + &mut self, + current_column_capacity: NonZeroUsize, + new_capacity: NonZeroUsize, + ) { + // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. + // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the `AbortOnPanic`'s Drop impl will be + // called, and abort the program. + + // SAFETY: + // - There's no overflow + // - `current_capacity` is indeed the capacity - safety requirement + // - current capacity > 0 + abort_on_panic(|| unsafe { + self.dense.realloc(current_column_capacity, new_capacity); + }); } } diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index d4690d264cb32..cac2639d9b35b 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -1,11 +1,8 @@ use super::*; use crate::{ change_detection::MaybeLocation, - component::TickCells, storage::{blob_array::BlobArray, thin_array_ptr::ThinArrayPtr}, }; -use alloc::vec::Vec; -use bevy_ptr::PtrMut; use core::panic::Location; /// Very similar to a normal [`Column`], but with the capacities and lengths cut out for performance reasons. @@ -83,6 +80,10 @@ impl ThinColumn { /// Swap-remove and forget the removed element. /// + /// It's the caller's responsibility to ensure that the returned value is dropped or used. + /// Failure to do so may result in resources not being released (i.e. file handles not + /// being release, memory leaks, etc) + /// /// # Safety /// - `last_element_index` must be the index of the last element—stored in the highest place in memory. /// - `row.as_usize()` <= `last_element_index` @@ -91,17 +92,25 @@ impl ThinColumn { &mut self, last_element_index: usize, row: TableRow, - ) { - let _ = self + ) -> (OwningPtr, ComponentTicks, MaybeLocation) { + let data = self .data .swap_remove_unchecked(row.as_usize(), last_element_index); - self.added_ticks - .swap_remove_unchecked(row.as_usize(), last_element_index); - self.changed_ticks - .swap_remove_unchecked(row.as_usize(), last_element_index); - self.changed_by - .as_mut() - .map(|changed_by| changed_by.swap_remove_unchecked(row.as_usize(), last_element_index)); + let added = self + .added_ticks + .swap_remove_unchecked(row.as_usize(), last_element_index) + .read(); + let changed = self + .changed_ticks + .swap_remove_unchecked(row.as_usize(), last_element_index) + .read(); + let changed_by = self.changed_by.as_mut().map(|changed_by| { + changed_by + .swap_remove_unchecked(row.as_usize(), last_element_index) + .read() + }); + + (data, ComponentTicks { added, changed }, changed_by) } /// Call [`realloc`](std::alloc::realloc) to expand / shrink the memory allocation for this [`ThinColumn`] @@ -295,406 +304,88 @@ impl ThinColumn { self.data.drop_last_element(last_element_index); } - /// Get a slice to the data stored in this [`ThinColumn`]. - /// - /// # Safety - /// - `T` must match the type of data that's stored in this [`ThinColumn`] - /// - `len` must match the actual length of this column (number of elements stored) - pub unsafe fn get_data_slice(&self, len: usize) -> &[UnsafeCell] { - self.data.get_sub_slice(len) - } - - /// Get a slice to the added [`ticks`](Tick) in this [`ThinColumn`]. + /// Returns the component data in this column at the given row /// /// # Safety - /// - `len` must match the actual length of this column (number of elements stored) - pub unsafe fn get_added_ticks_slice(&self, len: usize) -> &[UnsafeCell] { - self.added_ticks.as_slice(len) - } - - /// Get a slice to the changed [`ticks`](Tick) in this [`ThinColumn`]. - /// - /// # Safety - /// - `len` must match the actual length of this column (number of elements stored) - pub unsafe fn get_changed_ticks_slice(&self, len: usize) -> &[UnsafeCell] { - self.changed_ticks.as_slice(len) + /// - `row` must be within bounds (`row` < len) + pub unsafe fn get_data_unchecked(&self, row: TableRow) -> Ptr { + self.data.get_unchecked(row.as_usize()) } - /// Get a slice to the calling locations that last changed each value in this [`ThinColumn`] + /// Returns the added tick in this column at the given row /// /// # Safety - /// - `len` must match the actual length of this column (number of elements stored) - pub unsafe fn get_changed_by_slice( - &self, - len: usize, - ) -> MaybeLocation<&[UnsafeCell<&'static Location<'static>>]> { - self.changed_by - .as_ref() - .map(|changed_by| changed_by.as_slice(len)) - } -} - -/// A type-erased contiguous container for data of a homogeneous type. -/// -/// Conceptually, a [`Column`] is very similar to a type-erased `Vec`. -/// It also stores the change detection ticks for its components, kept in two separate -/// contiguous buffers internally. An element shares its data across these buffers by using the -/// same index (i.e. the entity at row 3 has it's data at index 3 and its change detection ticks at index 3). -/// -/// Like many other low-level storage types, [`Column`] has a limited and highly unsafe -/// interface. It's highly advised to use higher level types and their safe abstractions -/// instead of working directly with [`Column`]. -#[derive(Debug)] -pub struct Column { - pub(super) data: BlobVec, - pub(super) added_ticks: Vec>, - pub(super) changed_ticks: Vec>, - changed_by: MaybeLocation>>>, -} - -impl Column { - /// Constructs a new [`Column`], configured with a component's layout and an initial `capacity`. - #[inline] - pub(crate) fn with_capacity(component_info: &ComponentInfo, capacity: usize) -> Self { - Column { - // SAFETY: component_info.drop() is valid for the types that will be inserted. - data: unsafe { BlobVec::new(component_info.layout(), component_info.drop(), capacity) }, - added_ticks: Vec::with_capacity(capacity), - changed_ticks: Vec::with_capacity(capacity), - changed_by: MaybeLocation::new_with(|| Vec::with_capacity(capacity)), - } - } - - /// Fetches the [`Layout`] for the underlying type. - #[inline] - pub fn item_layout(&self) -> Layout { - self.data.layout() + /// - `row` must be within bounds (`row` < len) + pub unsafe fn get_added_tick_unchecked(&self, row: TableRow) -> &UnsafeCell { + self.added_ticks.get_unchecked(row.as_usize()) } - /// Writes component data to the column at given row. - /// Assumes the slot is initialized, calls drop. + /// Returns the changed tick in this column at the given row /// /// # Safety - /// Assumes data has already been allocated for the given row. - #[inline] - pub(crate) unsafe fn replace( - &mut self, - row: TableRow, - data: OwningPtr<'_>, - change_tick: Tick, - caller: MaybeLocation, - ) { - debug_assert!(row.as_usize() < self.len()); - self.data.replace_unchecked(row.as_usize(), data); - *self - .changed_ticks - .get_unchecked_mut(row.as_usize()) - .get_mut() = change_tick; - self.changed_by - .as_mut() - .map(|changed_by| changed_by.get_unchecked_mut(row.as_usize()).get_mut()) - .assign(caller); - } - - /// Gets the current number of elements stored in the column. - #[inline] - pub fn len(&self) -> usize { - self.data.len() - } - - /// Checks if the column is empty. Returns `true` if there are no elements, `false` otherwise. - #[inline] - pub fn is_empty(&self) -> bool { - self.data.is_empty() + /// - `row` must be within bounds (`row` < len) + pub unsafe fn get_changed_tick_unchecked(&self, row: TableRow) -> &UnsafeCell { + self.changed_ticks.get_unchecked(row.as_usize()) } - /// Removes an element from the [`Column`]. - /// - /// - The value will be dropped if it implements [`Drop`]. - /// - This does not preserve ordering, but is O(1). - /// - This does not do any bounds checking. - /// - The element is replaced with the last element in the [`Column`]. + /// Returns the component ticks in this column at the given row /// /// # Safety - /// `row` must be within the range `[0, self.len())`. - #[inline] - pub(crate) unsafe fn swap_remove_unchecked(&mut self, row: TableRow) { - self.data.swap_remove_and_drop_unchecked(row.as_usize()); - self.added_ticks.swap_remove(row.as_usize()); - self.changed_ticks.swap_remove(row.as_usize()); - self.changed_by - .as_mut() - .map(|changed_by| changed_by.swap_remove(row.as_usize())); + /// - `row` must be within bounds (`row` < len) + pub unsafe fn get_ticks_unchecked(&self, row: TableRow) -> ComponentTicks { + let added = self.get_added_tick_unchecked(row).read(); + let changed = self.get_changed_tick_unchecked(row).read(); + ComponentTicks { added, changed } } - /// Removes an element from the [`Column`] and returns it and its change detection ticks. - /// This does not preserve ordering, but is O(1) and does not do any bounds checking. - /// - /// The element is replaced with the last element in the [`Column`]. - /// - /// It's the caller's responsibility to ensure that the removed value is dropped or used. - /// Failure to do so may result in resources not being released (i.e. files handles not being - /// released, memory leaks, etc.) + /// Returns the component data in this column at the given row /// /// # Safety - /// `row` must be within the range `[0, self.len())`. - #[inline] - #[must_use = "The returned pointer should be used to dropped the removed component"] - pub(crate) unsafe fn swap_remove_and_forget_unchecked( - &mut self, + /// - `row` must be within bounds (`row` < len) + pub unsafe fn get_changed_by_unchecked( + &self, row: TableRow, - ) -> (OwningPtr<'_>, ComponentTicks, MaybeLocation) { - let data = self.data.swap_remove_and_forget_unchecked(row.as_usize()); - let added = self.added_ticks.swap_remove(row.as_usize()).into_inner(); - let changed = self.changed_ticks.swap_remove(row.as_usize()).into_inner(); - let caller = self - .changed_by - .as_mut() - .map(|changed_by| changed_by.swap_remove(row.as_usize()).into_inner()); - (data, ComponentTicks { added, changed }, caller) - } - - /// Pushes a new value onto the end of the [`Column`]. - /// - /// # Safety - /// `ptr` must point to valid data of this column's component type - pub(crate) unsafe fn push( - &mut self, - ptr: OwningPtr<'_>, - ticks: ComponentTicks, - caller: MaybeLocation, - ) { - self.data.push(ptr); - self.added_ticks.push(UnsafeCell::new(ticks.added)); - self.changed_ticks.push(UnsafeCell::new(ticks.changed)); + ) -> MaybeLocation<&UnsafeCell<&'static Location<'static>>> { self.changed_by - .as_mut() - .zip(caller) - .map(|(changed_by, caller)| changed_by.push(UnsafeCell::new(caller))); - } - - /// Fetches the data pointer to the first element of the [`Column`]. - /// - /// The pointer is type erased, so using this function to fetch anything - /// other than the first element will require computing the offset using - /// [`Column::item_layout`]. - #[inline] - pub fn get_data_ptr(&self) -> Ptr<'_> { - self.data.get_ptr() - } - - /// Fetches the slice to the [`Column`]'s data cast to a given type. - /// - /// Note: The values stored within are [`UnsafeCell`]. - /// Users of this API must ensure that accesses to each individual element - /// adhere to the safety invariants of [`UnsafeCell`]. - /// - /// # Safety - /// The type `T` must be the type of the items in this column. - pub unsafe fn get_data_slice(&self) -> &[UnsafeCell] { - self.data.get_slice() - } - - /// Fetches the slice to the [`Column`]'s "added" change detection ticks. - /// - /// Note: The values stored within are [`UnsafeCell`]. - /// Users of this API must ensure that accesses to each individual element - /// adhere to the safety invariants of [`UnsafeCell`]. - #[inline] - pub fn get_added_ticks_slice(&self) -> &[UnsafeCell] { - &self.added_ticks - } - - /// Fetches the slice to the [`Column`]'s "changed" change detection ticks. - /// - /// Note: The values stored within are [`UnsafeCell`]. - /// Users of this API must ensure that accesses to each individual element - /// adhere to the safety invariants of [`UnsafeCell`]. - #[inline] - pub fn get_changed_ticks_slice(&self) -> &[UnsafeCell] { - &self.changed_ticks - } - - /// Fetches a reference to the data and change detection ticks at `row`. - /// - /// Returns `None` if `row` is out of bounds. - #[inline] - pub fn get(&self, row: TableRow) -> Option<(Ptr<'_>, TickCells<'_>)> { - (row.as_usize() < self.data.len()) - // SAFETY: The row is length checked before fetching the pointer. This is being - // accessed through a read-only reference to the column. - .then(|| unsafe { - ( - self.data.get_unchecked(row.as_usize()), - TickCells { - added: self.added_ticks.get_unchecked(row.as_usize()), - changed: self.changed_ticks.get_unchecked(row.as_usize()), - }, - ) - }) - } - - /// Fetches a read-only reference to the data at `row`. - /// - /// Returns `None` if `row` is out of bounds. - #[inline] - pub fn get_data(&self, row: TableRow) -> Option> { - (row.as_usize() < self.data.len()).then(|| { - // SAFETY: The row is length checked before fetching the pointer. This is being - // accessed through a read-only reference to the column. - unsafe { self.data.get_unchecked(row.as_usize()) } - }) + .as_ref() + .map(|changed_by| changed_by.get_unchecked(row.as_usize())) } - /// Fetches a read-only reference to the data at `row`. Unlike [`Column::get`] this does not - /// do any bounds checking. + /// Get a slice to the data stored in this [`ThinColumn`]. /// /// # Safety - /// - `row` must be within the range `[0, self.len())`. - /// - no other mutable reference to the data of the same row can exist at the same time - #[inline] - pub unsafe fn get_data_unchecked(&self, row: TableRow) -> Ptr<'_> { - debug_assert!(row.as_usize() < self.data.len()); - self.data.get_unchecked(row.as_usize()) - } - - /// Fetches a mutable reference to the data at `row`. - /// - /// Returns `None` if `row` is out of bounds. - #[inline] - pub fn get_data_mut(&mut self, row: TableRow) -> Option> { - (row.as_usize() < self.data.len()).then(|| { - // SAFETY: The row is length checked before fetching the pointer. This is being - // accessed through an exclusive reference to the column. - unsafe { self.data.get_unchecked_mut(row.as_usize()) } - }) - } - - /// Fetches the "added" change detection tick for the value at `row`. - /// - /// Returns `None` if `row` is out of bounds. - /// - /// Note: The values stored within are [`UnsafeCell`]. - /// Users of this API must ensure that accesses to each individual element - /// adhere to the safety invariants of [`UnsafeCell`]. - #[inline] - pub fn get_added_tick(&self, row: TableRow) -> Option<&UnsafeCell> { - self.added_ticks.get(row.as_usize()) - } - - /// Fetches the "changed" change detection tick for the value at `row`. - /// - /// Returns `None` if `row` is out of bounds. - /// - /// Note: The values stored within are [`UnsafeCell`]. - /// Users of this API must ensure that accesses to each individual element - /// adhere to the safety invariants of [`UnsafeCell`]. - #[inline] - pub fn get_changed_tick(&self, row: TableRow) -> Option<&UnsafeCell> { - self.changed_ticks.get(row.as_usize()) - } - - /// Fetches the change detection ticks for the value at `row`. - /// - /// Returns `None` if `row` is out of bounds. - #[inline] - pub fn get_ticks(&self, row: TableRow) -> Option { - if row.as_usize() < self.data.len() { - // SAFETY: The size of the column has already been checked. - Some(unsafe { self.get_ticks_unchecked(row) }) - } else { - None - } + /// - `T` must match the type of data that's stored in this [`ThinColumn`] + /// - `len` must match the actual length of this column (number of elements stored) + pub unsafe fn get_data_slice(&self, len: usize) -> &[UnsafeCell] { + self.data.get_sub_slice(len) } - /// Fetches the "added" change detection tick for the value at `row`. Unlike [`Column::get_added_tick`] - /// this function does not do any bounds checking. + /// Get a slice to the added [`ticks`](Tick) in this [`ThinColumn`]. /// /// # Safety - /// `row` must be within the range `[0, self.len())`. - #[inline] - pub unsafe fn get_added_tick_unchecked(&self, row: TableRow) -> &UnsafeCell { - debug_assert!(row.as_usize() < self.added_ticks.len()); - self.added_ticks.get_unchecked(row.as_usize()) + /// - `len` must match the actual length of this column (number of elements stored) + pub unsafe fn get_added_ticks_slice(&self, len: usize) -> &[UnsafeCell] { + self.added_ticks.as_slice(len) } - /// Fetches the "changed" change detection tick for the value at `row`. Unlike [`Column::get_changed_tick`] - /// this function does not do any bounds checking. + /// Get a slice to the changed [`ticks`](Tick) in this [`ThinColumn`]. /// /// # Safety - /// `row` must be within the range `[0, self.len())`. - #[inline] - pub unsafe fn get_changed_tick_unchecked(&self, row: TableRow) -> &UnsafeCell { - debug_assert!(row.as_usize() < self.changed_ticks.len()); - self.changed_ticks.get_unchecked(row.as_usize()) + /// - `len` must match the actual length of this column (number of elements stored) + pub unsafe fn get_changed_ticks_slice(&self, len: usize) -> &[UnsafeCell] { + self.changed_ticks.as_slice(len) } - /// Fetches the change detection ticks for the value at `row`. Unlike [`Column::get_ticks`] - /// this function does not do any bounds checking. + /// Get a slice to the calling locations that last changed each value in this [`ThinColumn`] /// /// # Safety - /// `row` must be within the range `[0, self.len())`. - #[inline] - pub unsafe fn get_ticks_unchecked(&self, row: TableRow) -> ComponentTicks { - debug_assert!(row.as_usize() < self.added_ticks.len()); - debug_assert!(row.as_usize() < self.changed_ticks.len()); - ComponentTicks { - added: self.added_ticks.get_unchecked(row.as_usize()).read(), - changed: self.changed_ticks.get_unchecked(row.as_usize()).read(), - } - } - - /// Clears the column, removing all values. - /// - /// Note that this function has no effect on the allocated capacity of the [`Column`]> - pub fn clear(&mut self) { - self.data.clear(); - self.added_ticks.clear(); - self.changed_ticks.clear(); - self.changed_by.as_mut().map(Vec::clear); - } - - #[inline] - pub(crate) fn check_change_ticks(&mut self, change_tick: Tick) { - for component_ticks in &mut self.added_ticks { - component_ticks.get_mut().check_tick(change_tick); - } - for component_ticks in &mut self.changed_ticks { - component_ticks.get_mut().check_tick(change_tick); - } - } - - /// Fetches the calling location that last changed the value at `row`. - /// - /// Returns `None` if `row` is out of bounds. - /// - /// Note: The values stored within are [`UnsafeCell`]. - /// Users of this API must ensure that accesses to each individual element - /// adhere to the safety invariants of [`UnsafeCell`]. - #[inline] - pub fn get_changed_by( + /// - `len` must match the actual length of this column (number of elements stored) + pub unsafe fn get_changed_by_slice( &self, - row: TableRow, - ) -> MaybeLocation>>> { + len: usize, + ) -> MaybeLocation<&[UnsafeCell<&'static Location<'static>>]> { self.changed_by .as_ref() - .map(|changed_by| changed_by.get(row.as_usize())) - } - - /// Fetches the calling location that last changed the value at `row`. - /// - /// Unlike [`Column::get_changed_by`] this function does not do any bounds checking. - /// - /// # Safety - /// `row` must be within the range `[0, self.len())`. - #[inline] - pub unsafe fn get_changed_by_unchecked( - &self, - row: TableRow, - ) -> MaybeLocation<&UnsafeCell<&'static Location<'static>>> { - self.changed_by.as_ref().map(|changed_by| { - debug_assert!(row.as_usize() < changed_by.len()); - changed_by.get_unchecked(row.as_usize()) - }) + .map(|changed_by| changed_by.as_slice(len)) } } diff --git a/crates/bevy_ecs/src/storage/table/mod.rs b/crates/bevy_ecs/src/storage/table/mod.rs index 159a716c4ed05..8fbedbed868a6 100644 --- a/crates/bevy_ecs/src/storage/table/mod.rs +++ b/crates/bevy_ecs/src/storage/table/mod.rs @@ -3,19 +3,20 @@ use crate::{ component::{ComponentId, ComponentInfo, ComponentTicks, Components, Tick}, entity::Entity, query::DebugCheckedUnwrap, - storage::{blob_vec::BlobVec, ImmutableSparseSet, SparseSet}, + storage::{ImmutableSparseSet, SparseSet}, }; use alloc::{boxed::Box, vec, vec::Vec}; use bevy_platform_support::collections::HashMap; use bevy_ptr::{OwningPtr, Ptr, UnsafeCellDeref}; pub use column::*; use core::{ - alloc::Layout, cell::UnsafeCell, num::NonZeroUsize, ops::{Index, IndexMut}, panic::Location, }; + +use super::abort_on_panic; mod column; /// An opaque unique ID for a [`Table`] within a [`World`]. @@ -196,15 +197,6 @@ pub struct Table { entities: Vec, } -struct AbortOnPanic; - -impl Drop for AbortOnPanic { - fn drop(&mut self) { - // Panicking while unwinding will force an abort. - panic!("Aborting due to allocator error"); - } -} - impl Table { /// Fetches a read-only slice of the entities stored within the [`Table`]. #[inline] @@ -527,13 +519,13 @@ impl Table { /// The current capacity of the columns should be 0, if it's not 0, then the previous data will be overwritten and leaked. fn alloc_columns(&mut self, new_capacity: NonZeroUsize) { // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. - // To avoid this, we use `AbortOnPanic`. If the allocation triggered a panic, the `AbortOnPanic`'s Drop impl will be - // called, and abort the program. - let _guard = AbortOnPanic; - for col in self.columns.values_mut() { - col.alloc(new_capacity); - } - core::mem::forget(_guard); // The allocation was successful, so we don't drop the guard. + // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and + // abort the program. + abort_on_panic(|| { + for col in self.columns.values_mut() { + col.alloc(new_capacity); + } + }); } /// Reallocate memory for the columns in the [`Table`] @@ -546,18 +538,18 @@ impl Table { new_capacity: NonZeroUsize, ) { // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. - // To avoid this, we use `AbortOnPanic`. If the allocation triggered a panic, the `AbortOnPanic`'s Drop impl will be - // called, and abort the program. - let _guard = AbortOnPanic; + // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and + // abort the program. // SAFETY: // - There's no overflow // - `current_capacity` is indeed the capacity - safety requirement // - current capacity > 0 - for col in self.columns.values_mut() { - col.realloc(current_column_capacity, new_capacity); - } - core::mem::forget(_guard); // The allocation was successful, so we don't drop the guard. + abort_on_panic(|| unsafe { + for col in self.columns.values_mut() { + col.realloc(current_column_capacity, new_capacity); + } + }); } /// Allocates space for a new entity @@ -568,17 +560,21 @@ impl Table { self.reserve(1); let len = self.entity_count(); self.entities.push(entity); + for col in self.columns.values_mut() { - col.added_ticks - .initialize_unchecked(len, UnsafeCell::new(Tick::new(0))); - col.changed_ticks - .initialize_unchecked(len, UnsafeCell::new(Tick::new(0))); - col.changed_by - .as_mut() - .zip(MaybeLocation::caller()) - .map(|(changed_by, caller)| { - changed_by.initialize_unchecked(len, UnsafeCell::new(caller)); - }); + //SAFETY: `entity_count` is equal to the length of each column before insertion + unsafe { + col.added_ticks + .initialize_unchecked(len, UnsafeCell::new(Tick::new(0))); + col.changed_ticks + .initialize_unchecked(len, UnsafeCell::new(Tick::new(0))); + col.changed_by + .as_mut() + .zip(MaybeLocation::caller()) + .map(|(changed_by, caller)| { + changed_by.initialize_unchecked(len, UnsafeCell::new(caller)); + }); + } } TableRow::from_usize(len) } From bc7f23b82c211b160a559de20b3d1a0290767130 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Mon, 10 Mar 2025 16:36:15 -0700 Subject: [PATCH 02/20] tests fixed! --- crates/bevy_ecs/src/storage/sparse_set.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index f0861eb0221fe..6fdd29b3a6e87 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -6,7 +6,9 @@ use crate::{ }; use alloc::{boxed::Box, vec::Vec}; use bevy_ptr::{OwningPtr, Ptr}; -use core::{cell::UnsafeCell, hash::Hash, marker::PhantomData, num::NonZeroUsize, panic::Location}; +use core::{ + cell::UnsafeCell, hash::Hash, marker::PhantomData, mem, num::NonZeroUsize, panic::Location, +}; use nonmax::NonMaxUsize; use super::{abort_on_panic, ThinColumn}; @@ -319,9 +321,9 @@ impl ComponentSparseSet { self.sparse.remove(entity.index()).map(|dense_index| { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index.as_usize()]); - self.entities.swap_remove(dense_index.as_usize()); let last_index = self.len() - 1; let is_last = dense_index.as_usize() == last_index; + self.entities.swap_remove(dense_index.as_usize()); // SAFETY: dense_index was just removed from `sparse`, which ensures that it is valid let (value, _, _) = unsafe { self.dense @@ -346,9 +348,9 @@ impl ComponentSparseSet { if let Some(dense_index) = self.sparse.remove(entity.index()) { #[cfg(debug_assertions)] assert_eq!(entity, self.entities[dense_index.as_usize()]); - self.entities.swap_remove(dense_index.as_usize()); let last_index = self.len() - 1; let is_last = dense_index.as_usize() == last_index; + self.entities.swap_remove(dense_index.as_usize()); // SAFETY: if the sparse index points to something in the dense vec, it exists unsafe { self.dense @@ -435,6 +437,17 @@ impl ComponentSparseSet { } } +impl Drop for ComponentSparseSet { + fn drop(&mut self) { + // SAFETY: + // - self.capacity() is the capacity of the column + // - self.len() is the length of the column + unsafe { + self.dense.drop(self.capacity(), self.len()); + } + } +} + /// A data structure that blends dense and sparse storage /// /// `I` is the type of the indices, while `V` is the type of data stored in the dense storage. From 10d0f18c4635e6d9aef8dce40b0d1c42317e68d6 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Mon, 10 Mar 2025 16:45:42 -0700 Subject: [PATCH 03/20] fix docs --- crates/bevy_ecs/src/storage/sparse_set.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 6fdd29b3a6e87..bce79814c2897 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -407,8 +407,8 @@ impl ComponentSparseSet { /// The current capacity of the columns should be 0, if it's not 0, then the previous data will be overwritten and leaked. fn alloc_dense(&mut self, new_capacity: NonZeroUsize) { // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. - // To avoid this, we use `AbortOnPanic`. If the allocation triggered a panic, the `AbortOnPanic`'s Drop impl will be - // called, and abort the program. + // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and + // abort the program. abort_on_panic(|| { self.dense.alloc(new_capacity); }); @@ -424,8 +424,8 @@ impl ComponentSparseSet { new_capacity: NonZeroUsize, ) { // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. - // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the `AbortOnPanic`'s Drop impl will be - // called, and abort the program. + // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and + // abort the program. // SAFETY: // - There's no overflow From 7f5045028765af038140fa43f999b773efde6915 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Mon, 10 Mar 2025 16:54:05 -0700 Subject: [PATCH 04/20] fix imports --- crates/bevy_ecs/src/storage/sparse_set.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index bce79814c2897..c78c0b0155e10 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -6,9 +6,7 @@ use crate::{ }; use alloc::{boxed::Box, vec::Vec}; use bevy_ptr::{OwningPtr, Ptr}; -use core::{ - cell::UnsafeCell, hash::Hash, marker::PhantomData, mem, num::NonZeroUsize, panic::Location, -}; +use core::{cell::UnsafeCell, hash::Hash, marker::PhantomData, num::NonZeroUsize, panic::Location}; use nonmax::NonMaxUsize; use super::{abort_on_panic, ThinColumn}; From dbfcc1648aaae40b85708233495b89987213ce19 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Mon, 10 Mar 2025 19:23:46 -0700 Subject: [PATCH 05/20] simplify `Table` --- crates/bevy_ecs/src/storage/table/mod.rs | 22 +++++----------------- 1 file changed, 5 insertions(+), 17 deletions(-) diff --git a/crates/bevy_ecs/src/storage/table/mod.rs b/crates/bevy_ecs/src/storage/table/mod.rs index 800d8b7a24f37..d1b9a50bea99c 100644 --- a/crates/bevy_ecs/src/storage/table/mod.rs +++ b/crates/bevy_ecs/src/storage/table/mod.rs @@ -402,8 +402,7 @@ impl Table { // SAFETY: `row.as_usize()` < `len` unsafe { self.get_column(component_id)? - .changed_ticks - .get_unchecked(row.as_usize()) + .get_changed_tick_unchecked(row) }, ) } @@ -416,11 +415,7 @@ impl Table { ) -> Option<&UnsafeCell> { (row.as_usize() < self.entity_count()).then_some( // SAFETY: `row.as_usize()` < `len` - unsafe { - self.get_column(component_id)? - .added_ticks - .get_unchecked(row.as_usize()) - }, + unsafe { self.get_column(component_id)?.get_added_tick_unchecked(row) }, ) } @@ -433,12 +428,7 @@ impl Table { MaybeLocation::new_with_flattened(|| { (row.as_usize() < self.entity_count()).then_some( // SAFETY: `row.as_usize()` < `len` - unsafe { - self.get_column(component_id)? - .changed_by - .as_ref() - .map(|changed_by| changed_by.get_unchecked(row.as_usize())) - }, + unsafe { self.get_column(component_id)?.get_changed_by_unchecked(row) }, ) }) } @@ -452,10 +442,8 @@ impl Table { component_id: ComponentId, row: TableRow, ) -> Option { - self.get_column(component_id).map(|col| ComponentTicks { - added: col.added_ticks.get_unchecked(row.as_usize()).read(), - changed: col.changed_ticks.get_unchecked(row.as_usize()).read(), - }) + self.get_column(component_id) + .map(|col| col.get_ticks_unchecked(row)) } /// Fetches a read-only reference to the [`ThinColumn`] for a given [`Component`] within the table. From 7ecd79c98619573f9bb431df2650a1af811eae3d Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Mon, 10 Mar 2025 19:57:44 -0700 Subject: [PATCH 06/20] fix docs --- crates/bevy_ecs/src/storage/table/column.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index cac2639d9b35b..31318824051d6 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -338,7 +338,7 @@ impl ThinColumn { ComponentTicks { added, changed } } - /// Returns the component data in this column at the given row + /// Returns the calling location that last modified the given row in this column /// /// # Safety /// - `row` must be within bounds (`row` < len) From f595f0cbfe96051be083b95d30e6721baa8cc3f7 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Mon, 10 Mar 2025 20:20:49 -0700 Subject: [PATCH 07/20] inline methods --- crates/bevy_ecs/src/storage/table/column.rs | 9 +++++++++ crates/bevy_ecs/src/storage/table/mod.rs | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index 31318824051d6..9ac85f53c60d0 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -308,6 +308,7 @@ impl ThinColumn { /// /// # Safety /// - `row` must be within bounds (`row` < len) + #[inline] pub unsafe fn get_data_unchecked(&self, row: TableRow) -> Ptr { self.data.get_unchecked(row.as_usize()) } @@ -316,6 +317,7 @@ impl ThinColumn { /// /// # Safety /// - `row` must be within bounds (`row` < len) + #[inline] pub unsafe fn get_added_tick_unchecked(&self, row: TableRow) -> &UnsafeCell { self.added_ticks.get_unchecked(row.as_usize()) } @@ -324,6 +326,7 @@ impl ThinColumn { /// /// # Safety /// - `row` must be within bounds (`row` < len) + #[inline] pub unsafe fn get_changed_tick_unchecked(&self, row: TableRow) -> &UnsafeCell { self.changed_ticks.get_unchecked(row.as_usize()) } @@ -332,6 +335,7 @@ impl ThinColumn { /// /// # Safety /// - `row` must be within bounds (`row` < len) + #[inline] pub unsafe fn get_ticks_unchecked(&self, row: TableRow) -> ComponentTicks { let added = self.get_added_tick_unchecked(row).read(); let changed = self.get_changed_tick_unchecked(row).read(); @@ -342,6 +346,7 @@ impl ThinColumn { /// /// # Safety /// - `row` must be within bounds (`row` < len) + #[inline] pub unsafe fn get_changed_by_unchecked( &self, row: TableRow, @@ -356,6 +361,7 @@ impl ThinColumn { /// # Safety /// - `T` must match the type of data that's stored in this [`ThinColumn`] /// - `len` must match the actual length of this column (number of elements stored) + #[inline] pub unsafe fn get_data_slice(&self, len: usize) -> &[UnsafeCell] { self.data.get_sub_slice(len) } @@ -364,6 +370,7 @@ impl ThinColumn { /// /// # Safety /// - `len` must match the actual length of this column (number of elements stored) + #[inline] pub unsafe fn get_added_ticks_slice(&self, len: usize) -> &[UnsafeCell] { self.added_ticks.as_slice(len) } @@ -372,6 +379,7 @@ impl ThinColumn { /// /// # Safety /// - `len` must match the actual length of this column (number of elements stored) + #[inline] pub unsafe fn get_changed_ticks_slice(&self, len: usize) -> &[UnsafeCell] { self.changed_ticks.as_slice(len) } @@ -380,6 +388,7 @@ impl ThinColumn { /// /// # Safety /// - `len` must match the actual length of this column (number of elements stored) + #[inline] pub unsafe fn get_changed_by_slice( &self, len: usize, diff --git a/crates/bevy_ecs/src/storage/table/mod.rs b/crates/bevy_ecs/src/storage/table/mod.rs index d1b9a50bea99c..697aa87254c07 100644 --- a/crates/bevy_ecs/src/storage/table/mod.rs +++ b/crates/bevy_ecs/src/storage/table/mod.rs @@ -658,7 +658,7 @@ impl Table { row: TableRow, ) -> Option> { self.get_column(component_id) - .map(|col| col.data.get_unchecked(row.as_usize())) + .map(|col| col.get_data_unchecked(row)) } } From b0ea954e1ca1d502fce4e3e901cced22e9cea6d1 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Wed, 12 Mar 2025 13:06:50 -0700 Subject: [PATCH 08/20] Update crates/bevy_ecs/src/storage/sparse_set.rs Co-authored-by: SpecificProtagonist --- crates/bevy_ecs/src/storage/sparse_set.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index c78c0b0155e10..ba25926424c81 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -117,6 +117,7 @@ impl SparseArray { /// /// Designed for relatively fast insertions and deletions. pub struct ComponentSparseSet { + /// SAFETY: Equal in length & capacity to `self.entities` dense: ThinColumn, // Internally this only relies on the Entity index to keep track of where the component data is // stored for entities that are alive. The generation is not required, but is stored From 39e1ea342a9cc98776a4adbd6c2b96cee9e64349 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Wed, 12 Mar 2025 13:07:11 -0700 Subject: [PATCH 09/20] Update crates/bevy_ecs/src/storage/mod.rs Co-authored-by: SpecificProtagonist --- crates/bevy_ecs/src/storage/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index e275c8a41451a..06fb846a8f796 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -70,8 +70,8 @@ impl Drop for AbortOnPanic { } } -/// Wraps a function in an `AbortOnPanic` guard, which will be dropped if the function unwinds, -/// causing the program to abort. This should be used whenever an operation may cause a panic +/// Calls a function, aborting the program if it panics. +/// This should be used whenever an operation may cause a panic /// while an object is still in an invalid state, which could cause UB when dropped. #[inline(always)] pub fn abort_on_panic R, R>(f: F) -> R { From 527a54681dde18d28eea7734eab1d6f44663d7ed Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Wed, 12 Mar 2025 13:08:43 -0700 Subject: [PATCH 10/20] cleanup abort_on_panic --- crates/bevy_ecs/src/storage/mod.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/crates/bevy_ecs/src/storage/mod.rs b/crates/bevy_ecs/src/storage/mod.rs index 06fb846a8f796..84267078c8ed3 100644 --- a/crates/bevy_ecs/src/storage/mod.rs +++ b/crates/bevy_ecs/src/storage/mod.rs @@ -61,20 +61,20 @@ impl Storages { } } -struct AbortOnPanic; - -impl Drop for AbortOnPanic { - fn drop(&mut self) { - // Panicking while unwinding will force an abort. - panic!("Aborting due to allocator error"); - } -} - /// Calls a function, aborting the program if it panics. /// This should be used whenever an operation may cause a panic /// while an object is still in an invalid state, which could cause UB when dropped. #[inline(always)] -pub fn abort_on_panic R, R>(f: F) -> R { +fn abort_on_panic R, R>(f: F) -> R { + struct AbortOnPanic; + + impl Drop for AbortOnPanic { + fn drop(&mut self) { + // Panicking while unwinding will force an abort. + panic!("Aborting due to allocator error"); + } + } + let _guard = AbortOnPanic; let ret = f(); // the operation was successful, so we don't drop the guard From 0303e85b56b2c32ed4080071708f552c7182e787 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Wed, 12 Mar 2025 13:23:18 -0700 Subject: [PATCH 11/20] cleanup --- crates/bevy_ecs/src/storage/sparse_set.rs | 70 ++++++++--------------- 1 file changed, 24 insertions(+), 46 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index ba25926424c81..5cd11bfa14343 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -6,7 +6,13 @@ use crate::{ }; use alloc::{boxed::Box, vec::Vec}; use bevy_ptr::{OwningPtr, Ptr}; -use core::{cell::UnsafeCell, hash::Hash, marker::PhantomData, num::NonZeroUsize, panic::Location}; +use core::{ + cell::UnsafeCell, + hash::Hash, + marker::PhantomData, + num::{NonZero, NonZeroUsize}, + panic::Location, +}; use nonmax::NonMaxUsize; use super::{abort_on_panic, ThinColumn}; @@ -383,57 +389,29 @@ impl ComponentSparseSet { self.entities.reserve(additional); // use entities vector capacity as driving capacity for all related allocations - let new_capacity = self.capacity(); + // + // SAFETY: `additional` must be > 0 for the branch above to succeed, so `new_capacity` + // must be nonzero + let new_capacity = unsafe { NonZeroUsize::new_unchecked(self.capacity()) }; if column_cap == 0 { - // SAFETY: the current capacity is 0 - unsafe { self.alloc_dense(NonZeroUsize::new_unchecked(new_capacity)) }; + // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. + // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and + // abort the program. + abort_on_panic(|| { + self.dense.alloc(new_capacity); + }); } else { - // SAFETY: - // - `column_cap` is indeed the columns' capacity - unsafe { - self.realloc_dense( - NonZeroUsize::new_unchecked(column_cap), - NonZeroUsize::new_unchecked(new_capacity), - ); - }; + // SAFETY: `column_cap` is nonzero + let column_cap = unsafe { NonZeroUsize::new_unchecked(column_cap) }; + + // SAFETY: `column_cap` is indeed the column's capacity + abort_on_panic(|| unsafe { + self.dense.realloc(column_cap, new_capacity); + }); } } } - - /// Allocate memory for the columns in the [`Table`] - /// - /// The current capacity of the columns should be 0, if it's not 0, then the previous data will be overwritten and leaked. - fn alloc_dense(&mut self, new_capacity: NonZeroUsize) { - // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. - // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and - // abort the program. - abort_on_panic(|| { - self.dense.alloc(new_capacity); - }); - } - - /// Reallocate memory for the columns in the [`Table`] - /// - /// # Safety - /// - `current_column_capacity` is indeed the capacity of the columns - unsafe fn realloc_dense( - &mut self, - current_column_capacity: NonZeroUsize, - new_capacity: NonZeroUsize, - ) { - // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. - // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and - // abort the program. - - // SAFETY: - // - There's no overflow - // - `current_capacity` is indeed the capacity - safety requirement - // - current capacity > 0 - abort_on_panic(|| unsafe { - self.dense.realloc(current_column_capacity, new_capacity); - }); - } } impl Drop for ComponentSparseSet { From d01d4e6cc3ecf9b2f099a46442458b87499b9dbd Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Wed, 12 Mar 2025 13:28:21 -0700 Subject: [PATCH 12/20] docs --- crates/bevy_ecs/src/storage/table/column.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index 9ac85f53c60d0..99c1168b1b521 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -5,7 +5,10 @@ use crate::{ }; use core::panic::Location; -/// Very similar to a normal [`Column`], but with the capacities and lengths cut out for performance reasons. +/// Dense ECS component storage. +/// +/// A series of arrays storing component data and change ticks, with the capacities and lengths cut out for +/// performance reasons. /// /// This type is used by [`Table`], because all of the capacities and lengths of the [`Table`]'s columns must match. /// From 8aaf1a33ca771fbe379d328f2d88d1b2f1bf5027 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Wed, 12 Mar 2025 13:38:45 -0700 Subject: [PATCH 13/20] cleanup again --- crates/bevy_ecs/src/storage/sparse_set.rs | 34 +++++++++++++---------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 5cd11bfa14343..7191ad06c6f95 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -394,21 +394,25 @@ impl ComponentSparseSet { // must be nonzero let new_capacity = unsafe { NonZeroUsize::new_unchecked(self.capacity()) }; - if column_cap == 0 { - // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. - // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and - // abort the program. - abort_on_panic(|| { - self.dense.alloc(new_capacity); - }); - } else { - // SAFETY: `column_cap` is nonzero - let column_cap = unsafe { NonZeroUsize::new_unchecked(column_cap) }; - - // SAFETY: `column_cap` is indeed the column's capacity - abort_on_panic(|| unsafe { - self.dense.realloc(column_cap, new_capacity); - }); + match NonZeroUsize::new(column_cap) { + Some(column_cap) => { + // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. + // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and + // abort the program. + + // SAFETY: `column_cap` is indeed the column's capacity + abort_on_panic(|| unsafe { + self.dense.realloc(column_cap, new_capacity); + }); + } + None => { + // If any of these allocations trigger an unwind, the wrong capacity will be used while dropping this table - UB. + // To avoid this, we use `abort_on_panic`. If the allocation triggered a panic, the guard will be triggered, and + // abort the program. + abort_on_panic(|| { + self.dense.alloc(new_capacity); + }); + } } } } From 3007c112aeaa4f16cfb74cebaa723dfe80606dd2 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Wed, 12 Mar 2025 13:43:40 -0700 Subject: [PATCH 14/20] remove dead code in blob_vec.rs --- crates/bevy_ecs/src/storage/blob_vec.rs | 33 +---------------------- crates/bevy_ecs/src/storage/sparse_set.rs | 8 +----- 2 files changed, 2 insertions(+), 39 deletions(-) diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 2451fccb140f8..3dc742b8c9180 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -1,7 +1,7 @@ use alloc::alloc::handle_alloc_error; use bevy_ptr::{OwningPtr, Ptr, PtrMut}; use bevy_utils::OnDrop; -use core::{alloc::Layout, cell::UnsafeCell, num::NonZero, ptr::NonNull}; +use core::{alloc::Layout, num::NonZero, ptr::NonNull}; /// A flat, type-erased data storage type /// @@ -88,12 +88,6 @@ impl BlobVec { self.len == 0 } - /// Returns the [`Layout`] of the element type stored in the vector. - #[inline] - pub fn layout(&self) -> Layout { - self.item_layout - } - /// Reserves the minimum capacity for at least `additional` more elements to be inserted in the given `BlobVec`. /// After calling `reserve_exact`, capacity will be greater than or equal to `self.len() + additional`. Does nothing if /// the capacity is already sufficient. @@ -293,22 +287,6 @@ impl BlobVec { unsafe { p.promote() } } - /// Removes the value at `index` and drops it. - /// Does not do any bounds checking on `index`. - /// The removed element is replaced by the last element of the `BlobVec`. - /// - /// # Safety - /// It is the caller's responsibility to ensure that `index` is `< self.len()`. - #[inline] - pub unsafe fn swap_remove_and_drop_unchecked(&mut self, index: usize) { - debug_assert!(index < self.len()); - let drop = self.drop; - let value = self.swap_remove_and_forget_unchecked(index); - if let Some(drop) = drop { - drop(value); - } - } - /// Returns a reference to the element at `index`, without doing bounds checking. /// /// # Safety @@ -357,15 +335,6 @@ impl BlobVec { unsafe { PtrMut::new(self.data) } } - /// Get a reference to the entire [`BlobVec`] as if it were an array with elements of type `T` - /// - /// # Safety - /// The type `T` must be the type of the items in this [`BlobVec`]. - pub unsafe fn get_slice(&self) -> &[UnsafeCell] { - // SAFETY: the inner data will remain valid for as long as 'self. - unsafe { core::slice::from_raw_parts(self.data.as_ptr() as *const UnsafeCell, self.len) } - } - /// Clears the vector, removing (and dropping) all values. /// /// Note that this method has no effect on the allocated capacity of the vector. diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 7191ad06c6f95..63ef187ae5eb0 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -6,13 +6,7 @@ use crate::{ }; use alloc::{boxed::Box, vec::Vec}; use bevy_ptr::{OwningPtr, Ptr}; -use core::{ - cell::UnsafeCell, - hash::Hash, - marker::PhantomData, - num::{NonZero, NonZeroUsize}, - panic::Location, -}; +use core::{cell::UnsafeCell, hash::Hash, marker::PhantomData, num::NonZeroUsize, panic::Location}; use nonmax::NonMaxUsize; use super::{abort_on_panic, ThinColumn}; From ba457db5dd6b76cd7a04f5d3f28a02c34ad382f2 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Wed, 12 Mar 2025 16:01:49 -0700 Subject: [PATCH 15/20] remove extraneous return --- crates/bevy_ecs/src/storage/sparse_set.rs | 2 +- crates/bevy_ecs/src/storage/table/column.rs | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index 63ef187ae5eb0..d8f3b66bb6010 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -324,7 +324,7 @@ impl ComponentSparseSet { let is_last = dense_index.as_usize() == last_index; self.entities.swap_remove(dense_index.as_usize()); // SAFETY: dense_index was just removed from `sparse`, which ensures that it is valid - let (value, _, _) = unsafe { + let value = unsafe { self.dense .swap_remove_and_forget_unchecked(last_index, dense_index) }; diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index 99c1168b1b521..b9f86fa4ff010 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -95,25 +95,23 @@ impl ThinColumn { &mut self, last_element_index: usize, row: TableRow, - ) -> (OwningPtr, ComponentTicks, MaybeLocation) { + ) -> OwningPtr { let data = self .data .swap_remove_unchecked(row.as_usize(), last_element_index); - let added = self - .added_ticks + self.added_ticks .swap_remove_unchecked(row.as_usize(), last_element_index) .read(); - let changed = self - .changed_ticks + self.changed_ticks .swap_remove_unchecked(row.as_usize(), last_element_index) .read(); - let changed_by = self.changed_by.as_mut().map(|changed_by| { + self.changed_by.as_mut().map(|changed_by| { changed_by .swap_remove_unchecked(row.as_usize(), last_element_index) .read() }); - (data, ComponentTicks { added, changed }, changed_by) + data } /// Call [`realloc`](std::alloc::realloc) to expand / shrink the memory allocation for this [`ThinColumn`] From 6d1f92cf1e7f05df9e4410a9a79129d2d997d81d Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Thu, 13 Mar 2025 12:04:13 -0700 Subject: [PATCH 16/20] fix doc --- crates/bevy_ecs/src/storage/blob_vec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 3dc742b8c9180..f267dfa03c2c1 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -245,7 +245,7 @@ impl BlobVec { /// Appends an element to the back of the vector. /// /// # Safety - /// The `value` must match the [`layout`](`BlobVec::layout`) of the elements in the [`BlobVec`]. + /// The `value` must match the layout of the elements in the [`BlobVec`]. #[inline] pub unsafe fn push(&mut self, value: OwningPtr<'_>) { self.reserve(1); From d3e50db4328de9ffd845904d1f437f2e9f8c6658 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Fri, 14 Mar 2025 12:34:25 -0700 Subject: [PATCH 17/20] Update crates/bevy_ecs/src/storage/table/column.rs Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> --- crates/bevy_ecs/src/storage/table/column.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index b9f86fa4ff010..69aa1072b2a48 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -81,7 +81,10 @@ impl ThinColumn { }); } - /// Swap-remove and forget the removed element. + /// Removes an element from the [`ThinColumn`] and returns it. + /// This does not preserve ordering, but is O(1) and does not do any bounds checking. + /// + /// The element is replaced with the last element in the [`ThinColumn`]. /// /// It's the caller's responsibility to ensure that the returned value is dropped or used. /// Failure to do so may result in resources not being released (i.e. file handles not From c6a69721e4712400f084d5eccd98b8842a93a7a9 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Fri, 14 Mar 2025 12:35:30 -0700 Subject: [PATCH 18/20] remove extraneous `read()` calls --- crates/bevy_ecs/src/storage/table/column.rs | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index 69aa1072b2a48..c2e7704a700e5 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -103,16 +103,12 @@ impl ThinColumn { .data .swap_remove_unchecked(row.as_usize(), last_element_index); self.added_ticks - .swap_remove_unchecked(row.as_usize(), last_element_index) - .read(); + .swap_remove_unchecked(row.as_usize(), last_element_index); self.changed_ticks - .swap_remove_unchecked(row.as_usize(), last_element_index) - .read(); - self.changed_by.as_mut().map(|changed_by| { - changed_by - .swap_remove_unchecked(row.as_usize(), last_element_index) - .read() - }); + .swap_remove_unchecked(row.as_usize(), last_element_index); + self.changed_by + .as_mut() + .map(|changed_by| changed_by.swap_remove_unchecked(row.as_usize(), last_element_index)); data } From 0b002fa41dca773acbcb28997cddb6401e8f3c2d Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Fri, 14 Mar 2025 12:49:55 -0700 Subject: [PATCH 19/20] Dummy impl Debug for ThinColumn --- crates/bevy_ecs/src/storage/sparse_set.rs | 1 + crates/bevy_ecs/src/storage/table/column.rs | 9 ++++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/sparse_set.rs b/crates/bevy_ecs/src/storage/sparse_set.rs index d8f3b66bb6010..dac72f61cb156 100644 --- a/crates/bevy_ecs/src/storage/sparse_set.rs +++ b/crates/bevy_ecs/src/storage/sparse_set.rs @@ -116,6 +116,7 @@ impl SparseArray { /// A sparse data structure of [`Component`](crate::component::Component)s. /// /// Designed for relatively fast insertions and deletions. +#[derive(Debug)] pub struct ComponentSparseSet { /// SAFETY: Equal in length & capacity to `self.entities` dense: ThinColumn, diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index c2e7704a700e5..03b58c396e62f 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -3,7 +3,7 @@ use crate::{ change_detection::MaybeLocation, storage::{blob_array::BlobArray, thin_array_ptr::ThinArrayPtr}, }; -use core::panic::Location; +use core::{fmt, panic::Location}; /// Dense ECS component storage. /// @@ -22,6 +22,13 @@ pub struct ThinColumn { pub(super) changed_by: MaybeLocation>>>, } +impl fmt::Debug for ThinColumn { + // Required method + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ThinColumn").finish() + } +} + impl ThinColumn { /// Create a new [`ThinColumn`] with the given `capacity`. pub fn with_capacity(component_info: &ComponentInfo, capacity: usize) -> Self { From 6fe6a7f81d00610fb5f05f12a24158de563ef730 Mon Sep 17 00:00:00 2001 From: Emerson Coskey Date: Mon, 19 May 2025 15:22:45 -0700 Subject: [PATCH 20/20] remove dead code --- crates/bevy_ecs/src/storage/blob_vec.rs | 15 --------------- crates/bevy_ecs/src/storage/table/column.rs | 2 +- 2 files changed, 1 insertion(+), 16 deletions(-) diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index d5b77588be4be..f267dfa03c2c1 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -335,21 +335,6 @@ impl BlobVec { unsafe { PtrMut::new(self.data) } } - /// Get a reference to the entire [`BlobVec`] as if it were an array with elements of type `T` - /// - /// # Safety - /// The type `T` must be the type of the items in this [`BlobVec`]. - pub unsafe fn get_slice(&self) -> &[UnsafeCell] { - // SAFETY: the inner data will remain valid for as long as 'self. - unsafe { core::slice::from_raw_parts(self.data.as_ptr() as *const UnsafeCell, self.len) } - } - - /// Returns the drop function for values stored in the vector, - /// or `None` if they don't need to be dropped. - #[inline] - pub fn get_drop(&self) -> Option)> { - self.drop - } /// Clears the vector, removing (and dropping) all values. /// /// Note that this method has no effect on the allocated capacity of the vector. diff --git a/crates/bevy_ecs/src/storage/table/column.rs b/crates/bevy_ecs/src/storage/table/column.rs index 0c5c8b689af31..f55dedae52a73 100644 --- a/crates/bevy_ecs/src/storage/table/column.rs +++ b/crates/bevy_ecs/src/storage/table/column.rs @@ -409,6 +409,6 @@ impl ThinColumn { /// or `None` if they don't need to be dropped. #[inline] pub fn get_drop(&self) -> Option)> { - self.data.get_drop() + self.data.drop } }