Skip to content

Commit

Permalink
Replace BlobVec's swap_scratch with a swap_nonoverlapping (bevyengine…
Browse files Browse the repository at this point in the history
…#4853)

# 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`.
  • Loading branch information
james7132 authored and ItsDoot committed Feb 1, 2023
1 parent 6b27419 commit edec09f
Showing 1 changed file with 13 additions and 31 deletions.
44 changes: 13 additions & 31 deletions crates/bevy_ecs/src/storage/blob_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ pub(super) struct BlobVec {
len: usize,
// the `data` ptr's layout is always `array_layout(item_layout, capacity)`
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
drop: Option<unsafe fn(OwningPtr<'_>)>,
}
Expand All @@ -31,7 +29,6 @@ impl std::fmt::Debug for BlobVec {
.field("capacity", &self.capacity)
.field("len", &self.len)
.field("data", &self.data)
.field("swap_scratch", &self.swap_scratch)
.finish()
}
}
Expand All @@ -52,18 +49,14 @@ impl BlobVec {
if item_layout.size() == 0 {
let align = NonZeroUsize::new(item_layout.align()).expect("alignment must be > 0");
BlobVec {
swap_scratch: NonNull::dangling(),
data: bevy_ptr::dangling_with_align(align),
capacity: usize::MAX,
len: 0,
item_layout,
drop,
}
} 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 {
swap_scratch,
data: NonNull::dangling(),
capacity: 0,
len: 0,
Expand Down Expand Up @@ -213,28 +206,23 @@ impl BlobVec {
/// caller's responsibility to drop the returned pointer, if that is desirable.
///
/// # 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]
#[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<'_> {
// 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());
let last = self.len - 1;
let swap_scratch = self.swap_scratch.as_ptr();
std::ptr::copy_nonoverlapping::<u8>(
self.get_unchecked_mut(index).as_ptr(),
swap_scratch,
self.item_layout.size(),
);
std::ptr::copy::<u8>(
self.get_unchecked_mut(last).as_ptr(),
self.get_unchecked_mut(index).as_ptr(),
self.item_layout.size(),
);
self.len -= 1;
OwningPtr::new(self.swap_scratch)
let new_len = self.len - 1;
let size = self.item_layout.size();
if index != new_len {
std::ptr::swap_nonoverlapping::<u8>(
self.get_unchecked_mut(index).as_ptr(),
self.get_unchecked_mut(new_len).as_ptr(),
size,
);
}
self.len = new_len;
// 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()
}

/// Removes the value at `index` and copies the value stored into `ptr`.
Expand Down Expand Up @@ -333,12 +321,6 @@ impl BlobVec {
impl Drop for BlobVec {
fn drop(&mut self) {
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 =
array_layout(&self.item_layout, self.capacity).expect("array layout should be valid");
if array_layout.size() > 0 {
Expand Down

0 comments on commit edec09f

Please sign in to comment.