forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Directly copy moved Table components to the target location (#5056)

Browse Source 
# 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](https://github.com/bevyengine/bevy/pull/4853#issuecomment-1159346106).

![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`.
This commit is contained in:
James Liu 2022-06-27 16:52:26 +00:00
parent c27a3cff6d
commit 9eb69282ef
3 changed files with 56 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
crates/bevy_ecs/src/query/mod.rs
View File

@ -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();

21
crates/bevy_ecs/src/storage/blob_vec.rs
View File

@ -209,6 +209,27 @@ 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 swap_remove_unchecked(&mut self, index: usize, ptr: PtrMut<'_>) {
debug_assert!(index < self.len());
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::<u8>(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 not be used here.
std::ptr::copy::<u8>(last, target, self.item_layout.size());
// Invalidate the data stored in the last row, as it has been moved
self.len -= 1;
}
/// # Safety
/// It is the caller's responsibility to ensure that `index` is < self.len()
#[inline]

41
crates/bevy_ecs/src/storage/table.rs
View File

@ -1,6 +1,7 @@
use crate::{
component::{ComponentId, ComponentInfo, ComponentTicks, Components},
entity::Entity,
query::debug_checked_unreachable,
storage::{blob_vec::BlobVec, SparseSet},
};
use bevy_ptr::{OwningPtr, Ptr, PtrMut};
@ -122,6 +123,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.swap_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) {
@ -249,9 +274,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 {
@ -280,8 +307,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);
}
@ -311,9 +337,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,