From 3a4a850de7e6b39898063c28d6cd80574755266d Mon Sep 17 00:00:00 2001 From: Nathan Ward Date: Fri, 16 Jul 2021 19:57:20 +0000 Subject: [PATCH] [ecs] Improve `Commands` performance (#2332) # Objective - Currently `Commands` are quite slow due to the need to allocate for each command and wrap it in a `Box`. - For example: ```rust fn my_system(mut cmds: Commands) { cmds.spawn().insert(42).insert(3.14); } ``` will have 3 separate `Box` that need to be allocated and ran. ## Solution - Utilize a specialized data structure keyed `CommandQueueInner`. - The purpose of `CommandQueueInner` is to hold a collection of commands in contiguous memory. - This allows us to store each `Command` type contiguously in memory and quickly iterate through them and apply the `Command::write` trait function to each element. --- benches/benches/bevy_ecs/commands.rs | 4 +- .../src/system/commands/command_queue.rs | 237 ++++++++++++++++++ .../system/{commands.rs => commands/mod.rs} | 52 +--- crates/bevy_scene/src/command.rs | 4 +- .../src/hierarchy/child_builder.rs | 4 +- .../bevy_transform/src/hierarchy/hierarchy.rs | 2 +- 6 files changed, 257 insertions(+), 46 deletions(-) create mode 100644 crates/bevy_ecs/src/system/commands/command_queue.rs rename crates/bevy_ecs/src/system/{commands.rs => commands/mod.rs} (92%) diff --git a/benches/benches/bevy_ecs/commands.rs b/benches/benches/bevy_ecs/commands.rs index 57ba249f581fa8..d120589702903e 100644 --- a/benches/benches/bevy_ecs/commands.rs +++ b/benches/benches/bevy_ecs/commands.rs @@ -80,14 +80,14 @@ struct FakeCommandA; struct FakeCommandB(u64); impl Command for FakeCommandA { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { black_box(self); black_box(world); } } impl Command for FakeCommandB { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { black_box(self); black_box(world); } diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs new file mode 100644 index 00000000000000..64a3bd7914a6fe --- /dev/null +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -0,0 +1,237 @@ +use super::Command; +use crate::world::World; + +struct CommandMeta { + offset: usize, + func: unsafe fn(value: *mut u8, world: &mut World), +} + +/// A queue of [`Command`]s +// +// NOTE: [`CommandQueue`] is implemented via a `Vec` over a `Vec>` +// as an optimization. Since commands are used frequently in systems as a way to spawn +// entities/components/resources, and it's not currently possible to parallelize these +// due to mutable [`World`] access, maximizing performance for [`CommandQueue`] is +// preferred to simplicity of implementation. +#[derive(Default)] +pub struct CommandQueue { + bytes: Vec, + metas: Vec, +} + +// SAFE: All commands [`Command`] implement [`Send`] +unsafe impl Send for CommandQueue {} + +// SAFE: `&CommandQueue` never gives access to the inner commands. +unsafe impl Sync for CommandQueue {} + +impl CommandQueue { + /// Push a [`Command`] onto the queue. + #[inline] + pub fn push(&mut self, command: C) + where + C: Command, + { + /// SAFE: This function is only every called when the `command` bytes is the associated + /// [`Commands`] `T` type. Also this only reads the data via `read_unaligned` so unaligned + /// accesses are safe. + unsafe fn write_command(command: *mut u8, world: &mut World) { + let command = command.cast::().read_unaligned(); + command.write(world); + } + + let size = std::mem::size_of::(); + let old_len = self.bytes.len(); + + self.metas.push(CommandMeta { + offset: old_len, + func: write_command::, + }); + + if size > 0 { + self.bytes.reserve(size); + + // SAFE: The internal `bytes` vector has enough storage for the + // command (see the call the `reserve` above), and the vector has + // its length set appropriately. + // Also `command` is forgotten at the end of this function so that + // when `apply` is called later, a double `drop` does not occur. + unsafe { + std::ptr::copy_nonoverlapping( + &command as *const C as *const u8, + self.bytes.as_mut_ptr().add(old_len), + size, + ); + self.bytes.set_len(old_len + size); + } + } + + std::mem::forget(command); + } + + /// Execute the queued [`Command`]s in the world. + /// This clears the queue. + #[inline] + pub fn apply(&mut self, world: &mut World) { + // flush the previously queued entities + world.flush(); + + // SAFE: In the iteration below, `meta.func` will safely consume and drop each pushed command. + // This operation is so that we can reuse the bytes `Vec`'s internal storage and prevent + // unnecessary allocations. + unsafe { self.bytes.set_len(0) }; + + let byte_ptr = if self.bytes.as_mut_ptr().is_null() { + // SAFE: If the vector's buffer pointer is `null` this mean nothing has been pushed to its bytes. + // This means either that: + // + // 1) There are no commands so this pointer will never be read/written from/to. + // + // 2) There are only zero-sized commands pushed. + // According to https://doc.rust-lang.org/std/ptr/index.html + // "The canonical way to obtain a pointer that is valid for zero-sized accesses is NonNull::dangling" + // therefore it is safe to call `read_unaligned` on a pointer produced from `NonNull::dangling` for + // zero-sized commands. + unsafe { std::ptr::NonNull::dangling().as_mut() } + } else { + self.bytes.as_mut_ptr() + }; + + for meta in self.metas.drain(..) { + // SAFE: The implementation of `write_command` is safe for the according Command type. + // The bytes are safely cast to their original type, safely read, and then dropped. + unsafe { + (meta.func)(byte_ptr.add(meta.offset), world); + } + } + } +} + +#[cfg(test)] +mod test { + use super::*; + use std::{ + panic::AssertUnwindSafe, + sync::{ + atomic::{AtomicU32, Ordering}, + Arc, + }, + }; + + struct DropCheck(Arc); + + impl DropCheck { + fn new() -> (Self, Arc) { + let drops = Arc::new(AtomicU32::new(0)); + (Self(drops.clone()), drops) + } + } + + impl Drop for DropCheck { + fn drop(&mut self) { + self.0.fetch_add(1, Ordering::Relaxed); + } + } + + impl Command for DropCheck { + fn write(self, _: &mut World) {} + } + + #[test] + fn test_command_queue_inner_drop() { + 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); + + let mut world = World::new(); + queue.apply(&mut world); + + assert_eq!(drops_a.load(Ordering::Relaxed), 1); + assert_eq!(drops_b.load(Ordering::Relaxed), 1); + } + + struct SpawnCommand; + + impl Command for SpawnCommand { + fn write(self, world: &mut World) { + world.spawn(); + } + } + + #[test] + fn test_command_queue_inner() { + let mut queue = CommandQueue::default(); + + queue.push(SpawnCommand); + queue.push(SpawnCommand); + + let mut world = World::new(); + queue.apply(&mut world); + + assert_eq!(world.entities().len(), 2); + + // The previous call to `apply` cleared the queue. + // This call should do nothing. + queue.apply(&mut world); + assert_eq!(world.entities().len(), 2); + } + + // This has an arbitrary value `String` stored to ensure + // when then command gets pushed, the `bytes` vector gets + // some data added to it. + struct PanicCommand(String); + impl Command for PanicCommand { + fn write(self, _: &mut World) { + panic!("command is panicking"); + } + } + + #[test] + fn test_command_queue_inner_panic_safe() { + std::panic::set_hook(Box::new(|_| {})); + + let mut queue = CommandQueue::default(); + + queue.push(PanicCommand("I panic!".to_owned())); + queue.push(SpawnCommand); + + let mut world = World::new(); + + let _ = std::panic::catch_unwind(AssertUnwindSafe(|| { + queue.apply(&mut world); + })); + + // even though the first command panicking. + // the `bytes`/`metas` vectors were cleared. + assert_eq!(queue.bytes.len(), 0); + assert_eq!(queue.metas.len(), 0); + + // Even though the first command panicked, it's still ok to push + // more commands. + queue.push(SpawnCommand); + queue.push(SpawnCommand); + queue.apply(&mut world); + assert_eq!(world.entities().len(), 2); + } + + // NOTE: `CommandQueue` is `Send` because `Command` is send. + // If the `Command` trait gets reworked to be non-send, `CommandQueue` + // should be reworked. + // This test asserts that Command types are send. + fn assert_is_send_impl(_: impl Send) {} + fn assert_is_send(command: impl Command) { + assert_is_send_impl(command); + } + + #[test] + fn test_command_is_send() { + assert_is_send(SpawnCommand); + } +} diff --git a/crates/bevy_ecs/src/system/commands.rs b/crates/bevy_ecs/src/system/commands/mod.rs similarity index 92% rename from crates/bevy_ecs/src/system/commands.rs rename to crates/bevy_ecs/src/system/commands/mod.rs index 74b69bd2fbd28c..0488b09e4f770c 100644 --- a/crates/bevy_ecs/src/system/commands.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -1,3 +1,5 @@ +mod command_queue; + use crate::{ bundle::Bundle, component::Component, @@ -5,40 +7,12 @@ use crate::{ world::World, }; use bevy_utils::tracing::debug; +pub use command_queue::CommandQueue; use std::marker::PhantomData; /// A [`World`] mutation. pub trait Command: Send + Sync + 'static { - fn write(self: Box, world: &mut World); -} - -/// A queue of [`Command`]s. -#[derive(Default)] -pub struct CommandQueue { - commands: Vec>, -} - -impl CommandQueue { - /// Execute the queued [`Command`]s in the world. - /// This clears the queue. - pub fn apply(&mut self, world: &mut World) { - world.flush(); - for command in self.commands.drain(..) { - command.write(world); - } - } - - /// Push a boxed [`Command`] onto the queue. - #[inline] - pub fn push_boxed(&mut self, command: Box) { - self.commands.push(command); - } - - /// Push a [`Command`] onto the queue. - #[inline] - pub fn push(&mut self, command: T) { - self.push_boxed(Box::new(command)); - } + fn write(self, world: &mut World); } /// A list of commands that will be run to modify a [`World`]. @@ -292,7 +266,7 @@ impl Command for Spawn where T: Bundle, { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { world.spawn().insert_bundle(self.bundle); } } @@ -310,7 +284,7 @@ where I: IntoIterator + Send + Sync + 'static, I::Item: Bundle, { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { world.spawn_batch(self.bundles_iter); } } @@ -321,7 +295,7 @@ pub struct Despawn { } impl Command for Despawn { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { if !world.despawn(self.entity) { debug!("Failed to despawn non-existent entity {:?}", self.entity); } @@ -337,7 +311,7 @@ impl Command for InsertBundle where T: Bundle + 'static, { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { world.entity_mut(self.entity).insert_bundle(self.bundle); } } @@ -352,7 +326,7 @@ impl Command for Insert where T: Component, { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { world.entity_mut(self.entity).insert(self.component); } } @@ -367,7 +341,7 @@ impl Command for Remove where T: Component, { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { if let Some(mut entity_mut) = world.get_entity_mut(self.entity) { entity_mut.remove::(); } @@ -384,7 +358,7 @@ impl Command for RemoveBundle where T: Bundle, { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { if let Some(mut entity_mut) = world.get_entity_mut(self.entity) { // remove intersection to gracefully handle components that were removed before running // this command @@ -398,7 +372,7 @@ pub struct InsertResource { } impl Command for InsertResource { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { world.insert_resource(self.resource); } } @@ -408,7 +382,7 @@ pub struct RemoveResource { } impl Command for RemoveResource { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { world.remove_resource::(); } } diff --git a/crates/bevy_scene/src/command.rs b/crates/bevy_scene/src/command.rs index f7d04c60652c4e..42554e91793df3 100644 --- a/crates/bevy_scene/src/command.rs +++ b/crates/bevy_scene/src/command.rs @@ -13,7 +13,7 @@ pub struct SpawnScene { } impl Command for SpawnScene { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { let mut spawner = world.get_resource_mut::().unwrap(); spawner.spawn(self.scene_handle); } @@ -35,7 +35,7 @@ pub struct SpawnSceneAsChild { } impl Command for SpawnSceneAsChild { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { let mut spawner = world.get_resource_mut::().unwrap(); spawner.spawn_as_child(self.scene_handle, self.parent); } diff --git a/crates/bevy_transform/src/hierarchy/child_builder.rs b/crates/bevy_transform/src/hierarchy/child_builder.rs index 1c1ab208eefa4e..9731c8c02b0e53 100644 --- a/crates/bevy_transform/src/hierarchy/child_builder.rs +++ b/crates/bevy_transform/src/hierarchy/child_builder.rs @@ -15,7 +15,7 @@ pub struct InsertChildren { } impl Command for InsertChildren { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { for child in self.children.iter() { world .entity_mut(*child) @@ -46,7 +46,7 @@ pub struct ChildBuilder<'a, 'b> { } impl Command for PushChildren { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { for child in self.children.iter() { world .entity_mut(*child) diff --git a/crates/bevy_transform/src/hierarchy/hierarchy.rs b/crates/bevy_transform/src/hierarchy/hierarchy.rs index 9c8f4daca2ac1d..d50f241e43d747 100644 --- a/crates/bevy_transform/src/hierarchy/hierarchy.rs +++ b/crates/bevy_transform/src/hierarchy/hierarchy.rs @@ -37,7 +37,7 @@ fn despawn_with_children_recursive_inner(world: &mut World, entity: Entity) { } impl Command for DespawnRecursive { - fn write(self: Box, world: &mut World) { + fn write(self, world: &mut World) { despawn_with_children_recursive(world, self.entity); } }