Skip to content

Commit 9eb6928

Browse files
committed
Directly copy moved Table components to the target location (#5056)
# Objective Speed up entity moves between tables by reducing the number of copies conducted. Currently three separate copies are conducted: `src[index] -> swap scratch`, `src[last] -> src[index]`, and `swap scratch -> dst[target]`. The first and last copies can be merged by directly using the copy `src[index] -> dst[target]`, which can save quite some time if the component(s) in question are large. ## Solution This PR does the following: - Adds `BlobVec::swap_remove_unchecked(usize, PtrMut<'_>)`, which is identical to `swap_remove_and_forget_unchecked`, but skips the `swap_scratch` and directly copies the component into the provided `PtrMut<'_>`. - Build `Column::initialize_from_unchecked(&mut Column, usize, usize)` on top of it, which uses the above to directly initialize a row from another column. - Update most of the table move APIs to use `initialize_from_unchecked` instead of a combination of `swap_remove_and_forget_unchecked` and `initialize`. This is an alternative, though orthogonal, approach to achieve the same performance gains as seen in #4853. This (hopefully) shouldn't run into the same Miri limitations that said PR currently does. After this PR, `swap_remove_and_forget_unchecked` is still in use for Resources and swap_scratch likely still should be removed, so #4853 still has use, even if this PR is merged. ## Performance TODO: Microbenchmark This PR shows similar improvements to commands that add or remove table components that result in a table move. When tested on `many_cubes sphere`, some of the more command heavy systems saw notable improvements. In particular, `prepare_uniform_components<T>`, this saw a reduction in time from 1.35ms to 1.13ms (a 16.3% improvement) on my local machine, a similar if not slightly better gain than what #4853 showed [here](#4853 (comment)). ![image](https://user-images.githubusercontent.com/3137680/174570088-1c4c6fd7-3215-478c-9eb7-8bd9fe486b32.png) The command heavy `Extract` stage also saw a smaller overall improvement: ![image](https://user-images.githubusercontent.com/3137680/174572261-8a48f004-ab9f-4cb2-b304-a882b6d78065.png) --- ## Changelog Added: `BlobVec::swap_remove_unchecked`. Added: `Column::initialize_from_unchecked`.
1 parent c27a3cf commit 9eb6928

File tree

3 files changed

+56
-8
lines changed

3 files changed

+56
-8
lines changed

crates/bevy_ecs/src/query/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ pub use iter::*;
1111
pub use state::*;
1212

1313
#[allow(unreachable_code)]
14-
unsafe fn debug_checked_unreachable() -> ! {
14+
pub(crate) unsafe fn debug_checked_unreachable() -> ! {
1515
#[cfg(debug_assertions)]
1616
unreachable!();
1717
std::hint::unreachable_unchecked();

crates/bevy_ecs/src/storage/blob_vec.rs

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,27 @@ impl BlobVec {
209209
OwningPtr::new(self.swap_scratch)
210210
}
211211

212+
/// Removes the value at `index` and copies the value stored into `ptr`.
213+
/// Does not do any bounds checking on `index`.
214+
///
215+
/// # Safety
216+
/// It is the caller's responsibility to ensure that `index` is < `self.len()`
217+
/// and that `self[index]` has been properly initialized.
218+
#[inline]
219+
pub unsafe fn swap_remove_unchecked(&mut self, index: usize, ptr: PtrMut<'_>) {
220+
debug_assert!(index < self.len());
221+
let last = self.get_unchecked_mut(self.len - 1).as_ptr();
222+
let target = self.get_unchecked_mut(index).as_ptr();
223+
// Copy the item at the index into the provided ptr
224+
std::ptr::copy_nonoverlapping::<u8>(target, ptr.as_ptr(), self.item_layout.size());
225+
// Recompress the storage by moving the previous last element into the
226+
// now-free row overwriting the previous data. The removed row may be the last
227+
// one so a non-overlapping copy must not be used here.
228+
std::ptr::copy::<u8>(last, target, self.item_layout.size());
229+
// Invalidate the data stored in the last row, as it has been moved
230+
self.len -= 1;
231+
}
232+
212233
/// # Safety
213234
/// It is the caller's responsibility to ensure that `index` is < self.len()
214235
#[inline]

crates/bevy_ecs/src/storage/table.rs

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use crate::{
22
component::{ComponentId, ComponentInfo, ComponentTicks, Components},
33
entity::Entity,
4+
query::debug_checked_unreachable,
45
storage::{blob_vec::BlobVec, SparseSet},
56
};
67
use bevy_ptr::{OwningPtr, Ptr, PtrMut};
@@ -122,6 +123,30 @@ impl Column {
122123
(data, ticks)
123124
}
124125

126+
/// Removes the element from `other` at `src_row` and inserts it
127+
/// into the current column to initialize the values at `dst_row`.
128+
/// Does not do any bounds checking.
129+
///
130+
/// # Safety
131+
///
132+
/// - `other` must have the same data layout as `self`
133+
/// - `src_row` must be in bounds for `other`
134+
/// - `dst_row` must be in bounds for `self`
135+
/// - `other[src_row]` must be initialized to a valid value.
136+
/// - `self[dst_row]` must not be initalized yet.
137+
#[inline]
138+
pub(crate) unsafe fn initialize_from_unchecked(
139+
&mut self,
140+
other: &mut Column,
141+
src_row: usize,
142+
dst_row: usize,
143+
) {
144+
debug_assert!(self.data.layout() == other.data.layout());
145+
let ptr = self.data.get_unchecked_mut(dst_row);
146+
other.data.swap_remove_unchecked(src_row, ptr);
147+
*self.ticks.get_unchecked_mut(dst_row) = other.ticks.swap_remove(src_row);
148+
}
149+
125150
// # Safety
126151
// - ptr must point to valid data of this column's component type
127152
pub(crate) unsafe fn push(&mut self, ptr: OwningPtr<'_>, ticks: ComponentTicks) {
@@ -249,9 +274,11 @@ impl Table {
249274
let is_last = row == self.entities.len() - 1;
250275
let new_row = new_table.allocate(self.entities.swap_remove(row));
251276
for (component_id, column) in self.columns.iter_mut() {
252-
let (data, ticks) = column.swap_remove_and_forget_unchecked(row);
253277
if let Some(new_column) = new_table.get_column_mut(*component_id) {
254-
new_column.initialize(new_row, data, ticks);
278+
new_column.initialize_from_unchecked(column, row, new_row);
279+
} else {
280+
// It's the caller's responsibility to drop these cases.
281+
let (_, _) = column.swap_remove_and_forget_unchecked(row);
255282
}
256283
}
257284
TableMoveResult {
@@ -280,8 +307,7 @@ impl Table {
280307
let new_row = new_table.allocate(self.entities.swap_remove(row));
281308
for (component_id, column) in self.columns.iter_mut() {
282309
if let Some(new_column) = new_table.get_column_mut(*component_id) {
283-
let (data, ticks) = column.swap_remove_and_forget_unchecked(row);
284-
new_column.initialize(new_row, data, ticks);
310+
new_column.initialize_from_unchecked(column, row, new_row);
285311
} else {
286312
column.swap_remove_unchecked(row);
287313
}
@@ -311,9 +337,10 @@ impl Table {
311337
let is_last = row == self.entities.len() - 1;
312338
let new_row = new_table.allocate(self.entities.swap_remove(row));
313339
for (component_id, column) in self.columns.iter_mut() {
314-
let new_column = new_table.get_column_mut(*component_id).unwrap();
315-
let (data, ticks) = column.swap_remove_and_forget_unchecked(row);
316-
new_column.initialize(new_row, data, ticks);
340+
new_table
341+
.get_column_mut(*component_id)
342+
.unwrap_or_else(|| debug_checked_unreachable())
343+
.initialize_from_unchecked(column, row, new_row);
317344
}
318345
TableMoveResult {
319346
new_row,

0 commit comments

Comments
 (0)