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

Implement Drop for CommandQueue #10746

Merged
merged 4 commits into from
Nov 27, 2023
Merged
Changes from all 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
53 changes: 48 additions & 5 deletions crates/bevy_ecs/src/system/commands/command_queue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@ struct CommandMeta {
/// SAFETY: The `value` must point to a value of type `T: Command`,
/// where `T` is some specific type that was used to produce this metadata.
///
/// `world` is optional to allow this one function pointer to perform double-duty as a drop.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this to ensure CommandMeta is still exactly 8 bytes in size. Since the inner Drop implementation for C is specific to C, this is the only way to capture it without making CommandMeta at least 16 bytes large.

///
/// Returns the size of `T` in bytes.
apply_command_and_get_size: unsafe fn(value: OwningPtr<Unaligned>, world: &mut World) -> usize,
consume_command_and_get_size:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed this field for clarity.

unsafe fn(value: OwningPtr<Unaligned>, world: &mut Option<&mut World>) -> usize,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using &mut Option<&mut World> to allow for the return of the Option<&mut World> back to the CommandQueue after consume_command_and_get_size is done with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid the extra &mut by reborrowing the &mut World before calling consume_command_and_get_size, for example by passing world.as_deref_mut()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reduced the usage of the &mut Option<&mut World>, but I can't fully eliminate it. CommandQueue itself is only passed a mutable reference to World, and I don't see a way to convert &mut World into &mut Option<World>. In fact, I think such a transform is nonsensical.

}

/// Densely and efficiently stores a queue of heterogenous types implementing [`Command`].
Expand Down Expand Up @@ -53,11 +56,16 @@ impl CommandQueue {
}

let meta = CommandMeta {
apply_command_and_get_size: |command, world| {
// SAFETY: According to the invariants of `CommandMeta.apply_command_and_get_size`,
consume_command_and_get_size: |command, world| {
// SAFETY: According to the invariants of `CommandMeta.consume_command_and_get_size`,
// `command` must point to a value of type `C`.
let command: C = unsafe { command.read_unaligned() };
command.apply(world);
match world {
// Apply command to the provided world...
Some(world) => command.apply(world),
// ...or discard it.
None => drop(command),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An explicit drop call is not required, since it will be implicitly called once the scope of this closure ends. I've added it here purely for clarity of purpose.

}
std::mem::size_of::<C>()
},
};
Expand Down Expand Up @@ -97,6 +105,14 @@ impl CommandQueue {
// flush the previously queued entities
world.flush();

self.apply_or_drop_queued(Some(world));
}

/// If `world` is [`Some`], this will apply the queued [commands](`Command`).
/// If `world` is [`None`], this will drop the queued [commands](`Command`) (without applying them).
/// This clears the queue.
#[inline]
fn apply_or_drop_queued(&mut self, mut world: Option<&mut World>) {
// The range of pointers of the filled portion of `self.bytes`.
let bytes_range = self.bytes.as_mut_ptr_range();

Expand Down Expand Up @@ -127,7 +143,7 @@ impl CommandQueue {
// SAFETY: The data underneath the cursor must correspond to the type erased in metadata,
// since they were stored next to each other by `.push()`.
// For ZSTs, the type doesn't matter as long as the pointer is non-null.
let size = unsafe { (meta.apply_command_and_get_size)(cmd, world) };
let size = unsafe { (meta.consume_command_and_get_size)(cmd, &mut world) };
// Advance the cursor past the command. For ZSTs, the cursor will not move.
// At this point, it will either point to the next `CommandMeta`,
// or the cursor will be out of bounds and the loop will end.
Expand All @@ -143,6 +159,12 @@ impl CommandQueue {
}
}

impl Drop for CommandQueue {
fn drop(&mut self) {
self.apply_or_drop_queued(None);
}
}

#[cfg(test)]
mod test {
use super::*;
Expand Down Expand Up @@ -193,6 +215,27 @@ mod test {
assert_eq!(drops_b.load(Ordering::Relaxed), 1);
}

/// Asserts that inner [commands](`Command`) are dropped on early drop of [`CommandQueue`].
/// Originally identified as an issue in [#10676](https://github.com/bevyengine/bevy/issues/10676)
#[test]
fn test_command_queue_inner_drop_early() {
let mut queue = CommandQueue::default();

let (dropcheck_a, drops_a) = DropCheck::new();
let (dropcheck_b, drops_b) = DropCheck::new();

queue.push(dropcheck_a);
queue.push(dropcheck_b);

assert_eq!(drops_a.load(Ordering::Relaxed), 0);
assert_eq!(drops_b.load(Ordering::Relaxed), 0);

drop(queue);

assert_eq!(drops_a.load(Ordering::Relaxed), 1);
assert_eq!(drops_b.load(Ordering::Relaxed), 1);
}

struct SpawnCommand;

impl Command for SpawnCommand {
Expand Down