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

Replace BlobVec's swap_scratch with a swap_nonoverlapping (#4853)

Browse Source 
# Objective
BlobVec currently relies on a scratch piece of memory allocated at initialization to make a temporary copy of a component when using `swap_remove_and_{forget/drop}`. This is potentially suboptimal as it writes to a, well-known, but random part of memory instead of using the stack.

## Solution
As the `FIXME` in the file states, replace `swap_scratch` with a call to `swap_nonoverlapping::<u8>`. The swapped last entry is returned as a `OwnedPtr`.

In theory, this should be faster as the temporary swap is allocated on the stack, `swap_nonoverlapping` allows for easier vectorization for bigger types, and the same memory is used between the swap and the returned `OwnedPtr`.
This commit is contained in:
James Liu 2022-11-16 20:57:43 +00:00 committed by Carter Anderson
parent f47c65e640
commit dbbd023a1b
1 changed files with 13 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -17,8 +17,6 @@ pub(super) struct BlobVec {
len: usize, len: usize,
// the `data` ptr's layout is always `array_layout(item_layout, capacity)` // the `data` ptr's layout is always `array_layout(item_layout, capacity)`
data: NonNull<u8>, data: NonNull<u8>,
// the `swap_scratch` ptr's layout is always `item_layout`
swap_scratch: NonNull<u8>,
// None if the underlying type doesn't need to be dropped // None if the underlying type doesn't need to be dropped
drop: Option<unsafe fn(OwningPtr<'_>)>, drop: Option<unsafe fn(OwningPtr<'_>)>,
} }
@ -31,7 +29,6 @@ impl std::fmt::Debug for BlobVec {
.field("capacity", &self.capacity) .field("capacity", &self.capacity)
.field("len", &self.len) .field("len", &self.len)
.field("data", &self.data) .field("data", &self.data)
.field("swap_scratch", &self.swap_scratch)
.finish() .finish()
} }
} }
@ -52,7 +49,6 @@ impl BlobVec {
if item_layout.size() == 0 { if item_layout.size() == 0 {
let align = NonZeroUsize::new(item_layout.align()).expect("alignment must be > 0"); let align = NonZeroUsize::new(item_layout.align()).expect("alignment must be > 0");
BlobVec { BlobVec {
swap_scratch: NonNull::dangling(),
data: bevy_ptr::dangling_with_align(align), data: bevy_ptr::dangling_with_align(align),
capacity: usize::MAX, capacity: usize::MAX,
len: 0, len: 0,
@ -60,10 +56,7 @@ impl BlobVec {
drop, drop,
} }
} else { } else {
let swap_scratch = NonNull::new(std::alloc::alloc(item_layout))
.unwrap_or_else(|| std::alloc::handle_alloc_error(item_layout));
let mut blob_vec = BlobVec { let mut blob_vec = BlobVec {
swap_scratch,
data: NonNull::dangling(), data: NonNull::dangling(),
capacity: 0, capacity: 0,
len: 0, len: 0,
@ -213,28 +206,23 @@ impl BlobVec {
/// caller's responsibility to drop the returned pointer, if that is desirable. /// caller's responsibility to drop the returned pointer, if that is desirable.
/// ///
/// # Safety /// # Safety
/// It is the caller's responsibility to ensure that `index` is < `self.len()` /// It is the caller's responsibility to ensure that `index` is less than `self.len()`.
#[inline] #[inline]
#[must_use = "The returned pointer should be used to dropped the removed element"] #[must_use = "The returned pointer should be used to dropped the removed element"]
pub unsafe fn swap_remove_and_forget_unchecked(&mut self, index: usize) -> OwningPtr<'_> { pub unsafe fn swap_remove_and_forget_unchecked(&mut self, index: usize) -> OwningPtr<'_> {
// FIXME: This should probably just use `core::ptr::swap` and return an `OwningPtr`
// into the underlying `BlobVec` allocation, and remove swap_scratch
debug_assert!(index < self.len()); debug_assert!(index < self.len());
let last = self.len - 1; let new_len = self.len - 1;
let swap_scratch = self.swap_scratch.as_ptr(); let size = self.item_layout.size();
std::ptr::copy_nonoverlapping::<u8>( if index != new_len {
self.get_unchecked_mut(index).as_ptr(), std::ptr::swap_nonoverlapping::<u8>(
swap_scratch, self.get_unchecked_mut(index).as_ptr(),
self.item_layout.size(), self.get_unchecked_mut(new_len).as_ptr(),
); size,
std::ptr::copy::<u8>( );
self.get_unchecked_mut(last).as_ptr(), }
self.get_unchecked_mut(index).as_ptr(), self.len = new_len;
self.item_layout.size(), // Cannot use get_unchecked here as this is technically out of bounds after changing len.
); self.get_ptr_mut().byte_add(new_len * size).promote()
self.len -= 1;
OwningPtr::new(self.swap_scratch)
} }
/// Removes the value at `index` and copies the value stored into `ptr`. /// Removes the value at `index` and copies the value stored into `ptr`.
@ -333,12 +321,6 @@ impl BlobVec {
impl Drop for BlobVec { impl Drop for BlobVec {
fn drop(&mut self) { fn drop(&mut self) {
self.clear(); self.clear();
if self.item_layout.size() > 0 {
// SAFETY: the `swap_scratch` pointer is always allocated using `self.item_layout`
unsafe {
std::alloc::dealloc(self.swap_scratch.as_ptr(), self.item_layout);
}
}
let array_layout = let array_layout =
array_layout(&self.item_layout, self.capacity).expect("array layout should be valid"); array_layout(&self.item_layout, self.capacity).expect("array layout should be valid");
if array_layout.size() > 0 { if array_layout.size() > 0 {