From 87059a3eec02f2da6749e5f18dac75581399aae2 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 20 Jun 2022 01:56:57 -0700 Subject: [PATCH 1/4] Directly copy moved components to the target location --- crates/bevy_ecs/src/query/mod.rs | 2 +- crates/bevy_ecs/src/storage/blob_vec.rs | 23 ++++++++++++++ crates/bevy_ecs/src/storage/table.rs | 41 ++++++++++++++++++++----- 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/query/mod.rs b/crates/bevy_ecs/src/query/mod.rs index 857fe033ab1d4..d037c4a541974 100644 --- a/crates/bevy_ecs/src/query/mod.rs +++ b/crates/bevy_ecs/src/query/mod.rs @@ -11,7 +11,7 @@ pub use iter::*; pub use state::*; #[allow(unreachable_code)] -unsafe fn debug_checked_unreachable() -> ! { +pub(crate) unsafe fn debug_checked_unreachable() -> ! { #[cfg(debug_assertions)] unreachable!(); std::hint::unreachable_unchecked(); diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 7a3991d35e48e..8e57dec254a26 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -209,6 +209,29 @@ impl BlobVec { OwningPtr::new(self.swap_scratch) } + /// Removes the value at `index` and copies the value stored into `ptr`. + /// Does not do any bounds checking on `index`. + /// + /// # Safety + /// It is the caller's responsibility to ensure that `index` is < `self.len()` + /// and that `self[index]` has been properly initialized. + #[inline] + pub unsafe fn remove_unchecked(&mut self, index: usize, ptr: PtrMut<'_>) { + debug_assert!(index < self.len()); + let last = self.len - 1; + std::ptr::copy_nonoverlapping::( + self.get_unchecked_mut(index).as_ptr(), + ptr.as_ptr(), + self.item_layout.size(), + ); + std::ptr::copy::( + self.get_unchecked_mut(last).as_ptr(), + self.get_unchecked_mut(index).as_ptr(), + self.item_layout.size(), + ); + self.len -= 1; + } + /// # Safety /// It is the caller's responsibility to ensure that `index` is < self.len() #[inline] diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 6150c679c88a9..81a02e1c8f261 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -1,6 +1,7 @@ use crate::{ component::{ComponentId, ComponentInfo, ComponentTicks, Components}, entity::Entity, + query::debug_checked_unreachable, storage::{BlobVec, SparseSet}, }; use bevy_ptr::{OwningPtr, Ptr, PtrMut}; @@ -119,6 +120,30 @@ impl Column { (data, ticks) } + /// Removes the element from `other` at `src_row` and inserts it + /// into the current column to initialize the values at `dst_row`. + /// Does not do any bounds checking. + /// + /// # Safety + /// + /// - `other` must have the same data layout as `self + /// - `src_row` must be in bounds for `other` + /// - `dst_row` must be in bounds for `self` + /// - `other[src_row]` must be initialized to a valid value. + /// - `self[dst_row]` must not be initalized yet. + #[inline] + pub(crate) unsafe fn initialize_from_unchecked( + &mut self, + other: &mut Column, + src_row: usize, + dst_row: usize, + ) { + debug_assert!(self.data.layout() == other.data.layout()); + let ptr = self.data.get_unchecked_mut(dst_row); + other.data.remove_unchecked(src_row, ptr); + *self.ticks.get_unchecked_mut(dst_row) = other.ticks.swap_remove(src_row); + } + // # Safety // - ptr must point to valid data of this column's component type pub(crate) unsafe fn push(&mut self, ptr: OwningPtr<'_>, ticks: ComponentTicks) { @@ -253,9 +278,11 @@ impl Table { let is_last = row == self.entities.len() - 1; let new_row = new_table.allocate(self.entities.swap_remove(row)); for (component_id, column) in self.columns.iter_mut() { - let (data, ticks) = column.swap_remove_and_forget_unchecked(row); if let Some(new_column) = new_table.get_column_mut(*component_id) { - new_column.initialize(new_row, data, ticks); + new_column.initialize_from_unchecked(column, row, new_row); + } else { + // It's the caller's responsibility to drop these cases. + let (_, _) = column.swap_remove_and_forget_unchecked(row); } } TableMoveResult { @@ -284,8 +311,7 @@ impl Table { let new_row = new_table.allocate(self.entities.swap_remove(row)); for (component_id, column) in self.columns.iter_mut() { if let Some(new_column) = new_table.get_column_mut(*component_id) { - let (data, ticks) = column.swap_remove_and_forget_unchecked(row); - new_column.initialize(new_row, data, ticks); + new_column.initialize_from_unchecked(column, row, new_row); } else { column.swap_remove_unchecked(row); } @@ -315,9 +341,10 @@ impl Table { let is_last = row == self.entities.len() - 1; let new_row = new_table.allocate(self.entities.swap_remove(row)); for (component_id, column) in self.columns.iter_mut() { - let new_column = new_table.get_column_mut(*component_id).unwrap(); - let (data, ticks) = column.swap_remove_and_forget_unchecked(row); - new_column.initialize(new_row, data, ticks); + new_table + .get_column_mut(*component_id) + .unwrap_or_else(|| debug_checked_unreachable()) + .initialize_from_unchecked(column, row, new_row); } TableMoveResult { new_row, From df2b08a84d30e9408f448b69814cc1da2c9125e6 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 20 Jun 2022 02:46:06 -0700 Subject: [PATCH 2/4] Fix docs --- crates/bevy_ecs/src/storage/table.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 81a02e1c8f261..4fce412ec4958 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -126,7 +126,7 @@ impl Column { /// /// # Safety /// - /// - `other` must have the same data layout as `self + /// - `other` must have the same data layout as `self` /// - `src_row` must be in bounds for `other` /// - `dst_row` must be in bounds for `self` /// - `other[src_row]` must be initialized to a valid value. From 0672ecbe3991a77399c26de5f8f9fd374f0b1d00 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 20 Jun 2022 17:27:05 -0700 Subject: [PATCH 3/4] Review comments --- crates/bevy_ecs/src/storage/blob_vec.rs | 22 ++++++++++------------ crates/bevy_ecs/src/storage/table.rs | 2 +- 2 files changed, 11 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/storage/blob_vec.rs b/crates/bevy_ecs/src/storage/blob_vec.rs index 8e57dec254a26..63986331d1411 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -216,19 +216,17 @@ impl BlobVec { /// It is the caller's responsibility to ensure that `index` is < `self.len()` /// and that `self[index]` has been properly initialized. #[inline] - pub unsafe fn remove_unchecked(&mut self, index: usize, ptr: PtrMut<'_>) { + pub unsafe fn swap_remove_unchecked(&mut self, index: usize, ptr: PtrMut<'_>) { debug_assert!(index < self.len()); - let last = self.len - 1; - std::ptr::copy_nonoverlapping::( - self.get_unchecked_mut(index).as_ptr(), - ptr.as_ptr(), - self.item_layout.size(), - ); - std::ptr::copy::( - self.get_unchecked_mut(last).as_ptr(), - self.get_unchecked_mut(index).as_ptr(), - self.item_layout.size(), - ); + let last = self.get_unchecked_mut(self.len - 1).as_ptr(); + let target = self.get_unchecked_mut(index).as_ptr(); + // Copy the item at the index into the provided ptr + std::ptr::copy_nonoverlapping::(target, ptr.as_ptr(), self.item_layout.size()); + // Recompress the storage by moving the previous last element into the + // now-free row overwriting the previous data. The removed row may be the last + // one so a non-overlapping copy must be used here. + std::ptr::copy::(last, target, self.item_layout.size()); + // Invalidate the data stored in the last row, as it has been moved self.len -= 1; } diff --git a/crates/bevy_ecs/src/storage/table.rs b/crates/bevy_ecs/src/storage/table.rs index 4fce412ec4958..0625dc9f25b5b 100644 --- a/crates/bevy_ecs/src/storage/table.rs +++ b/crates/bevy_ecs/src/storage/table.rs @@ -140,7 +140,7 @@ impl Column { ) { debug_assert!(self.data.layout() == other.data.layout()); let ptr = self.data.get_unchecked_mut(dst_row); - other.data.remove_unchecked(src_row, ptr); + other.data.swap_remove_unchecked(src_row, ptr); *self.ticks.get_unchecked_mut(dst_row) = other.ticks.swap_remove(src_row); } From b48d38ce36171f2f67600805c14a8c87f33d37a1 Mon Sep 17 00:00:00 2001 From: james7132 Date: Mon, 20 Jun 2022 18:11:10 -0700 Subject: [PATCH 4/4] Stupid double negatives --- 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 63986331d1411..f68e86d868130 100644 --- a/crates/bevy_ecs/src/storage/blob_vec.rs +++ b/crates/bevy_ecs/src/storage/blob_vec.rs @@ -224,7 +224,7 @@ impl BlobVec { std::ptr::copy_nonoverlapping::(target, ptr.as_ptr(), self.item_layout.size()); // Recompress the storage by moving the previous last element into the // now-free row overwriting the previous data. The removed row may be the last - // one so a non-overlapping copy must be used here. + // one so a non-overlapping copy must not be used here. std::ptr::copy::(last, target, self.item_layout.size()); // Invalidate the data stored in the last row, as it has been moved self.len -= 1;