From 3bf9f6c4182eb6317c3659b5f5412ac6f7f3bb0d Mon Sep 17 00:00:00 2001 From: NathanW Date: Wed, 9 Jun 2021 13:27:12 -0600 Subject: [PATCH 01/25] [ecs] Do not allocate each `Command` Rework CommandQueue to not allocate for each command --- crates/bevy_ecs/src/system/commands.rs | 58 +++-- crates/bevy_scene/src/command.rs | 4 +- .../src/hierarchy/child_builder.rs | 4 +- .../bevy_transform/src/hierarchy/hierarchy.rs | 2 +- crates/bevy_utils/src/anystack.rs | 218 ++++++++++++++++++ crates/bevy_utils/src/lib.rs | 1 + 6 files changed, 261 insertions(+), 26 deletions(-) create mode 100644 crates/bevy_utils/src/anystack.rs diff --git a/crates/bevy_ecs/src/system/commands.rs b/crates/bevy_ecs/src/system/commands.rs index 74b69bd2fbd28..f675ab19c141e 100644 --- a/crates/bevy_ecs/src/system/commands.rs +++ b/crates/bevy_ecs/src/system/commands.rs @@ -4,18 +4,28 @@ use crate::{ entity::{Entities, Entity}, world::World, }; -use bevy_utils::tracing::debug; +use bevy_utils::{anystack::AnyStack, tracing::debug}; use std::marker::PhantomData; /// A [`World`] mutation. pub trait Command: Send + Sync + 'static { - fn write(self: Box, world: &mut World); + fn write(self, world: &mut World); +} + +/// # Safety +/// +/// This function is only used when the provided `world` is a `&mut World` and +/// is only ever called once, so it's safe to consume `command`. +unsafe fn write_command_on_mut_world(command: *mut u8, world: *mut u8) { + let world = &mut *world.cast::(); + let command = command.cast::().read(); + command.write(world); } /// A queue of [`Command`]s. #[derive(Default)] pub struct CommandQueue { - commands: Vec>, + commands: AnyStack, } impl CommandQueue { @@ -23,21 +33,27 @@ impl CommandQueue { /// This clears the queue. pub fn apply(&mut self, world: &mut World) { world.flush(); - for command in self.commands.drain(..) { - command.write(world); + // SAFE: + // `apply`: the provided function `write_command_on_mut_world` safely + // handle the provided [`World`], drops the associate `Command`, and + // clears the inner [`AnyStack`]. + // + // `clear`: is safely used because the call to `apply` above + // ensures each added command is dropped. + unsafe { + self.commands.apply(world as *mut World as *mut u8); + self.commands.clear(); } } - /// 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)); + // SAFE: `write_command_on_mut_world` safely casts `command` back to its original type + // and safely casts the `user_data` back to a `World`. + unsafe { + self.commands.push(command, write_command_on_mut_world::); + } } } @@ -292,7 +308,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 +326,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 +337,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 +353,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 +368,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 +383,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 +400,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 +414,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 +424,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 f7d04c60652c4..42554e91793df 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 1c1ab208eefa4..9731c8c02b0e5 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 9c8f4daca2ac1..d50f241e43d74 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); } } diff --git a/crates/bevy_utils/src/anystack.rs b/crates/bevy_utils/src/anystack.rs new file mode 100644 index 0000000000000..51c9b81b8ba0a --- /dev/null +++ b/crates/bevy_utils/src/anystack.rs @@ -0,0 +1,218 @@ +struct AnyStackMeta { + offset: usize, + func: unsafe fn(value: *mut u8, user_data: *mut u8), +} + +pub struct AnyStack { + bytes: Vec, + metas: Vec, +} + +impl Default for AnyStack { + fn default() -> Self { + Self { + bytes: vec![], + metas: vec![], + } + } +} + +unsafe impl Send for AnyStack {} +unsafe impl Sync for AnyStack {} + +impl AnyStack { + #[inline] + pub fn new() -> Self { + Self::default() + } + + #[inline] + pub fn len(&self) -> usize { + self.metas.len() + } + + #[inline] + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// Clears in internal bytes and metas storage. + /// + /// # Safety + /// + /// This does not [`drop`] the pushed values. + /// The pushed values must be dropped via [`AnyStack::apply`] and + /// the provided `func` before calling this function. + #[inline] + pub unsafe fn clear(&mut self) { + self.bytes.clear(); + self.metas.clear(); + } + + /// Push a new `value` onto the stack. + /// + /// # Safety + /// + /// `func` must safely handle the provided `value` bytes and `user_data` bytes. + /// [`AnyStack`] does not drop the contained member. + /// The user should manually call [`AnyStack::apply`] and in the implementation + /// of the provided `drop` function from [`AnyStack::push`], the element should + /// be dropped. + pub unsafe fn push( + &mut self, + value: U, + func: unsafe fn(value: *mut u8, user_data: *mut u8), + ) { + let align = std::mem::align_of::(); + let size = std::mem::size_of::(); + + if self.is_empty() { + self.bytes.reserve(size); + } + + let old_len = self.bytes.len(); + + let aligned_offset = loop { + let aligned_offset = self.bytes.as_ptr().add(old_len).align_offset(align); + + if old_len + aligned_offset + size > self.bytes.capacity() { + self.bytes.reserve(aligned_offset + size); + } else { + break aligned_offset; + } + }; + + let offset = old_len + aligned_offset; + let total_bytes = size + aligned_offset; + self.bytes.set_len(old_len + total_bytes); + + self.metas.push(AnyStackMeta { offset, func }); + + std::ptr::copy_nonoverlapping( + &value as *const U as *const u8, + self.bytes.as_mut_ptr().add(offset), + size, + ); + + std::mem::forget(value); + } + + /// Call each user `func` for each inserted value with `user_data`. + /// + /// # Safety + /// + /// It is up to the user to safely handle `user_data` in each of the initially + /// provided `func` functions in [`AnyStack::push`]. + pub unsafe fn apply(&mut self, user_data: *mut u8) { + let byte_ptr = self.bytes.as_mut_ptr(); + for meta in self.metas.iter() { + (meta.func)(byte_ptr.add(meta.offset), user_data); + } + } +} + +#[cfg(test)] +mod test { + use super::*; + use std::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); + } + } + + /// # Safety + /// + /// This function does not touch the `user_data` provided, + /// and this is only used when the value is a `DropCheck`. + /// Lastly, this drops the `DropCheck` value and is only + /// every call once. + unsafe fn drop_check_func(bytes: *mut u8, _: *mut u8) { + assert_eq!(bytes.align_offset(std::mem::align_of::()), 0); + let _ = bytes.cast::().read(); + } + + #[test] + fn test_anystack_drop() { + let mut stack = AnyStack::new(); + + let (dropcheck_a, drops_a) = DropCheck::new(); + let (dropcheck_b, drops_b) = DropCheck::new(); + + // SAFE: `noop_func` never reads/write from the provided bytes. + unsafe { + stack.push(dropcheck_a, drop_check_func); + stack.push(dropcheck_b, drop_check_func); + } + + assert_eq!(drops_a.load(Ordering::Relaxed), 0); + assert_eq!(drops_b.load(Ordering::Relaxed), 0); + + // SAFE: The `drop_check_func` does not access the null `user_data`. + unsafe { + stack.apply(std::ptr::null_mut()); + } + + assert_eq!(drops_a.load(Ordering::Relaxed), 1); + assert_eq!(drops_b.load(Ordering::Relaxed), 1); + } + + struct FakeWorld(u32); + + /// # Safety + /// `bytes` must point to a valid `u32` + /// `world` must point to a mutable `FakeWorld`. + /// Since `u32` is a primitive type, it does not require a `drop` call. + unsafe fn increment_fake_world_u32(bytes: *mut u8, world: *mut u8) { + let world = &mut *world.cast::(); + world.0 += *bytes.cast::(); + } + + #[test] + fn test_anystack() { + let mut stack = AnyStack::new(); + + assert!(stack.is_empty()); + assert_eq!(stack.len(), 0); + + // SAFE: the provided function safely handles the `bytes` and provided `user_data`. + unsafe { + stack.push(5u32, increment_fake_world_u32); + stack.push(10u32, increment_fake_world_u32); + } + + assert!(!stack.is_empty()); + assert_eq!(stack.len(), 2); + + let mut world = FakeWorld(0); + + // SAFE: the given `user_data` is a `&mut FakeWorld` which is safely + // handled in the invocation of `increment_fake_world_u32` + unsafe { + stack.apply(&mut world as *mut FakeWorld as *mut u8); + } + + assert_eq!(world.0, 15); + + // SAFE: the data is only `u32` so they don't need to be dropped. + unsafe { + stack.clear(); + } + + assert!(stack.is_empty()); + assert_eq!(stack.len(), 0); + } +} diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index c3b78781d3271..d6fe4d273ad64 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -1,3 +1,4 @@ +pub mod anystack; mod enum_variant_meta; pub use enum_variant_meta::*; From 2654470532dc4513d01a5fc4a515876fa71d89e6 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 11 Jun 2021 16:25:00 -0600 Subject: [PATCH 02/25] fix doc typo --- crates/bevy_utils/src/anystack.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_utils/src/anystack.rs b/crates/bevy_utils/src/anystack.rs index 51c9b81b8ba0a..748cd17117fdb 100644 --- a/crates/bevy_utils/src/anystack.rs +++ b/crates/bevy_utils/src/anystack.rs @@ -56,7 +56,7 @@ impl AnyStack { /// `func` must safely handle the provided `value` bytes and `user_data` bytes. /// [`AnyStack`] does not drop the contained member. /// The user should manually call [`AnyStack::apply`] and in the implementation - /// of the provided `drop` function from [`AnyStack::push`], the element should + /// of the provided `func` function from [`AnyStack::push`], the element should /// be dropped. pub unsafe fn push( &mut self, From e0c6f835789362548815257a17278e93482cf987 Mon Sep 17 00:00:00 2001 From: NathanSWard Date: Sat, 12 Jun 2021 10:01:43 -0600 Subject: [PATCH 03/25] reduce unsafe code, and remove alignment requirements --- crates/bevy_ecs/src/system/commands.rs | 29 ++--- crates/bevy_utils/src/anystack.rs | 142 ++++++++++--------------- 2 files changed, 70 insertions(+), 101 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands.rs b/crates/bevy_ecs/src/system/commands.rs index f675ab19c141e..d4a7e649649b9 100644 --- a/crates/bevy_ecs/src/system/commands.rs +++ b/crates/bevy_ecs/src/system/commands.rs @@ -14,18 +14,18 @@ pub trait Command: Send + Sync + 'static { /// # Safety /// -/// This function is only used when the provided `world` is a `&mut World` and -/// is only ever called once, so it's safe to consume `command`. -unsafe fn write_command_on_mut_world(command: *mut u8, world: *mut u8) { - let world = &mut *world.cast::(); - let command = command.cast::().read(); +/// This function is only every associated when the `command` bytes is the associated +/// [`Commands`] `T` type. Also this only read the data via `read_unaligned` to unaligned +/// accesses are safe. +unsafe fn write_command_on_mut_world(command: *mut u8, world: &mut World) { + let command = command.cast::().read_unaligned(); command.write(world); } /// A queue of [`Command`]s. #[derive(Default)] pub struct CommandQueue { - commands: AnyStack, + commands: AnyStack, } impl CommandQueue { @@ -33,24 +33,17 @@ impl CommandQueue { /// This clears the queue. pub fn apply(&mut self, world: &mut World) { world.flush(); - // SAFE: - // `apply`: the provided function `write_command_on_mut_world` safely - // handle the provided [`World`], drops the associate `Command`, and - // clears the inner [`AnyStack`]. - // - // `clear`: is safely used because the call to `apply` above - // ensures each added command is dropped. - unsafe { - self.commands.apply(world as *mut World as *mut u8); - self.commands.clear(); - } + // Note: the provided function `write_command_on_mut_world` safely + // drops the associated `Command`. + self.commands.consume(world); } /// Push a [`Command`] onto the queue. #[inline] pub fn push(&mut self, command: T) { // SAFE: `write_command_on_mut_world` safely casts `command` back to its original type - // and safely casts the `user_data` back to a `World`. + // and only access the command via `read_unaligned`. + // Also it correctly `drops` the command. unsafe { self.commands.push(command, write_command_on_mut_world::); } diff --git a/crates/bevy_utils/src/anystack.rs b/crates/bevy_utils/src/anystack.rs index 748cd17117fdb..36d0935d6dc89 100644 --- a/crates/bevy_utils/src/anystack.rs +++ b/crates/bevy_utils/src/anystack.rs @@ -1,14 +1,14 @@ -struct AnyStackMeta { +struct AnyStackMeta { offset: usize, - func: unsafe fn(value: *mut u8, user_data: *mut u8), + func: unsafe fn(value: *mut u8, user_data: &mut T), } -pub struct AnyStack { +pub struct AnyStack { bytes: Vec, - metas: Vec, + metas: Vec>, } -impl Default for AnyStack { +impl Default for AnyStack { fn default() -> Self { Self { bytes: vec![], @@ -17,97 +17,89 @@ impl Default for AnyStack { } } -unsafe impl Send for AnyStack {} -unsafe impl Sync for AnyStack {} +// SAFE: All values pushed onto the stack are required to be [`Send`] +unsafe impl Send for AnyStack {} -impl AnyStack { +// SAFE: All values pushed onto the stack are required to be [`Sync`] +unsafe impl Sync for AnyStack {} + +impl AnyStack { + ////Constructs a new, empty `AnyStack`. + /// The stack will not allocate until elements are pushed onto it. #[inline] pub fn new() -> Self { Self::default() } + /// Returns the number of elements in the [`AnyStack`], also referred to as its ‘length’. #[inline] pub fn len(&self) -> usize { self.metas.len() } + /// Returns true if the [`AnyStack`] contains no elements. #[inline] pub fn is_empty(&self) -> bool { self.len() == 0 } - /// Clears in internal bytes and metas storage. - /// - /// # Safety - /// - /// This does not [`drop`] the pushed values. - /// The pushed values must be dropped via [`AnyStack::apply`] and - /// the provided `func` before calling this function. - #[inline] - pub unsafe fn clear(&mut self) { - self.bytes.clear(); - self.metas.clear(); - } - /// Push a new `value` onto the stack. /// /// # Safety /// - /// `func` must safely handle the provided `value` bytes and `user_data` bytes. + /// `func` must safely handle the provided `value` bytes. + /// _NOTE_: the bytes provided to the given function may **NOT** be aligned. If you wish + /// to read the value, consider using [`std::ptr::read_unalinged`]. + /// /// [`AnyStack`] does not drop the contained member. - /// The user should manually call [`AnyStack::apply`] and in the implementation - /// of the provided `func` function from [`AnyStack::push`], the element should - /// be dropped. - pub unsafe fn push( - &mut self, - value: U, - func: unsafe fn(value: *mut u8, user_data: *mut u8), - ) { - let align = std::mem::align_of::(); + /// The user should manually call [`AnyStack::consume`] and in the implementation + /// of the provided `func` the element should be dropped. + #[inline] + pub unsafe fn push(&mut self, value: U, func: unsafe fn(value: *mut u8, user_data: &mut T)) + where + U: Send + Sync, + { let size = std::mem::size_of::(); - if self.is_empty() { - self.bytes.reserve(size); - } - let old_len = self.bytes.len(); - let aligned_offset = loop { - let aligned_offset = self.bytes.as_ptr().add(old_len).align_offset(align); - - if old_len + aligned_offset + size > self.bytes.capacity() { - self.bytes.reserve(aligned_offset + size); - } else { - break aligned_offset; - } - }; - - let offset = old_len + aligned_offset; - let total_bytes = size + aligned_offset; - self.bytes.set_len(old_len + total_bytes); - - self.metas.push(AnyStackMeta { offset, func }); - + self.bytes.reserve(size); std::ptr::copy_nonoverlapping( &value as *const U as *const u8, - self.bytes.as_mut_ptr().add(offset), + self.bytes.as_mut_ptr().add(old_len), size, ); + self.bytes.set_len(old_len + size); + + self.metas.push(AnyStackMeta { + offset: old_len, + func, + }); std::mem::forget(value); } - /// Call each user `func` for each inserted value with `user_data`. + /// Call each user `func` for each inserted value with `user_data` + /// and then clears the internal bytes/metas vectors. /// - /// # Safety + /// # Warning /// - /// It is up to the user to safely handle `user_data` in each of the initially - /// provided `func` functions in [`AnyStack::push`]. - pub unsafe fn apply(&mut self, user_data: *mut u8) { + /// This does not [`drop`] the pushed values. + /// If the value should be dropped, the initially provided `func` + /// should ensure any necessary cleanup occurs. + pub fn consume(&mut self, user_data: &mut T) { let byte_ptr = self.bytes.as_mut_ptr(); for meta in self.metas.iter() { - (meta.func)(byte_ptr.add(meta.offset), user_data); + // SAFE: The safety guarantees are promised to be held by the caller + // from [`AnyStack::push`]. + // Also each value has it's offset correctly stored in it's assocaited meta. + unsafe { + (meta.func)(byte_ptr.add(meta.offset), user_data); + } } + + self.bytes.clear(); + self.metas.clear(); } } @@ -136,18 +128,16 @@ mod test { /// # Safety /// - /// This function does not touch the `user_data` provided, - /// and this is only used when the value is a `DropCheck`. - /// Lastly, this drops the `DropCheck` value and is only + /// This function is only used when the value is a `DropCheck`. + /// This also drops the `DropCheck` value and is only /// every call once. - unsafe fn drop_check_func(bytes: *mut u8, _: *mut u8) { - assert_eq!(bytes.align_offset(std::mem::align_of::()), 0); - let _ = bytes.cast::().read(); + unsafe fn drop_check_func(bytes: *mut u8, _: &mut ()) { + let _ = bytes.cast::().read_unaligned(); } #[test] fn test_anystack_drop() { - let mut stack = AnyStack::new(); + let mut stack = AnyStack::<()>::new(); let (dropcheck_a, drops_a) = DropCheck::new(); let (dropcheck_b, drops_b) = DropCheck::new(); @@ -161,10 +151,7 @@ mod test { assert_eq!(drops_a.load(Ordering::Relaxed), 0); assert_eq!(drops_b.load(Ordering::Relaxed), 0); - // SAFE: The `drop_check_func` does not access the null `user_data`. - unsafe { - stack.apply(std::ptr::null_mut()); - } + stack.consume(&mut ()); assert_eq!(drops_a.load(Ordering::Relaxed), 1); assert_eq!(drops_b.load(Ordering::Relaxed), 1); @@ -174,11 +161,9 @@ mod test { /// # Safety /// `bytes` must point to a valid `u32` - /// `world` must point to a mutable `FakeWorld`. /// Since `u32` is a primitive type, it does not require a `drop` call. - unsafe fn increment_fake_world_u32(bytes: *mut u8, world: *mut u8) { - let world = &mut *world.cast::(); - world.0 += *bytes.cast::(); + unsafe fn increment_fake_world_u32(bytes: *mut u8, world: &mut FakeWorld) { + world.0 += bytes.cast::().read_unaligned(); } #[test] @@ -199,19 +184,10 @@ mod test { let mut world = FakeWorld(0); - // SAFE: the given `user_data` is a `&mut FakeWorld` which is safely - // handled in the invocation of `increment_fake_world_u32` - unsafe { - stack.apply(&mut world as *mut FakeWorld as *mut u8); - } + stack.consume(&mut world); assert_eq!(world.0, 15); - // SAFE: the data is only `u32` so they don't need to be dropped. - unsafe { - stack.clear(); - } - assert!(stack.is_empty()); assert_eq!(stack.len(), 0); } From 0578d09ccfc8a16186388eecd11b3425bec9f228 Mon Sep 17 00:00:00 2001 From: NathanSWard Date: Sat, 12 Jun 2021 10:21:00 -0600 Subject: [PATCH 04/25] doc typos --- crates/bevy_ecs/src/system/commands.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands.rs b/crates/bevy_ecs/src/system/commands.rs index d4a7e649649b9..a4c99b012dc46 100644 --- a/crates/bevy_ecs/src/system/commands.rs +++ b/crates/bevy_ecs/src/system/commands.rs @@ -14,8 +14,8 @@ pub trait Command: Send + Sync + 'static { /// # Safety /// -/// This function is only every associated when the `command` bytes is the associated -/// [`Commands`] `T` type. Also this only read the data via `read_unaligned` to unaligned +/// 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_on_mut_world(command: *mut u8, world: &mut World) { let command = command.cast::().read_unaligned(); From bda5cac2f025a9eb663c50e7a4a385a67ec995e3 Mon Sep 17 00:00:00 2001 From: NathanW Date: Sat, 12 Jun 2021 20:42:59 -0600 Subject: [PATCH 05/25] move anystack to implementation detail. rename to command-queue-inner --- .../src/system/commands/command_queue.rs | 184 +++++++++++++++++ .../system/{commands.rs => commands/mod.rs} | 26 +-- crates/bevy_utils/src/anystack.rs | 193 ------------------ 3 files changed, 191 insertions(+), 212 deletions(-) create mode 100644 crates/bevy_ecs/src/system/commands/command_queue.rs rename crates/bevy_ecs/src/system/{commands.rs => commands/mod.rs} (94%) 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 0000000000000..aaa30807c1baa --- /dev/null +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -0,0 +1,184 @@ +use super::Command; +use crate::world::World; + +/// # Safety +/// +/// 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 invoke_command(command: *mut u8, world: &mut World) { + let command = command.cast::().read_unaligned(); + command.write(world); +} + +struct CommandMeta { + offset: usize, + func: unsafe fn(value: *mut u8, world: &mut World), +} + +#[derive(Default)] +pub(crate) struct CommandQueueInner { + bytes: Vec, + metas: Vec, +} + +// SAFE: All commands [`Command`] implement [`Send`] +unsafe impl Send for CommandQueueInner {} + +// SAFE: All commands [`Command`] implement [`Sync`] +unsafe impl Sync for CommandQueueInner {} + +impl CommandQueueInner { + ////Constructs a new, empty `CommandQueueInner`. + /// The stack will not allocate until commands are pushed onto it. + #[inline] + pub fn new() -> Self { + Self::default() + } + + /// Returns the number of commands in the [`CommandQueueInner`], also referred to as its ‘length’. + #[inline] + pub fn len(&self) -> usize { + self.metas.len() + } + + /// Returns true if the [`CommandQueueInner`] contains no elements. + #[inline] + pub fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// Push a new `command` onto the queue. + #[inline] + pub fn push(&mut self, command: C) + where + C: Command, + { + let size = std::mem::size_of::(); + + let old_len = self.bytes.len(); + + 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); + } + + self.metas.push(CommandMeta { + offset: old_len, + func: invoke_command::, + }); + + std::mem::forget(command); + } + + /// Invoke each command `func` for each inserted value with `world` + /// and then clears the internal bytes/metas vectors. + /// + /// # Warning + /// + /// This does not [`drop`] the pushed commands. + /// If the command should be dropped, the initially provided `func` + /// should ensure any necessary cleanup occurs. + pub fn apply(&mut self, world: &mut World) { + let byte_ptr = self.bytes.as_mut_ptr(); + for meta in self.metas.iter() { + // The implementation of `invoke_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); + } + } + + self.bytes.clear(); + self.metas.clear(); + } +} + +#[cfg(test)] +mod test { + use super::*; + use std::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 = CommandQueueInner::new(); + + 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 = CommandQueueInner::new(); + + assert!(queue.is_empty()); + assert_eq!(queue.len(), 0); + + queue.push(SpawnCommand); + queue.push(SpawnCommand); + + assert!(!queue.is_empty()); + assert_eq!(queue.len(), 2); + + let mut world = World::new(); + queue.apply(&mut world); + + assert_eq!(world.entities().len(), 2); + + assert!(queue.is_empty()); + assert_eq!(queue.len(), 0); + } +} diff --git a/crates/bevy_ecs/src/system/commands.rs b/crates/bevy_ecs/src/system/commands/mod.rs similarity index 94% rename from crates/bevy_ecs/src/system/commands.rs rename to crates/bevy_ecs/src/system/commands/mod.rs index a4c99b012dc46..25754d367bb8a 100644 --- a/crates/bevy_ecs/src/system/commands.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -1,10 +1,13 @@ +mod command_queue; + use crate::{ bundle::Bundle, component::Component, entity::{Entities, Entity}, world::World, }; -use bevy_utils::{anystack::AnyStack, tracing::debug}; +use bevy_utils::tracing::debug; +use command_queue::CommandQueueInner; use std::marker::PhantomData; /// A [`World`] mutation. @@ -12,20 +15,10 @@ pub trait Command: Send + Sync + 'static { fn write(self, world: &mut World); } -/// # Safety -/// -/// 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_on_mut_world(command: *mut u8, world: &mut World) { - let command = command.cast::().read_unaligned(); - command.write(world); -} - /// A queue of [`Command`]s. #[derive(Default)] pub struct CommandQueue { - commands: AnyStack, + commands: CommandQueueInner, } impl CommandQueue { @@ -35,18 +28,13 @@ impl CommandQueue { world.flush(); // Note: the provided function `write_command_on_mut_world` safely // drops the associated `Command`. - self.commands.consume(world); + self.commands.apply(world); } /// Push a [`Command`] onto the queue. #[inline] pub fn push(&mut self, command: T) { - // SAFE: `write_command_on_mut_world` safely casts `command` back to its original type - // and only access the command via `read_unaligned`. - // Also it correctly `drops` the command. - unsafe { - self.commands.push(command, write_command_on_mut_world::); - } + self.commands.push(command); } } diff --git a/crates/bevy_utils/src/anystack.rs b/crates/bevy_utils/src/anystack.rs index 36d0935d6dc89..8b137891791fe 100644 --- a/crates/bevy_utils/src/anystack.rs +++ b/crates/bevy_utils/src/anystack.rs @@ -1,194 +1 @@ -struct AnyStackMeta { - offset: usize, - func: unsafe fn(value: *mut u8, user_data: &mut T), -} -pub struct AnyStack { - bytes: Vec, - metas: Vec>, -} - -impl Default for AnyStack { - fn default() -> Self { - Self { - bytes: vec![], - metas: vec![], - } - } -} - -// SAFE: All values pushed onto the stack are required to be [`Send`] -unsafe impl Send for AnyStack {} - -// SAFE: All values pushed onto the stack are required to be [`Sync`] -unsafe impl Sync for AnyStack {} - -impl AnyStack { - ////Constructs a new, empty `AnyStack`. - /// The stack will not allocate until elements are pushed onto it. - #[inline] - pub fn new() -> Self { - Self::default() - } - - /// Returns the number of elements in the [`AnyStack`], also referred to as its ‘length’. - #[inline] - pub fn len(&self) -> usize { - self.metas.len() - } - - /// Returns true if the [`AnyStack`] contains no elements. - #[inline] - pub fn is_empty(&self) -> bool { - self.len() == 0 - } - - /// Push a new `value` onto the stack. - /// - /// # Safety - /// - /// `func` must safely handle the provided `value` bytes. - /// _NOTE_: the bytes provided to the given function may **NOT** be aligned. If you wish - /// to read the value, consider using [`std::ptr::read_unalinged`]. - /// - /// [`AnyStack`] does not drop the contained member. - /// The user should manually call [`AnyStack::consume`] and in the implementation - /// of the provided `func` the element should be dropped. - #[inline] - pub unsafe fn push(&mut self, value: U, func: unsafe fn(value: *mut u8, user_data: &mut T)) - where - U: Send + Sync, - { - let size = std::mem::size_of::(); - - let old_len = self.bytes.len(); - - self.bytes.reserve(size); - std::ptr::copy_nonoverlapping( - &value as *const U as *const u8, - self.bytes.as_mut_ptr().add(old_len), - size, - ); - self.bytes.set_len(old_len + size); - - self.metas.push(AnyStackMeta { - offset: old_len, - func, - }); - - std::mem::forget(value); - } - - /// Call each user `func` for each inserted value with `user_data` - /// and then clears the internal bytes/metas vectors. - /// - /// # Warning - /// - /// This does not [`drop`] the pushed values. - /// If the value should be dropped, the initially provided `func` - /// should ensure any necessary cleanup occurs. - pub fn consume(&mut self, user_data: &mut T) { - let byte_ptr = self.bytes.as_mut_ptr(); - for meta in self.metas.iter() { - // SAFE: The safety guarantees are promised to be held by the caller - // from [`AnyStack::push`]. - // Also each value has it's offset correctly stored in it's assocaited meta. - unsafe { - (meta.func)(byte_ptr.add(meta.offset), user_data); - } - } - - self.bytes.clear(); - self.metas.clear(); - } -} - -#[cfg(test)] -mod test { - use super::*; - use std::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); - } - } - - /// # Safety - /// - /// This function is only used when the value is a `DropCheck`. - /// This also drops the `DropCheck` value and is only - /// every call once. - unsafe fn drop_check_func(bytes: *mut u8, _: &mut ()) { - let _ = bytes.cast::().read_unaligned(); - } - - #[test] - fn test_anystack_drop() { - let mut stack = AnyStack::<()>::new(); - - let (dropcheck_a, drops_a) = DropCheck::new(); - let (dropcheck_b, drops_b) = DropCheck::new(); - - // SAFE: `noop_func` never reads/write from the provided bytes. - unsafe { - stack.push(dropcheck_a, drop_check_func); - stack.push(dropcheck_b, drop_check_func); - } - - assert_eq!(drops_a.load(Ordering::Relaxed), 0); - assert_eq!(drops_b.load(Ordering::Relaxed), 0); - - stack.consume(&mut ()); - - assert_eq!(drops_a.load(Ordering::Relaxed), 1); - assert_eq!(drops_b.load(Ordering::Relaxed), 1); - } - - struct FakeWorld(u32); - - /// # Safety - /// `bytes` must point to a valid `u32` - /// Since `u32` is a primitive type, it does not require a `drop` call. - unsafe fn increment_fake_world_u32(bytes: *mut u8, world: &mut FakeWorld) { - world.0 += bytes.cast::().read_unaligned(); - } - - #[test] - fn test_anystack() { - let mut stack = AnyStack::new(); - - assert!(stack.is_empty()); - assert_eq!(stack.len(), 0); - - // SAFE: the provided function safely handles the `bytes` and provided `user_data`. - unsafe { - stack.push(5u32, increment_fake_world_u32); - stack.push(10u32, increment_fake_world_u32); - } - - assert!(!stack.is_empty()); - assert_eq!(stack.len(), 2); - - let mut world = FakeWorld(0); - - stack.consume(&mut world); - - assert_eq!(world.0, 15); - - assert!(stack.is_empty()); - assert_eq!(stack.len(), 0); - } -} From af29f6c07582ce383ae6729cf289cd824bd4c862 Mon Sep 17 00:00:00 2001 From: NathanW Date: Sat, 12 Jun 2021 20:47:20 -0600 Subject: [PATCH 06/25] remove remaining anystack files --- crates/bevy_utils/src/anystack.rs | 1 - crates/bevy_utils/src/lib.rs | 1 - 2 files changed, 2 deletions(-) delete mode 100644 crates/bevy_utils/src/anystack.rs diff --git a/crates/bevy_utils/src/anystack.rs b/crates/bevy_utils/src/anystack.rs deleted file mode 100644 index 8b137891791fe..0000000000000 --- a/crates/bevy_utils/src/anystack.rs +++ /dev/null @@ -1 +0,0 @@ - diff --git a/crates/bevy_utils/src/lib.rs b/crates/bevy_utils/src/lib.rs index d6fe4d273ad64..c3b78781d3271 100644 --- a/crates/bevy_utils/src/lib.rs +++ b/crates/bevy_utils/src/lib.rs @@ -1,4 +1,3 @@ -pub mod anystack; mod enum_variant_meta; pub use enum_variant_meta::*; From 206529319617ea02a72e0c642864a8ab44edd4e7 Mon Sep 17 00:00:00 2001 From: NathanW Date: Sat, 12 Jun 2021 21:09:49 -0600 Subject: [PATCH 07/25] remove unused functions --- .../src/system/commands/command_queue.rs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index aaa30807c1baa..b11a6d5c9fb0c 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -29,25 +29,6 @@ unsafe impl Send for CommandQueueInner {} unsafe impl Sync for CommandQueueInner {} impl CommandQueueInner { - ////Constructs a new, empty `CommandQueueInner`. - /// The stack will not allocate until commands are pushed onto it. - #[inline] - pub fn new() -> Self { - Self::default() - } - - /// Returns the number of commands in the [`CommandQueueInner`], also referred to as its ‘length’. - #[inline] - pub fn len(&self) -> usize { - self.metas.len() - } - - /// Returns true if the [`CommandQueueInner`] contains no elements. - #[inline] - pub fn is_empty(&self) -> bool { - self.len() == 0 - } - /// Push a new `command` onto the queue. #[inline] pub fn push(&mut self, command: C) From 30c47bd683c85d3ae0330919cf7481b60029adda Mon Sep 17 00:00:00 2001 From: NathanW Date: Sat, 12 Jun 2021 23:27:23 -0600 Subject: [PATCH 08/25] fix new function to default --- crates/bevy_ecs/src/system/commands/command_queue.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index b11a6d5c9fb0c..26a6759818d65 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -115,7 +115,7 @@ mod test { #[test] fn test_command_queue_inner_drop() { - let mut queue = CommandQueueInner::new(); + let mut queue = CommandQueueInner::default(); let (dropcheck_a, drops_a) = DropCheck::new(); let (dropcheck_b, drops_b) = DropCheck::new(); @@ -143,7 +143,7 @@ mod test { #[test] fn test_command_queue_inner() { - let mut queue = CommandQueueInner::new(); + let mut queue = CommandQueueInner::default(); assert!(queue.is_empty()); assert_eq!(queue.len(), 0); From 453a93e4cd50208ac503e30e72b0afc523b792c0 Mon Sep 17 00:00:00 2001 From: NathanW Date: Sat, 12 Jun 2021 23:30:29 -0600 Subject: [PATCH 09/25] fix tests --- crates/bevy_ecs/src/system/commands/command_queue.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 26a6759818d65..0a3df728c0287 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -145,21 +145,17 @@ mod test { fn test_command_queue_inner() { let mut queue = CommandQueueInner::default(); - assert!(queue.is_empty()); - assert_eq!(queue.len(), 0); - queue.push(SpawnCommand); queue.push(SpawnCommand); - assert!(!queue.is_empty()); - assert_eq!(queue.len(), 2); - let mut world = World::new(); queue.apply(&mut world); assert_eq!(world.entities().len(), 2); - assert!(queue.is_empty()); - assert_eq!(queue.len(), 0); + // The previous call to `apply` clearer the queue. + // This call should do nothing. + queue.apply(&mut world); + assert_eq!(world.entities().len(), 2); } } From a8f474794d7ffda31c77b9ba041539e748071d92 Mon Sep 17 00:00:00 2001 From: NathanW Date: Sun, 13 Jun 2021 12:17:37 -0600 Subject: [PATCH 10/25] update docs/comments --- crates/bevy_ecs/src/system/commands/command_queue.rs | 10 ++-------- crates/bevy_ecs/src/system/commands/mod.rs | 4 ++++ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 0a3df728c0287..894980a5b6c08 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -25,7 +25,7 @@ pub(crate) struct CommandQueueInner { // SAFE: All commands [`Command`] implement [`Send`] unsafe impl Send for CommandQueueInner {} -// SAFE: All commands [`Command`] implement [`Sync`] +// SAFE: `&CommandQueueInner` never gives access to the inner commands. unsafe impl Sync for CommandQueueInner {} impl CommandQueueInner { @@ -64,13 +64,7 @@ impl CommandQueueInner { } /// Invoke each command `func` for each inserted value with `world` - /// and then clears the internal bytes/metas vectors. - /// - /// # Warning - /// - /// This does not [`drop`] the pushed commands. - /// If the command should be dropped, the initially provided `func` - /// should ensure any necessary cleanup occurs. + /// and then clears the internal bytes/metas command vectors. pub fn apply(&mut self, world: &mut World) { let byte_ptr = self.bytes.as_mut_ptr(); for meta in self.metas.iter() { diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 25754d367bb8a..626e35db37651 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -11,6 +11,10 @@ use command_queue::CommandQueueInner; use std::marker::PhantomData; /// A [`World`] mutation. +// +// NOTE: `CommandQueueInner` is `Send` because `Command` is send. +// If the `Command` trait gets reworked to be non-send, `CommandQueueInner` +// should be reworked. pub trait Command: Send + Sync + 'static { fn write(self, world: &mut World); } From 3d45f1d2608f4181f67415ea9463d76ab5a5837a Mon Sep 17 00:00:00 2001 From: NathanW Date: Sun, 13 Jun 2021 21:12:20 -0600 Subject: [PATCH 11/25] fix comment typo --- crates/bevy_ecs/src/system/commands/command_queue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 894980a5b6c08..e48784d0cb028 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -147,7 +147,7 @@ mod test { assert_eq!(world.entities().len(), 2); - // The previous call to `apply` clearer the queue. + // The previous call to `apply` cleared the queue. // This call should do nothing. queue.apply(&mut world); assert_eq!(world.entities().len(), 2); From 31f90d9332c04572e62370ea5a30fbc335e4502c Mon Sep 17 00:00:00 2001 From: NathanW Date: Mon, 14 Jun 2021 21:37:30 -0600 Subject: [PATCH 12/25] add special case for 0 sized commands --- .../src/system/commands/command_queue.rs | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index e48784d0cb028..0b2150c5634fb 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -36,30 +36,31 @@ impl CommandQueueInner { C: Command, { let size = std::mem::size_of::(); - let old_len = self.bytes.len(); - 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); - } - self.metas.push(CommandMeta { offset: old_len, func: invoke_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); } From abb04a8b90bdbda706653287cb6a4c01221514d5 Mon Sep 17 00:00:00 2001 From: NathanW Date: Mon, 14 Jun 2021 21:53:01 -0600 Subject: [PATCH 13/25] fix issues with possible reading from null-ptr --- crates/bevy_ecs/src/system/commands/command_queue.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 0b2150c5634fb..2be5c5e1d2bcc 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -43,9 +43,12 @@ impl CommandQueueInner { func: invoke_command::, }); - if size > 0 { - self.bytes.reserve(size); + // Even if `size` == 0, we still need the vector to allocate. + // When we call `read_unaliged` in `invoke_command`, the ptr must be non-null + // therefore, `self.bytes.as_ptr()` must be non-null. + self.bytes.reserve(size.max(1)); + if size > 0 { // 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. From c3080cff8de93b291f97d7fe7d4de1883d488f6b Mon Sep 17 00:00:00 2001 From: NathanW Date: Mon, 14 Jun 2021 22:39:59 -0600 Subject: [PATCH 14/25] 0-sized struct optimization --- .../src/system/commands/command_queue.rs | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 2be5c5e1d2bcc..f408362eaa378 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -7,7 +7,18 @@ use crate::world::World; /// [`Commands`] `T` type. Also this only reads the data via `read_unaligned` so unaligned /// accesses are safe. unsafe fn invoke_command(command: *mut u8, world: &mut World) { - let command = command.cast::().read_unaligned(); + let command = if std::mem::size_of::() > 0 { + command.cast::().read_unaligned() + } else { + // NOTE: This is necessary because if the `CommandQueueInner` is only filled with 0-sized + // commands the `bytes` vec will never allocate. Then means that `bytes.as_ptr()` could be null + // and reading a null pointer is always UB. + // However 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 the below code is safe to do. + let ptr = std::ptr::NonNull::::dangling().as_ptr(); + ptr.cast::().read() + }; command.write(world); } @@ -43,12 +54,9 @@ impl CommandQueueInner { func: invoke_command::, }); - // Even if `size` == 0, we still need the vector to allocate. - // When we call `read_unaliged` in `invoke_command`, the ptr must be non-null - // therefore, `self.bytes.as_ptr()` must be non-null. - self.bytes.reserve(size.max(1)); - 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. From 0975f63a1e9586e999364a415f5f63b8296b07c6 Mon Sep 17 00:00:00 2001 From: NathanW Date: Tue, 15 Jun 2021 18:39:10 -0600 Subject: [PATCH 15/25] add test that checks if commands implement 'Send' --- .../bevy_ecs/src/system/commands/command_queue.rs | 14 ++++++++++++++ crates/bevy_ecs/src/system/commands/mod.rs | 4 ---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index f408362eaa378..470bba63d04f2 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -164,4 +164,18 @@ mod test { queue.apply(&mut world); assert_eq!(world.entities().len(), 2); } + + // NOTE: `CommandQueueInner` is `Send` because `Command` is send. + // If the `Command` trait gets reworked to be non-send, `CommandQueueInner` + // should be reworked. + // This test asserts that Command types are send. + fn assert_is_send(_: impl Send) {} + fn a_command(command: impl Command) { + assert_is_send(command); + } + + #[test] + fn test_command_is_send() { + a_command(SpawnCommand); + } } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 626e35db37651..25754d367bb8a 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -11,10 +11,6 @@ use command_queue::CommandQueueInner; use std::marker::PhantomData; /// A [`World`] mutation. -// -// NOTE: `CommandQueueInner` is `Send` because `Command` is send. -// If the `Command` trait gets reworked to be non-send, `CommandQueueInner` -// should be reworked. pub trait Command: Send + Sync + 'static { fn write(self, world: &mut World); } From 2a7940bc43130794494901b47f36a7e238efc8e3 Mon Sep 17 00:00:00 2001 From: NathanW Date: Wed, 16 Jun 2021 09:49:12 -0600 Subject: [PATCH 16/25] fix CommandQueueInner not being panic safe --- .../src/system/commands/command_queue.rs | 56 ++++++++++++++++--- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 470bba63d04f2..c636a74042965 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -78,26 +78,28 @@ impl CommandQueueInner { /// Invoke each command `func` for each inserted value with `world` /// and then clears the internal bytes/metas command vectors. pub fn apply(&mut self, world: &mut World) { - let byte_ptr = self.bytes.as_mut_ptr(); - for meta in self.metas.iter() { + let mut bytes = std::mem::replace(&mut self.bytes, vec![]); + let byte_ptr = bytes.as_mut_ptr(); + + for meta in self.metas.drain(..) { // The implementation of `invoke_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); } } - - self.bytes.clear(); - self.metas.clear(); } } #[cfg(test)] mod test { use super::*; - use std::sync::{ - atomic::{AtomicU32, Ordering}, - Arc, + use std::{ + panic::AssertUnwindSafe, + sync::{ + atomic::{AtomicU32, Ordering}, + Arc, + }, }; struct DropCheck(Arc); @@ -165,6 +167,44 @@ mod test { 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 = CommandQueueInner::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: `CommandQueueInner` is `Send` because `Command` is send. // If the `Command` trait gets reworked to be non-send, `CommandQueueInner` // should be reworked. From 90bc0bbb387da98dcf9fa5c54fa7cc4d50910e5f Mon Sep 17 00:00:00 2001 From: NathanW Date: Wed, 16 Jun 2021 13:06:01 -0600 Subject: [PATCH 17/25] reuse bytes vector allocations --- crates/bevy_ecs/src/system/commands/command_queue.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index c636a74042965..87eed19068cd6 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -78,8 +78,12 @@ impl CommandQueueInner { /// Invoke each command `func` for each inserted value with `world` /// and then clears the internal bytes/metas command vectors. pub fn apply(&mut self, world: &mut World) { - let mut bytes = std::mem::replace(&mut self.bytes, vec![]); - let byte_ptr = bytes.as_mut_ptr(); + let byte_ptr = self.bytes.as_mut_ptr(); + + // 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) }; for meta in self.metas.drain(..) { // The implementation of `invoke_command` is safe for the according Command type. From b15ce066d625832b0d1f4a82e435beb013d75c13 Mon Sep 17 00:00:00 2001 From: NathanW Date: Thu, 17 Jun 2021 09:46:10 -0600 Subject: [PATCH 18/25] prevent possilbe UB with set_len and adding to a null pointer --- .../bevy_ecs/src/system/commands/command_queue.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 87eed19068cd6..686e7ee578a2d 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -78,15 +78,22 @@ impl CommandQueueInner { /// Invoke each command `func` for each inserted value with `world` /// and then clears the internal bytes/metas command vectors. pub fn apply(&mut self, world: &mut World) { - let byte_ptr = self.bytes.as_mut_ptr(); - // 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 it's bytes. + // This means either there are no command or there are only zero-sized commands. + // In either of these cases, this pointer will never be read/written to/from. + unsafe { std::ptr::NonNull::dangling().as_mut() } + } else { + self.bytes.as_mut_ptr() + }; + for meta in self.metas.drain(..) { - // The implementation of `invoke_command` is safe for the according Command type. + // SAFE: The implementation of `invoke_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); From 72f230cdcd81f289a3ccff49c25130e01c7fefb1 Mon Sep 17 00:00:00 2001 From: NathanW Date: Fri, 18 Jun 2021 15:46:14 -0600 Subject: [PATCH 19/25] address review comments --- .../src/system/commands/command_queue.rs | 46 ++++++++----------- crates/bevy_ecs/src/system/commands/mod.rs | 2 - 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 686e7ee578a2d..52fb04c8c3824 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -1,27 +1,6 @@ use super::Command; use crate::world::World; -/// # Safety -/// -/// 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 invoke_command(command: *mut u8, world: &mut World) { - let command = if std::mem::size_of::() > 0 { - command.cast::().read_unaligned() - } else { - // NOTE: This is necessary because if the `CommandQueueInner` is only filled with 0-sized - // commands the `bytes` vec will never allocate. Then means that `bytes.as_ptr()` could be null - // and reading a null pointer is always UB. - // However 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 the below code is safe to do. - let ptr = std::ptr::NonNull::::dangling().as_ptr(); - ptr.cast::().read() - }; - command.write(world); -} - struct CommandMeta { offset: usize, func: unsafe fn(value: *mut u8, world: &mut World), @@ -46,12 +25,20 @@ impl CommandQueueInner { 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: invoke_command::, + func: write_command::, }); if size > 0 { @@ -84,16 +71,23 @@ impl CommandQueueInner { 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 it's bytes. - // This means either there are no command or there are only zero-sized commands. - // In either of these cases, this pointer will never be read/written to/from. + // 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 `invoke_command` is safe for the according Command type. + // 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); diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index 25754d367bb8a..f73967ca0b246 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -26,8 +26,6 @@ impl CommandQueue { /// This clears the queue. pub fn apply(&mut self, world: &mut World) { world.flush(); - // Note: the provided function `write_command_on_mut_world` safely - // drops the associated `Command`. self.commands.apply(world); } From aa6057258ad3997b6e39d224b8f7fb03f08bffa3 Mon Sep 17 00:00:00 2001 From: NathanSWard Date: Sun, 20 Jun 2021 14:53:59 -0600 Subject: [PATCH 20/25] directly use CommandQueueInner as CommandQueue --- .../src/system/commands/command_queue.rs | 42 +++++++++++-------- crates/bevy_ecs/src/system/commands/mod.rs | 23 +--------- 2 files changed, 26 insertions(+), 39 deletions(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 52fb04c8c3824..bfc846347cb78 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -6,20 +6,27 @@ struct CommandMeta { 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 simplicitiy of implementation. #[derive(Default)] -pub(crate) struct CommandQueueInner { +pub struct CommandQueue { bytes: Vec, metas: Vec, } // SAFE: All commands [`Command`] implement [`Send`] -unsafe impl Send for CommandQueueInner {} +unsafe impl Send for CommandQueue {} -// SAFE: `&CommandQueueInner` never gives access to the inner commands. -unsafe impl Sync for CommandQueueInner {} +// SAFE: `&CommandQueue` never gives access to the inner commands. +unsafe impl Sync for CommandQueue {} -impl CommandQueueInner { - /// Push a new `command` onto the queue. +impl CommandQueue { + /// Push a [`Command`] onto the queue. #[inline] pub fn push(&mut self, command: C) where @@ -62,8 +69,9 @@ impl CommandQueueInner { std::mem::forget(command); } - /// Invoke each command `func` for each inserted value with `world` - /// and then clears the internal bytes/metas command vectors. + /// Execute the queued [`Command`]s in the world. + /// This clears the queue. + #[inline] pub fn apply(&mut self, world: &mut World) { // 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 @@ -128,7 +136,7 @@ mod test { #[test] fn test_command_queue_inner_drop() { - let mut queue = CommandQueueInner::default(); + let mut queue = CommandQueue::default(); let (dropcheck_a, drops_a) = DropCheck::new(); let (dropcheck_b, drops_b) = DropCheck::new(); @@ -156,7 +164,7 @@ mod test { #[test] fn test_command_queue_inner() { - let mut queue = CommandQueueInner::default(); + let mut queue = CommandQueue::default(); queue.push(SpawnCommand); queue.push(SpawnCommand); @@ -186,7 +194,7 @@ mod test { fn test_command_queue_inner_panic_safe() { std::panic::set_hook(Box::new(|_| {})); - let mut queue = CommandQueueInner::default(); + let mut queue = CommandQueue::default(); queue.push(PanicCommand("I panic!".to_owned())); queue.push(SpawnCommand); @@ -210,17 +218,17 @@ mod test { assert_eq!(world.entities().len(), 2); } - // NOTE: `CommandQueueInner` is `Send` because `Command` is send. - // If the `Command` trait gets reworked to be non-send, `CommandQueueInner` + // 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 Send) {} - fn a_command(command: impl Command) { - assert_is_send(command); + 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() { - a_command(SpawnCommand); + assert_is_send(SpawnCommand); } } diff --git a/crates/bevy_ecs/src/system/commands/mod.rs b/crates/bevy_ecs/src/system/commands/mod.rs index f73967ca0b246..0488b09e4f770 100644 --- a/crates/bevy_ecs/src/system/commands/mod.rs +++ b/crates/bevy_ecs/src/system/commands/mod.rs @@ -7,7 +7,7 @@ use crate::{ world::World, }; use bevy_utils::tracing::debug; -use command_queue::CommandQueueInner; +pub use command_queue::CommandQueue; use std::marker::PhantomData; /// A [`World`] mutation. @@ -15,27 +15,6 @@ pub trait Command: Send + Sync + 'static { fn write(self, world: &mut World); } -/// A queue of [`Command`]s. -#[derive(Default)] -pub struct CommandQueue { - commands: CommandQueueInner, -} - -impl CommandQueue { - /// Execute the queued [`Command`]s in the world. - /// This clears the queue. - pub fn apply(&mut self, world: &mut World) { - world.flush(); - self.commands.apply(world); - } - - /// Push a [`Command`] onto the queue. - #[inline] - pub fn push(&mut self, command: T) { - self.commands.push(command); - } -} - /// A list of commands that will be run to modify a [`World`]. pub struct Commands<'a> { queue: &'a mut CommandQueue, From 3b706a81c770bc52e05f0560b9621c2fc8d00af6 Mon Sep 17 00:00:00 2001 From: NathanSWard Date: Sun, 20 Jun 2021 15:33:47 -0600 Subject: [PATCH 21/25] remember to flush world --- crates/bevy_ecs/src/system/commands/command_queue.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index bfc846347cb78..4880684f7b7d9 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -73,6 +73,9 @@ impl CommandQueue { /// This clears the queue. #[inline] pub fn apply(&mut self, world: &mut World) { + // flush previouly 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. From e88f779a4de9d5d8f9be9c8f4cdc2ea46b4a2313 Mon Sep 17 00:00:00 2001 From: NathanSWard Date: Sun, 20 Jun 2021 15:34:26 -0600 Subject: [PATCH 22/25] typo --- crates/bevy_ecs/src/system/commands/command_queue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 4880684f7b7d9..d9efabb3db67b 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -73,7 +73,7 @@ impl CommandQueue { /// This clears the queue. #[inline] pub fn apply(&mut self, world: &mut World) { - // flush previouly queued entities + // flush previously queued entities world.flush(); // SAFE: In the iteration below, `meta.func` will safely consume and drop each pushed command. From 217e2770e7b439e8319b83ef295b4695bbe20a56 Mon Sep 17 00:00:00 2001 From: NathanSWard Date: Sun, 20 Jun 2021 15:42:10 -0600 Subject: [PATCH 23/25] re-trigger ci --- crates/bevy_ecs/src/system/commands/command_queue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index d9efabb3db67b..491bffffc0f70 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -73,7 +73,7 @@ impl CommandQueue { /// This clears the queue. #[inline] pub fn apply(&mut self, world: &mut World) { - // flush previously queued entities + // flush the previously queued entities world.flush(); // SAFE: In the iteration below, `meta.func` will safely consume and drop each pushed command. From a9be103b3da909c2f9ecb956c9c64e672b0eb100 Mon Sep 17 00:00:00 2001 From: NathanW Date: Mon, 21 Jun 2021 10:11:56 -0600 Subject: [PATCH 24/25] fix typo --- crates/bevy_ecs/src/system/commands/command_queue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/commands/command_queue.rs b/crates/bevy_ecs/src/system/commands/command_queue.rs index 491bffffc0f70..64a3bd7914a6f 100644 --- a/crates/bevy_ecs/src/system/commands/command_queue.rs +++ b/crates/bevy_ecs/src/system/commands/command_queue.rs @@ -12,7 +12,7 @@ struct CommandMeta { // 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 simplicitiy of implementation. +// preferred to simplicity of implementation. #[derive(Default)] pub struct CommandQueue { bytes: Vec, From 09ccbc0e3c55d3d8b16c2aa293ad1a9a07bce598 Mon Sep 17 00:00:00 2001 From: NathanSWard Date: Fri, 16 Jul 2021 10:39:32 -0600 Subject: [PATCH 25/25] update commands perf test impl --- benches/benches/bevy_ecs/commands.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/benches/benches/bevy_ecs/commands.rs b/benches/benches/bevy_ecs/commands.rs index 57ba249f581fa..d120589702903 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); }