bevy/crates/bevy_ecs/src/system/commands/command_queue.rs

243 lines
7.4 KiB
Rust
Raw Normal View History

use std::mem::{ManuallyDrop, MaybeUninit};
use super::Command;
use crate::world::World;
struct CommandMeta {
offset: usize,
func: unsafe fn(value: *mut MaybeUninit<u8>, world: &mut World),
}
/// A queue of [`Command`]s
//
// NOTE: [`CommandQueue`] is implemented via a `Vec<MaybeUninit<u8>>` over a `Vec<Box<dyn Command>>`
// 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<MaybeUninit<u8>>,
metas: Vec<CommandMeta>,
}
add more `SAFETY` comments and lint for missing ones in `bevy_ecs` (#4835) # Objective `SAFETY` comments are meant to be placed before `unsafe` blocks and should contain the reasoning of why in this case the usage of unsafe is okay. This is useful when reading the code because it makes it clear which assumptions are required for safety, and makes it easier to spot possible unsoundness holes. It also forces the code writer to think of something to write and maybe look at the safety contracts of any called unsafe methods again to double-check their correct usage. There's a clippy lint called `undocumented_unsafe_blocks` which warns when using a block without such a comment. ## Solution - since clippy expects `SAFETY` instead of `SAFE`, rename those - add `SAFETY` comments in more places - for the last remaining 3 places, add an `#[allow()]` and `// TODO` since I wasn't comfortable enough with the code to justify their safety - add ` #![warn(clippy::undocumented_unsafe_blocks)]` to `bevy_ecs` ### Note for reviewers The first commit only renames `SAFETY` to `SAFE` so it doesn't need a thorough review. https://github.com/bevyengine/bevy/pull/4835/files/cb042a416ecbe5e7d74797449969e064d8a5f13c..55cef2d6fa3aa634667a60f6d5abc16f43f16298 is the diff for all other changes. ### Safety comments where I'm not too familiar with the code https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/entity/mod.rs#L540-L546 https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/world/entity_ref.rs#L249-L252 ### Locations left undocumented with a `TODO` comment https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/schedule/executor_parallel.rs#L196-L199 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L287-L289 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L413-L415 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
2022-07-04 14:44:24 +00:00
// SAFETY: All commands [`Command`] implement [`Send`]
unsafe impl Send for CommandQueue {}
add more `SAFETY` comments and lint for missing ones in `bevy_ecs` (#4835) # Objective `SAFETY` comments are meant to be placed before `unsafe` blocks and should contain the reasoning of why in this case the usage of unsafe is okay. This is useful when reading the code because it makes it clear which assumptions are required for safety, and makes it easier to spot possible unsoundness holes. It also forces the code writer to think of something to write and maybe look at the safety contracts of any called unsafe methods again to double-check their correct usage. There's a clippy lint called `undocumented_unsafe_blocks` which warns when using a block without such a comment. ## Solution - since clippy expects `SAFETY` instead of `SAFE`, rename those - add `SAFETY` comments in more places - for the last remaining 3 places, add an `#[allow()]` and `// TODO` since I wasn't comfortable enough with the code to justify their safety - add ` #![warn(clippy::undocumented_unsafe_blocks)]` to `bevy_ecs` ### Note for reviewers The first commit only renames `SAFETY` to `SAFE` so it doesn't need a thorough review. https://github.com/bevyengine/bevy/pull/4835/files/cb042a416ecbe5e7d74797449969e064d8a5f13c..55cef2d6fa3aa634667a60f6d5abc16f43f16298 is the diff for all other changes. ### Safety comments where I'm not too familiar with the code https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/entity/mod.rs#L540-L546 https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/world/entity_ref.rs#L249-L252 ### Locations left undocumented with a `TODO` comment https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/schedule/executor_parallel.rs#L196-L199 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L287-L289 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L413-L415 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
2022-07-04 14:44:24 +00:00
// SAFETY: `&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<C>(&mut self, command: C)
where
C: Command,
{
add more `SAFETY` comments and lint for missing ones in `bevy_ecs` (#4835) # Objective `SAFETY` comments are meant to be placed before `unsafe` blocks and should contain the reasoning of why in this case the usage of unsafe is okay. This is useful when reading the code because it makes it clear which assumptions are required for safety, and makes it easier to spot possible unsoundness holes. It also forces the code writer to think of something to write and maybe look at the safety contracts of any called unsafe methods again to double-check their correct usage. There's a clippy lint called `undocumented_unsafe_blocks` which warns when using a block without such a comment. ## Solution - since clippy expects `SAFETY` instead of `SAFE`, rename those - add `SAFETY` comments in more places - for the last remaining 3 places, add an `#[allow()]` and `// TODO` since I wasn't comfortable enough with the code to justify their safety - add ` #![warn(clippy::undocumented_unsafe_blocks)]` to `bevy_ecs` ### Note for reviewers The first commit only renames `SAFETY` to `SAFE` so it doesn't need a thorough review. https://github.com/bevyengine/bevy/pull/4835/files/cb042a416ecbe5e7d74797449969e064d8a5f13c..55cef2d6fa3aa634667a60f6d5abc16f43f16298 is the diff for all other changes. ### Safety comments where I'm not too familiar with the code https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/entity/mod.rs#L540-L546 https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/world/entity_ref.rs#L249-L252 ### Locations left undocumented with a `TODO` comment https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/schedule/executor_parallel.rs#L196-L199 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L287-L289 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L413-L415 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
2022-07-04 14:44:24 +00:00
/// 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<T: Command>(command: *mut MaybeUninit<u8>, world: &mut World) {
let command = command.cast::<T>().read_unaligned();
command.write(world);
}
let size = std::mem::size_of::<C>();
let old_len = self.bytes.len();
self.metas.push(CommandMeta {
offset: old_len,
func: write_command::<C>,
});
// Use `ManuallyDrop` to forget `command` right away, avoiding
// any use of it after the `ptr::copy_nonoverlapping`.
let command = ManuallyDrop::new(command);
if size > 0 {
self.bytes.reserve(size);
add more `SAFETY` comments and lint for missing ones in `bevy_ecs` (#4835) # Objective `SAFETY` comments are meant to be placed before `unsafe` blocks and should contain the reasoning of why in this case the usage of unsafe is okay. This is useful when reading the code because it makes it clear which assumptions are required for safety, and makes it easier to spot possible unsoundness holes. It also forces the code writer to think of something to write and maybe look at the safety contracts of any called unsafe methods again to double-check their correct usage. There's a clippy lint called `undocumented_unsafe_blocks` which warns when using a block without such a comment. ## Solution - since clippy expects `SAFETY` instead of `SAFE`, rename those - add `SAFETY` comments in more places - for the last remaining 3 places, add an `#[allow()]` and `// TODO` since I wasn't comfortable enough with the code to justify their safety - add ` #![warn(clippy::undocumented_unsafe_blocks)]` to `bevy_ecs` ### Note for reviewers The first commit only renames `SAFETY` to `SAFE` so it doesn't need a thorough review. https://github.com/bevyengine/bevy/pull/4835/files/cb042a416ecbe5e7d74797449969e064d8a5f13c..55cef2d6fa3aa634667a60f6d5abc16f43f16298 is the diff for all other changes. ### Safety comments where I'm not too familiar with the code https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/entity/mod.rs#L540-L546 https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/world/entity_ref.rs#L249-L252 ### Locations left undocumented with a `TODO` comment https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/schedule/executor_parallel.rs#L196-L199 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L287-L289 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L413-L415 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
2022-07-04 14:44:24 +00:00
// SAFETY: The internal `bytes` vector has enough storage for the
// command (see the call the `reserve` above), the vector has
// its length set appropriately and can contain any kind of bytes.
// In case we're writing a ZST and the `Vec` hasn't allocated yet
// then `as_mut_ptr` will be a dangling (non null) pointer, and
// thus valid for ZST writes.
// Also `command` is forgotten so that when `apply` is called
// later, a double `drop` does not occur.
unsafe {
std::ptr::copy_nonoverlapping(
&*command as *const C as *const MaybeUninit<u8>,
self.bytes.as_mut_ptr().add(old_len),
size,
);
self.bytes.set_len(old_len + size);
}
}
}
/// 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();
add more `SAFETY` comments and lint for missing ones in `bevy_ecs` (#4835) # Objective `SAFETY` comments are meant to be placed before `unsafe` blocks and should contain the reasoning of why in this case the usage of unsafe is okay. This is useful when reading the code because it makes it clear which assumptions are required for safety, and makes it easier to spot possible unsoundness holes. It also forces the code writer to think of something to write and maybe look at the safety contracts of any called unsafe methods again to double-check their correct usage. There's a clippy lint called `undocumented_unsafe_blocks` which warns when using a block without such a comment. ## Solution - since clippy expects `SAFETY` instead of `SAFE`, rename those - add `SAFETY` comments in more places - for the last remaining 3 places, add an `#[allow()]` and `// TODO` since I wasn't comfortable enough with the code to justify their safety - add ` #![warn(clippy::undocumented_unsafe_blocks)]` to `bevy_ecs` ### Note for reviewers The first commit only renames `SAFETY` to `SAFE` so it doesn't need a thorough review. https://github.com/bevyengine/bevy/pull/4835/files/cb042a416ecbe5e7d74797449969e064d8a5f13c..55cef2d6fa3aa634667a60f6d5abc16f43f16298 is the diff for all other changes. ### Safety comments where I'm not too familiar with the code https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/entity/mod.rs#L540-L546 https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/world/entity_ref.rs#L249-L252 ### Locations left undocumented with a `TODO` comment https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/schedule/executor_parallel.rs#L196-L199 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L287-L289 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L413-L415 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
2022-07-04 14:44:24 +00:00
// SAFETY: 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<u8>`'s internal storage and prevent
// unnecessary allocations.
unsafe { self.bytes.set_len(0) };
for meta in self.metas.drain(..) {
add more `SAFETY` comments and lint for missing ones in `bevy_ecs` (#4835) # Objective `SAFETY` comments are meant to be placed before `unsafe` blocks and should contain the reasoning of why in this case the usage of unsafe is okay. This is useful when reading the code because it makes it clear which assumptions are required for safety, and makes it easier to spot possible unsoundness holes. It also forces the code writer to think of something to write and maybe look at the safety contracts of any called unsafe methods again to double-check their correct usage. There's a clippy lint called `undocumented_unsafe_blocks` which warns when using a block without such a comment. ## Solution - since clippy expects `SAFETY` instead of `SAFE`, rename those - add `SAFETY` comments in more places - for the last remaining 3 places, add an `#[allow()]` and `// TODO` since I wasn't comfortable enough with the code to justify their safety - add ` #![warn(clippy::undocumented_unsafe_blocks)]` to `bevy_ecs` ### Note for reviewers The first commit only renames `SAFETY` to `SAFE` so it doesn't need a thorough review. https://github.com/bevyengine/bevy/pull/4835/files/cb042a416ecbe5e7d74797449969e064d8a5f13c..55cef2d6fa3aa634667a60f6d5abc16f43f16298 is the diff for all other changes. ### Safety comments where I'm not too familiar with the code https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/entity/mod.rs#L540-L546 https://github.com/bevyengine/bevy/blob/774012ece50e4add4fcc8324ec48bbecf5546c3c/crates/bevy_ecs/src/world/entity_ref.rs#L249-L252 ### Locations left undocumented with a `TODO` comment https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/schedule/executor_parallel.rs#L196-L199 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L287-L289 https://github.com/bevyengine/bevy/blob/5dde944a3051426ac69fdedc5699f7da97a7e147/crates/bevy_ecs/src/world/entity_ref.rs#L413-L415 Co-authored-by: Jakob Hellermann <hellermann@sipgate.de>
2022-07-04 14:44:24 +00:00
// SAFETY: The implementation of `write_command` is safe for the according Command type.
// It's ok to read from `bytes.as_mut_ptr()` because we just wrote to it in `push`.
// The bytes are safely cast to their original type, safely read, and then dropped.
unsafe {
(meta.func)(self.bytes.as_mut_ptr().add(meta.offset), world);
}
}
}
}
#[cfg(test)]
mod test {
use super::*;
use std::{
panic::AssertUnwindSafe,
sync::{
atomic::{AtomicU32, Ordering},
Arc,
},
};
struct DropCheck(Arc<AtomicU32>);
impl DropCheck {
fn new() -> (Self, Arc<AtomicU32>) {
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) {
Spawn now takes a Bundle (#6054) # Objective Now that we can consolidate Bundles and Components under a single insert (thanks to #2975 and #6039), almost 100% of world spawns now look like `world.spawn().insert((Some, Tuple, Here))`. Spawning an entity without any components is an extremely uncommon pattern, so it makes sense to give spawn the "first class" ergonomic api. This consolidated api should be made consistent across all spawn apis (such as World and Commands). ## Solution All `spawn` apis (`World::spawn`, `Commands:;spawn`, `ChildBuilder::spawn`, and `WorldChildBuilder::spawn`) now accept a bundle as input: ```rust // before: commands .spawn() .insert((A, B, C)); world .spawn() .insert((A, B, C); // after commands.spawn((A, B, C)); world.spawn((A, B, C)); ``` All existing instances of `spawn_bundle` have been deprecated in favor of the new `spawn` api. A new `spawn_empty` has been added, replacing the old `spawn` api. By allowing `world.spawn(some_bundle)` to replace `world.spawn().insert(some_bundle)`, this opened the door to removing the initial entity allocation in the "empty" archetype / table done in `spawn()` (and subsequent move to the actual archetype in `.insert(some_bundle)`). This improves spawn performance by over 10%: ![image](https://user-images.githubusercontent.com/2694663/191627587-4ab2f949-4ccd-4231-80eb-80dd4d9ad6b9.png) To take this measurement, I added a new `world_spawn` benchmark. Unfortunately, optimizing `Commands::spawn` is slightly less trivial, as Commands expose the Entity id of spawned entities prior to actually spawning. Doing the optimization would (naively) require assurances that the `spawn(some_bundle)` command is applied before all other commands involving the entity (which would not necessarily be true, if memory serves). Optimizing `Commands::spawn` this way does feel possible, but it will require careful thought (and maybe some additional checks), which deserves its own PR. For now, it has the same performance characteristics of the current `Commands::spawn_bundle` on main. **Note that 99% of this PR is simple renames and refactors. The only code that needs careful scrutiny is the new `World::spawn()` impl, which is relatively straightforward, but it has some new unsafe code (which re-uses battle tested BundlerSpawner code path).** --- ## Changelog - All `spawn` apis (`World::spawn`, `Commands:;spawn`, `ChildBuilder::spawn`, and `WorldChildBuilder::spawn`) now accept a bundle as input - All instances of `spawn_bundle` have been deprecated in favor of the new `spawn` api - World and Commands now have `spawn_empty()`, which is equivalent to the old `spawn()` behavior. ## Migration Guide ```rust // Old (0.8): commands .spawn() .insert_bundle((A, B, C)); // New (0.9) commands.spawn((A, B, C)); // Old (0.8): commands.spawn_bundle((A, B, C)); // New (0.9) commands.spawn((A, B, C)); // Old (0.8): let entity = commands.spawn().id(); // New (0.9) let entity = commands.spawn_empty().id(); // Old (0.8) let entity = world.spawn().id(); // New (0.9) let entity = world.spawn_empty(); ```
2022-09-23 19:55:54 +00:00
world.spawn_empty();
}
}
#[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);
}
struct CommandWithPadding(u8, u16);
impl Command for CommandWithPadding {
fn write(self, _: &mut World) {}
}
#[cfg(miri)]
#[test]
fn test_uninit_bytes() {
let mut queue = CommandQueue::default();
queue.push(CommandWithPadding(0, 0));
let _ = format!("{:?}", queue.bytes);
}
}