Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Skip drop when needs_drop is false #4773

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions crates/bevy_ecs/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use bevy_ptr::OwningPtr;
use std::{
alloc::Layout,
any::{Any, TypeId},
mem::needs_drop,
};

/// A component is data associated with an [`Entity`](crate::entity::Entity). Each entity can have
Expand Down Expand Up @@ -111,7 +112,11 @@ impl ComponentInfo {
}

#[inline]
pub fn drop(&self) -> unsafe fn(OwningPtr<'_>) {
/// Get the function which should be called to clean up values, i.e.
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
/// [`Drop`] them.
/// Returns `None` if the underlying type doesn't need to be dropped,
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
/// as reported by [`needs_drop`].
pub fn drop(&self) -> Option<unsafe fn(OwningPtr<'_>)> {
self.descriptor.drop
}

Expand Down Expand Up @@ -168,7 +173,8 @@ pub struct ComponentDescriptor {
layout: Layout,
// SAFETY: this function must be safe to call with pointers pointing to items of the type
// this descriptor describes.
drop: for<'a> unsafe fn(OwningPtr<'a>),
// None if the underlying type doesn't need to be dropped
drop: Option<for<'a> unsafe fn(OwningPtr<'a>)>,
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
}

// We need to ignore the `drop` field in our `Debug` impl
Expand Down Expand Up @@ -197,7 +203,7 @@ impl ComponentDescriptor {
is_send_and_sync: true,
type_id: Some(TypeId::of::<T>()),
layout: Layout::new::<T>(),
drop: Self::drop_ptr::<T>,
drop: needs_drop::<T>().then(|| Self::drop_ptr::<T> as _),
}
}

Expand All @@ -213,7 +219,7 @@ impl ComponentDescriptor {
is_send_and_sync: true,
type_id: Some(TypeId::of::<T>()),
layout: Layout::new::<T>(),
drop: Self::drop_ptr::<T>,
drop: needs_drop::<T>().then(|| Self::drop_ptr::<T> as _),
}
}

Expand All @@ -224,7 +230,7 @@ impl ComponentDescriptor {
is_send_and_sync: false,
type_id: Some(TypeId::of::<T>()),
layout: Layout::new::<T>(),
drop: Self::drop_ptr::<T>,
drop: needs_drop::<T>().then(|| Self::drop_ptr::<T> as _),
}
}

Expand Down
42 changes: 26 additions & 16 deletions crates/bevy_ecs/src/storage/blob_vec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ pub struct BlobVec {
len: usize,
data: NonNull<u8>,
swap_scratch: NonNull<u8>,
drop: unsafe fn(OwningPtr<'_>),
// None if the underlying type doesn't need to be dropped
drop: Option<unsafe fn(OwningPtr<'_>)>,
}

// We want to ignore the `drop` field in our `Debug` impl
Expand All @@ -36,9 +37,13 @@ impl BlobVec {
/// # Safety
///
/// `drop` should be safe to call with an [`OwningPtr`] pointing to any item that's been pushed into this [`BlobVec`].
///
/// If `drop` is `None`, the items will be leaked. This should generally be set as None based on [`needs_drop`].
///
/// [`needs_drop`]: core::mem::needs_drop
pub unsafe fn new(
item_layout: Layout,
drop: unsafe fn(OwningPtr<'_>),
drop: Option<unsafe fn(OwningPtr<'_>)>,
capacity: usize,
) -> BlobVec {
if item_layout.size() == 0 {
Expand Down Expand Up @@ -141,7 +146,9 @@ impl BlobVec {
let ptr = self.get_unchecked_mut(index).promote().as_ptr();
self.len = 0;
// Drop the old value, then write back, justifying the promotion
(self.drop)(OwningPtr::new(NonNull::new_unchecked(ptr)));
if let Some(drop) = self.drop {
(drop)(OwningPtr::new(NonNull::new_unchecked(ptr)));
}
std::ptr::copy_nonoverlapping::<u8>(value.as_ptr(), ptr, self.item_layout.size());
self.len = old_len;
}
Expand Down Expand Up @@ -204,7 +211,9 @@ impl BlobVec {
debug_assert!(index < self.len());
let drop = self.drop;
let value = self.swap_remove_and_forget_unchecked(index);
(drop)(value);
if let Some(drop) = drop {
(drop)(value);
}
}

/// # Safety
Expand Down Expand Up @@ -252,14 +261,15 @@ impl BlobVec {
// We set len to 0 _before_ dropping elements for unwind safety. This ensures we don't
// accidentally drop elements twice in the event of a drop impl panicking.
self.len = 0;
let drop = self.drop;
let layout_size = self.item_layout.size();
for i in 0..len {
unsafe {
// NOTE: this doesn't use self.get_unchecked(i) because the debug_assert on index
// will panic here due to self.len being set to 0
let ptr = self.get_ptr_mut().byte_add(i * layout_size).promote();
(drop)(ptr);
if let Some(drop) = self.drop {
let layout_size = self.item_layout.size();
for i in 0..len {
unsafe {
// NOTE: this doesn't use self.get_unchecked(i) because the debug_assert on index
// will panic here due to self.len being set to 0
let ptr = self.get_ptr_mut().byte_add(i * layout_size).promote();
(drop)(ptr);
}
}
}
}
Expand Down Expand Up @@ -375,8 +385,8 @@ mod tests {
#[test]
fn resize_test() {
let item_layout = Layout::new::<usize>();
let drop = drop_ptr::<usize>;
let mut blob_vec = unsafe { BlobVec::new(item_layout, drop, 64) };
// usize doesn't need dropping
DJMcNab marked this conversation as resolved.
Show resolved Hide resolved
let mut blob_vec = unsafe { BlobVec::new(item_layout, None, 64) };
unsafe {
for i in 0..1_000 {
push(&mut blob_vec, i as usize);
Expand Down Expand Up @@ -406,7 +416,7 @@ mod tests {
{
let item_layout = Layout::new::<Foo>();
let drop = drop_ptr::<Foo>;
let mut blob_vec = unsafe { BlobVec::new(item_layout, drop, 2) };
let mut blob_vec = unsafe { BlobVec::new(item_layout, Some(drop), 2) };
assert_eq!(blob_vec.capacity(), 2);
unsafe {
let foo1 = Foo {
Expand Down Expand Up @@ -466,6 +476,6 @@ mod tests {
fn blob_vec_drop_empty_capacity() {
let item_layout = Layout::new::<Foo>();
let drop = drop_ptr::<Foo>;
let _ = unsafe { BlobVec::new(item_layout, drop, 0) };
let _ = unsafe { BlobVec::new(item_layout, Some(drop), 0) };
}
}
5 changes: 5 additions & 0 deletions crates/bevy_render/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,11 @@ impl Plugin for RenderPlugin {
.get_stage_mut::<SystemStage>(&RenderStage::Cleanup)
.unwrap();
cleanup.run(&mut render_app.world);
}
{
#[cfg(feature = "trace")]
let _stage_span =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjorn3 I've left this in for now, as I'm struggling to find a compelling argument for running Cleanup and clearing the entities to have no differentiation. Perhaps it should be nested, but that's a different question to what you were requesting.

bevy_utils::tracing::info_span!("stage", name = "clear_entities").entered();

render_app.world.clear_entities();
}
Expand Down