From 681676760eba532c5cdfff3dfc290a0e819daf38 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:07:10 -0400 Subject: [PATCH 01/29] use `UnsafeWorldCell` for system impls --- crates/bevy_ecs/src/schedule/condition.rs | 3 ++- crates/bevy_ecs/src/system/combinator.rs | 3 ++- .../src/system/exclusive_function_system.rs | 4 ++-- crates/bevy_ecs/src/system/function_system.rs | 14 +++++++------- crates/bevy_ecs/src/system/system.rs | 5 +++-- 5 files changed, 16 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index 5e002f67933f5..4662c9f4fe961 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -5,6 +5,7 @@ use std::ops::Not; use crate::component::{self, ComponentId}; use crate::query::Access; use crate::system::{CombinatorSystem, Combine, IntoSystem, ReadOnlySystem, System}; +use crate::world::unsafe_world_cell::UnsafeWorldCell; use crate::world::World; pub type BoxedCondition = Box>; @@ -978,7 +979,7 @@ where self.condition.is_exclusive() } - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { // SAFETY: The inner condition system asserts its own safety. !self.condition.run_unsafe(input, world) } diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 671a29431c883..07fda362c58e7 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -7,6 +7,7 @@ use crate::{ component::{ComponentId, Tick}, prelude::World, query::Access, + world::unsafe_world_cell::UnsafeWorldCell, }; use super::{ReadOnlySystem, System}; @@ -157,7 +158,7 @@ where self.a.is_exclusive() || self.b.is_exclusive() } - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { Func::combine( input, // SAFETY: The world accesses for both underlying systems have been registered, diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index d82c6ff153bee..48d31a9e9f652 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -6,7 +6,7 @@ use crate::{ check_system_change_tick, ExclusiveSystemParam, ExclusiveSystemParamItem, In, IntoSystem, System, SystemMeta, }, - world::{World, WorldId}, + world::{unsafe_world_cell::UnsafeWorldCell, World, WorldId}, }; use bevy_utils::all_tuples; @@ -88,7 +88,7 @@ where } #[inline] - unsafe fn run_unsafe(&mut self, _input: Self::In, _world: &World) -> Self::Out { + unsafe fn run_unsafe(&mut self, _input: Self::In, _world: UnsafeWorldCell) -> Self::Out { panic!("Cannot run exclusive systems with a shared World reference"); } diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 8bda168b78bf0..21028a45e769f 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -4,7 +4,7 @@ use crate::{ prelude::FromWorld, query::{Access, FilteredAccessSet}, system::{check_system_change_tick, ReadOnlySystemParam, System, SystemParam, SystemParamItem}, - world::{World, WorldId}, + world::{unsafe_world_cell::UnsafeWorldCell, World, WorldId}, }; use bevy_utils::all_tuples; @@ -476,17 +476,17 @@ where } #[inline] - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out { + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out { let change_tick = world.increment_change_tick(); - // Safety: - // We update the archetype component access correctly based on `Param`'s requirements - // in `update_archetype_component_access`. - // Our caller upholds the requirements. + // SAFETY: + // - We have registered all world accesses used by `F::Param`, so the caller + // will ensure that `world` has permission to use those accesses. + // - World validation: FIXME (see https://github.com/bevyengine/bevy/pull/7778). let params = F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, - world.as_unsafe_world_cell_migration_internal(), + world, change_tick, ); let out = self.func.run(input, params); diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index b8c11b6262677..c8d277ad72193 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -2,6 +2,7 @@ use bevy_utils::tracing::warn; use core::fmt::Debug; use crate::component::Tick; +use crate::world::unsafe_world_cell::UnsafeWorldCell; use crate::{archetype::ArchetypeComponentId, component::ComponentId, query::Access, world::World}; use std::any::TypeId; @@ -49,12 +50,12 @@ pub trait System: Send + Sync + 'static { /// 1. This system is the only system running on the given world across all threads. /// 2. This system only runs in parallel with other systems that do not conflict with the /// [`System::archetype_component_access()`]. - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out; + unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out; /// Runs the system with the given input in the world. fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out { self.update_archetype_component_access(world); // SAFETY: world and resources are exclusively borrowed - unsafe { self.run_unsafe(input, world) } + unsafe { self.run_unsafe(input, world.as_unsafe_world_cell()) } } fn apply_buffers(&mut self, world: &mut World); /// Initialize the system. From 6c25b97974d76d4e6e8f06f2afdd737ab8837ef7 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:25:28 -0400 Subject: [PATCH 02/29] add `ReadOnlySystem::run_read_only` --- crates/bevy_ecs/src/system/system.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index c8d277ad72193..62fbbd0a779d9 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -40,7 +40,7 @@ pub trait System: Send + Sync + 'static { fn is_exclusive(&self) -> bool; /// Runs the system with the given input in the world. Unlike [`System::run`], this function - /// takes a shared reference to [`World`] and may therefore break Rust's aliasing rules, making + /// takes [`UnsafeWorldCell`] and may therefore break Rust's aliasing rules, making /// it unsafe to call. /// /// # Safety @@ -86,7 +86,17 @@ pub trait System: Send + Sync + 'static { /// # Safety /// /// This must only be implemented for system types which do not mutate the `World`. -pub unsafe trait ReadOnlySystem: System {} +pub unsafe trait ReadOnlySystem: System { + /// Runs this system with the given input in the World. + /// This this system is known not to modify the world, it can run + /// with a shared reference to the world (`&World`). + fn run_read_only(&mut self, input: Self::In, world: &World) -> Self::Out { + // SAFETY: `&World` gives us immutable access to the entire world. + // The implementor of `Self` guarantees that the system will not + // mutate the world. + unsafe { self.run_unsafe(world.as_unsafe_world_cell_readonly()) } + } +} /// A convenience type alias for a boxed [`System`] trait object. pub type BoxedSystem = Box>; From ea4f90286e8d8909d3a29d720c07135a17b02130 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:28:46 -0400 Subject: [PATCH 03/29] validate world in `run_read_only` --- crates/bevy_ecs/src/system/system.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 62fbbd0a779d9..9063ac7dae18f 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -91,9 +91,13 @@ pub unsafe trait ReadOnlySystem: System { /// This this system is known not to modify the world, it can run /// with a shared reference to the world (`&World`). fn run_read_only(&mut self, input: Self::In, world: &World) -> Self::Out { - // SAFETY: `&World` gives us immutable access to the entire world. - // The implementor of `Self` guarantees that the system will not - // mutate the world. + self.update_archetype_component_access(world); + // SAFETY: + // -`&World` gives us immutable access to the entire world. + // The implementor of `Self` guarantees that the system will not + // mutate the world. + // - We have just called `update_archetype_component_access`, which will + // panic if the world is not valid. unsafe { self.run_unsafe(world.as_unsafe_world_cell_readonly()) } } } From 87f267fe481daea125d5832a9dabd5aa64438ba8 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:29:52 -0400 Subject: [PATCH 04/29] use `UnsafeWorldCell` in the system executor --- .../src/schedule/executor/multi_threaded.rs | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 0606e7fbdc799..4ceb98f2ab290 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -18,7 +18,7 @@ use crate::{ is_apply_system_buffers, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule, }, system::BoxedSystem, - world::World, + world::{unsafe_world_cell::UnsafeWorldCell, World}, }; use crate as bevy_ecs; @@ -169,7 +169,7 @@ impl SystemExecutor for MultiThreadedExecutor { .map(|e| e.0.clone()); let thread_executor = thread_executor.as_deref(); - let world = SyncUnsafeCell::from_mut(world); + let world_cell = world.as_unsafe_world_cell(); let SyncUnsafeSchedule { systems, mut conditions, @@ -185,7 +185,7 @@ impl SystemExecutor for MultiThreadedExecutor { while self.num_completed_systems < num_systems { // SAFETY: self.ready_systems does not contain running systems unsafe { - self.spawn_system_tasks(scope, systems, &mut conditions, world); + self.spawn_system_tasks(scope, systems, &mut conditions, world_cell); } if self.num_running_systems > 0 { @@ -217,7 +217,7 @@ impl SystemExecutor for MultiThreadedExecutor { if self.apply_final_buffers { // Do one final apply buffers after all systems have completed // Commands should be applied while on the scope's thread, not the executor's thread - apply_system_buffers(&self.unapplied_systems, systems, world.get_mut()); + apply_system_buffers(&self.unapplied_systems, systems, world); self.unapplied_systems.clear(); debug_assert!(self.unapplied_systems.is_clear()); } @@ -263,7 +263,7 @@ impl MultiThreadedExecutor { scope: &Scope<'_, 'scope, ()>, systems: &'scope [SyncUnsafeCell], conditions: &mut Conditions, - cell: &'scope SyncUnsafeCell, + world_cell: UnsafeWorldCell<'scope>, ) { if self.exclusive_running { return; @@ -282,7 +282,7 @@ impl MultiThreadedExecutor { // SAFETY: No exclusive system is running. // Therefore, there is no existing mutable reference to the world. - let world = unsafe { &*cell.get() }; + let world = unsafe { world_cell.world() }; if !self.can_run(system_index, system, conditions, world) { // NOTE: exclusive systems with ambiguities are susceptible to // being significantly displaced here (compared to single-threaded order) @@ -305,7 +305,7 @@ impl MultiThreadedExecutor { // SAFETY: `can_run` confirmed that no systems are running. // Therefore, there is no existing reference to the world. unsafe { - let world = &mut *cell.get(); + let world = world_cell.world_mut(); self.spawn_exclusive_system_task(scope, system_index, systems, world); } break; @@ -313,7 +313,7 @@ impl MultiThreadedExecutor { // SAFETY: No other reference to this system exists. unsafe { - self.spawn_system_task(scope, system_index, systems, world); + self.spawn_system_task(scope, system_index, systems, world_cell); } } @@ -427,7 +427,7 @@ impl MultiThreadedExecutor { scope: &Scope<'_, 'scope, ()>, system_index: usize, systems: &'scope [SyncUnsafeCell], - world: &'scope World, + world: UnsafeWorldCell<'scope>, ) { // SAFETY: this system is not running, no other reference exists let system = unsafe { &mut *systems[system_index].get() }; @@ -611,8 +611,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World .map(|condition| { #[cfg(feature = "trace")] let _condition_span = info_span!("condition", name = &*condition.name()).entered(); - // SAFETY: caller ensures system access is compatible - unsafe { condition.run_unsafe((), world) } + unsafe { condition.run_read_only((), world) } }) .fold(true, |acc, res| acc && res) } From f7ff99dfad016471a1cdc24593af5f0732d2aef7 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:32:01 -0400 Subject: [PATCH 05/29] use elided lifetimes in the system executor --- .../src/schedule/executor/multi_threaded.rs | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 4ceb98f2ab290..e160f825380c3 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -258,12 +258,12 @@ impl MultiThreadedExecutor { /// # Safety /// Caller must ensure that `self.ready_systems` does not contain any systems that /// have been mutably borrowed (such as the systems currently running). - unsafe fn spawn_system_tasks<'scope>( + unsafe fn spawn_system_tasks( &mut self, - scope: &Scope<'_, 'scope, ()>, - systems: &'scope [SyncUnsafeCell], + scope: &Scope<()>, + systems: &[SyncUnsafeCell], conditions: &mut Conditions, - world_cell: UnsafeWorldCell<'scope>, + world_cell: UnsafeWorldCell, ) { if self.exclusive_running { return; @@ -422,12 +422,12 @@ impl MultiThreadedExecutor { /// # Safety /// Caller must not alias systems that are running. - unsafe fn spawn_system_task<'scope>( + unsafe fn spawn_system_task( &mut self, - scope: &Scope<'_, 'scope, ()>, + scope: &Scope<()>, system_index: usize, - systems: &'scope [SyncUnsafeCell], - world: UnsafeWorldCell<'scope>, + systems: &[SyncUnsafeCell], + world: UnsafeWorldCell, ) { // SAFETY: this system is not running, no other reference exists let system = unsafe { &mut *systems[system_index].get() }; @@ -475,12 +475,12 @@ impl MultiThreadedExecutor { /// # Safety /// Caller must ensure no systems are currently borrowed. - unsafe fn spawn_exclusive_system_task<'scope>( + unsafe fn spawn_exclusive_system_task( &mut self, - scope: &Scope<'_, 'scope, ()>, + scope: &Scope<()>, system_index: usize, - systems: &'scope [SyncUnsafeCell], - world: &'scope mut World, + systems: &[SyncUnsafeCell], + world: &mut World, ) { // SAFETY: this system is not running, no other reference exists let system = unsafe { &mut *systems[system_index].get() }; From bf82d9dabc36fa1b092a8e9f3f9ed558ba75ad63 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:34:33 -0400 Subject: [PATCH 06/29] remove an unnecessary unsafe block --- crates/bevy_ecs/src/schedule/executor/multi_threaded.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index e160f825380c3..d3ddc3cfd0bea 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -611,7 +611,7 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World .map(|condition| { #[cfg(feature = "trace")] let _condition_span = info_span!("condition", name = &*condition.name()).entered(); - unsafe { condition.run_read_only((), world) } + condition.run_read_only((), world) }) .fold(true, |acc, res| acc && res) } From 6f73a7d64a014a6e5459a50ce2409154e7833d4a Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:35:47 -0400 Subject: [PATCH 07/29] improve docs for `run_read_only` --- crates/bevy_ecs/src/system/system.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 9063ac7dae18f..12a68ee71a405 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -88,8 +88,8 @@ pub trait System: Send + Sync + 'static { /// This must only be implemented for system types which do not mutate the `World`. pub unsafe trait ReadOnlySystem: System { /// Runs this system with the given input in the World. - /// This this system is known not to modify the world, it can run - /// with a shared reference to the world (`&World`). + /// + /// Unlike [`System::run`], this method can be run with a shared reference to the world. fn run_read_only(&mut self, input: Self::In, world: &World) -> Self::Out { self.update_archetype_component_access(world); // SAFETY: From f026dd291b905c157042c48bd5bdc2d8c2570d54 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:38:09 -0400 Subject: [PATCH 08/29] modify safety invariants for `System::run_unsafe` --- crates/bevy_ecs/src/system/system.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 12a68ee71a405..cfe3da0ac0af5 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -45,11 +45,9 @@ pub trait System: Send + Sync + 'static { /// /// # Safety /// - /// This might access world and resources in an unsafe manner. This should only be called in one - /// of the following contexts: - /// 1. This system is the only system running on the given world across all threads. - /// 2. This system only runs in parallel with other systems that do not conflict with the - /// [`System::archetype_component_access()`]. + /// The caller must ensure that `world` has permission to access any world data + /// registered in [`Self::archetype_component_access`]. No systems with conflicting + /// data access may run at the same time. unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out; /// Runs the system with the given input in the world. fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out { From 46a44a54725e4a3adbb7d608d95eb06ea97f1eee Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:39:17 -0400 Subject: [PATCH 09/29] tweak some phrasing --- crates/bevy_ecs/src/system/function_system.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 21028a45e769f..34ebbbdea69cb 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -481,7 +481,7 @@ where // SAFETY: // - We have registered all world accesses used by `F::Param`, so the caller - // will ensure that `world` has permission to use those accesses. + // will ensure that `world` has permission to access those data. // - World validation: FIXME (see https://github.com/bevyengine/bevy/pull/7778). let params = F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), From 9b3021526683b17c694129023a03d19f54b7da9b Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 10:44:08 -0400 Subject: [PATCH 10/29] simplify a condition-folding invariant --- .../src/schedule/executor/multi_threaded.rs | 4 +++- crates/bevy_ecs/src/system/system.rs | 16 +--------------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index d3ddc3cfd0bea..d678c00fd5a51 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -611,7 +611,9 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World .map(|condition| { #[cfg(feature = "trace")] let _condition_span = info_span!("condition", name = &*condition.name()).entered(); - condition.run_read_only((), world) + // SAFETY: `&World` gives us immutable access to the entire world. + // Since `condition` is `ReadOnlySystem`, it will never mutably access the world. + unsafe { condition.run_unsafe((), world.as_unsafe_world_cell_readonly()) } }) .fold(true, |acc, res| acc && res) } diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index cfe3da0ac0af5..231e44e31af14 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -84,21 +84,7 @@ pub trait System: Send + Sync + 'static { /// # Safety /// /// This must only be implemented for system types which do not mutate the `World`. -pub unsafe trait ReadOnlySystem: System { - /// Runs this system with the given input in the World. - /// - /// Unlike [`System::run`], this method can be run with a shared reference to the world. - fn run_read_only(&mut self, input: Self::In, world: &World) -> Self::Out { - self.update_archetype_component_access(world); - // SAFETY: - // -`&World` gives us immutable access to the entire world. - // The implementor of `Self` guarantees that the system will not - // mutate the world. - // - We have just called `update_archetype_component_access`, which will - // panic if the world is not valid. - unsafe { self.run_unsafe(world.as_unsafe_world_cell_readonly()) } - } -} +pub unsafe trait ReadOnlySystem: System {} /// A convenience type alias for a boxed [`System`] trait object. pub type BoxedSystem = Box>; From df64a3c7c93a3c43a3b80df9883a66abfa5a7adf Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 11:13:12 -0400 Subject: [PATCH 11/29] improve safety invariants in the executor --- .../src/schedule/executor/multi_threaded.rs | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index d678c00fd5a51..f8a85ad149e67 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -169,7 +169,6 @@ impl SystemExecutor for MultiThreadedExecutor { .map(|e| e.0.clone()); let thread_executor = thread_executor.as_deref(); - let world_cell = world.as_unsafe_world_cell(); let SyncUnsafeSchedule { systems, mut conditions, @@ -183,7 +182,10 @@ impl SystemExecutor for MultiThreadedExecutor { // alongside systems that claim the local thread let executor = async { while self.num_completed_systems < num_systems { - // SAFETY: self.ready_systems does not contain running systems + let world_cell = world.as_unsafe_world_cell(); + // SAFETY: + // - self.ready_systems does not contain running systems. + // - `world_cell` has mutable access to the entire world. unsafe { self.spawn_system_tasks(scope, systems, &mut conditions, world_cell); } @@ -256,8 +258,10 @@ impl MultiThreadedExecutor { } /// # Safety - /// Caller must ensure that `self.ready_systems` does not contain any systems that - /// have been mutably borrowed (such as the systems currently running). + /// - Caller must ensure that `self.ready_systems` does not contain any systems that + /// have been mutably borrowed (such as the systems currently running). + /// - `world_cell` must have permission to access all world data (not counting + /// any world data that is claimed by systems currently running on this executor). unsafe fn spawn_system_tasks( &mut self, scope: &Scope<()>, @@ -311,7 +315,10 @@ impl MultiThreadedExecutor { break; } - // SAFETY: No other reference to this system exists. + // SAFETY: + // - No other reference to this system exists. + // - `can_run` returned true, so no systems with conflicing world access + // are running. unsafe { self.spawn_system_task(scope, system_index, systems, world_cell); } @@ -421,7 +428,9 @@ impl MultiThreadedExecutor { } /// # Safety - /// Caller must not alias systems that are running. + /// - Caller must not alias systems that are running. + /// - `world` must have permission to access the world data + /// used by the specified system. unsafe fn spawn_system_task( &mut self, scope: &Scope<()>, @@ -442,7 +451,8 @@ impl MultiThreadedExecutor { #[cfg(feature = "trace")] let system_guard = system_span.enter(); let res = std::panic::catch_unwind(AssertUnwindSafe(|| { - // SAFETY: access is compatible + // SAFETY: The caller ensures that we have permission to + // access the world data used by the system. unsafe { system.run_unsafe((), world) }; })); #[cfg(feature = "trace")] From d8e70b4c323fa7149e761686a69dae189d87b2e1 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 11:17:25 -0400 Subject: [PATCH 12/29] use a more appropriate world access function --- crates/bevy_ecs/src/schedule/executor/multi_threaded.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index f8a85ad149e67..82849bb094bf3 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -286,7 +286,7 @@ impl MultiThreadedExecutor { // SAFETY: No exclusive system is running. // Therefore, there is no existing mutable reference to the world. - let world = unsafe { world_cell.world() }; + let world = unsafe { world_cell.world_metadata() }; if !self.can_run(system_index, system, conditions, world) { // NOTE: exclusive systems with ambiguities are susceptible to // being significantly displaced here (compared to single-threaded order) From 6e036cd94bf27aa33800e5c2ea2199b835e21b07 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 18:41:39 -0400 Subject: [PATCH 13/29] return `Tick` from `UnsafeWorldCell::increment_change_tick --- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 54afd95f7323a..14a74d035878b 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -202,12 +202,10 @@ impl<'w> UnsafeWorldCell<'w> { } #[inline] - pub fn increment_change_tick(self) -> u32 { + pub fn increment_change_tick(self) -> Tick { // SAFETY: // - we only access world metadata - unsafe { self.world_metadata() } - .change_tick - .fetch_add(1, Ordering::AcqRel) + unsafe { self.world_metadata() }.increment_change_tick() } /// Shorthand helper function for getting the [`ArchetypeComponentId`] for a resource. From 0fb60cacbc9bb4d145db9abc12f38318bbd50c24 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Sun, 2 Apr 2023 18:55:38 -0400 Subject: [PATCH 14/29] fix lifetimes --- .../src/schedule/executor/multi_threaded.rs | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 82849bb094bf3..54fce4feca81f 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -181,8 +181,8 @@ impl SystemExecutor for MultiThreadedExecutor { // the executor itself is a `Send` future so that it can run // alongside systems that claim the local thread let executor = async { + let world_cell = world.as_unsafe_world_cell(); while self.num_completed_systems < num_systems { - let world_cell = world.as_unsafe_world_cell(); // SAFETY: // - self.ready_systems does not contain running systems. // - `world_cell` has mutable access to the entire world. @@ -262,12 +262,12 @@ impl MultiThreadedExecutor { /// have been mutably borrowed (such as the systems currently running). /// - `world_cell` must have permission to access all world data (not counting /// any world data that is claimed by systems currently running on this executor). - unsafe fn spawn_system_tasks( + unsafe fn spawn_system_tasks<'scope>( &mut self, - scope: &Scope<()>, - systems: &[SyncUnsafeCell], + scope: &Scope<'_, 'scope, ()>, + systems: &'scope [SyncUnsafeCell], conditions: &mut Conditions, - world_cell: UnsafeWorldCell, + world_cell: UnsafeWorldCell<'scope>, ) { if self.exclusive_running { return; @@ -431,12 +431,12 @@ impl MultiThreadedExecutor { /// - Caller must not alias systems that are running. /// - `world` must have permission to access the world data /// used by the specified system. - unsafe fn spawn_system_task( + unsafe fn spawn_system_task<'scope>( &mut self, - scope: &Scope<()>, + scope: &Scope<'_, 'scope, ()>, system_index: usize, - systems: &[SyncUnsafeCell], - world: UnsafeWorldCell, + systems: &'scope [SyncUnsafeCell], + world: UnsafeWorldCell<'scope>, ) { // SAFETY: this system is not running, no other reference exists let system = unsafe { &mut *systems[system_index].get() }; @@ -485,12 +485,12 @@ impl MultiThreadedExecutor { /// # Safety /// Caller must ensure no systems are currently borrowed. - unsafe fn spawn_exclusive_system_task( + unsafe fn spawn_exclusive_system_task<'scope>( &mut self, - scope: &Scope<()>, + scope: &Scope<'_, 'scope, ()>, system_index: usize, - systems: &[SyncUnsafeCell], - world: &mut World, + systems: &'scope [SyncUnsafeCell], + world: &'scope mut World, ) { // SAFETY: this system is not running, no other reference exists let system = unsafe { &mut *systems[system_index].get() }; From 21066ebe6a83cbc855a312e8a12ec360647e79e2 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 3 Apr 2023 10:11:12 -0400 Subject: [PATCH 15/29] improve docs for `System::run` --- crates/bevy_ecs/src/system/system.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 231e44e31af14..8a4df6623b1f9 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -40,8 +40,8 @@ pub trait System: Send + Sync + 'static { fn is_exclusive(&self) -> bool; /// Runs the system with the given input in the world. Unlike [`System::run`], this function - /// takes [`UnsafeWorldCell`] and may therefore break Rust's aliasing rules, making - /// it unsafe to call. + /// can be called in parallel with other systems and may break Rust's aliasing rules + /// if used incorrectly, making it unsafe to call. /// /// # Safety /// From a1d47d3168666c2af8d2d9d9b6817fc557e0aaf9 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 3 Apr 2023 10:45:27 -0400 Subject: [PATCH 16/29] make condition evaluation function unsafe --- .../src/schedule/executor/multi_threaded.rs | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 54fce4feca81f..332e4f31715cb 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -284,10 +284,7 @@ impl MultiThreadedExecutor { // Therefore, no other reference to this system exists and there is no aliasing. let system = unsafe { &mut *systems[system_index].get() }; - // SAFETY: No exclusive system is running. - // Therefore, there is no existing mutable reference to the world. - let world = unsafe { world_cell.world_metadata() }; - if !self.can_run(system_index, system, conditions, world) { + if !self.can_run(system_index, system, conditions, world_cell) { // NOTE: exclusive systems with ambiguities are susceptible to // being significantly displaced here (compared to single-threaded order) // if systems after them in topological order can run @@ -297,7 +294,9 @@ impl MultiThreadedExecutor { self.ready_systems.set(system_index, false); - if !self.should_run(system_index, system, conditions, world) { + // SAFETY: `can_run` returned true, so there can be no systems + // running whose accesses would conflict with any conditions. + if !self.should_run(system_index, system, conditions, world_cell) { self.skip_system_and_signal_dependents(system_index); continue; } @@ -333,7 +332,7 @@ impl MultiThreadedExecutor { system_index: usize, system: &mut BoxedSystem, conditions: &mut Conditions, - world: &World, + world: UnsafeWorldCell, ) -> bool { let system_meta = &self.system_task_metadata[system_index]; if system_meta.is_exclusive && self.num_running_systems > 0 { @@ -344,6 +343,10 @@ impl MultiThreadedExecutor { return false; } + // SAFETY: We only use the world to update system archetype + // component accesses, which only accesses metadata. + let world = unsafe { world.world_metadata() }; + // TODO: an earlier out if world's archetypes did not change for set_idx in conditions.sets_with_conditions_of_systems[system_index] .difference(&self.evaluated_sets) @@ -388,12 +391,16 @@ impl MultiThreadedExecutor { true } - fn should_run( + /// # Safety + /// `world` must have permission to read any world data required by + /// the system's conditions: this includes conditions for the system + /// itself, and conditions for any of the system's sets. + unsafe fn should_run( &mut self, system_index: usize, _system: &BoxedSystem, conditions: &mut Conditions, - world: &World, + world: UnsafeWorldCell, ) -> bool { let mut should_run = !self.skipped_systems.contains(system_index); for set_idx in conditions.sets_with_conditions_of_systems[system_index].ones() { @@ -401,7 +408,9 @@ impl MultiThreadedExecutor { continue; } - // evaluate system set's conditions + // Evaluate the system set's conditions. + // SAFETY: The caller ensures that `world` has permission to read + // any data required by the conditions. let set_conditions_met = evaluate_and_fold_conditions(&mut conditions.set_conditions[set_idx], world); @@ -414,7 +423,9 @@ impl MultiThreadedExecutor { self.evaluated_sets.insert(set_idx); } - // evaluate system's conditions + // Evaluate the system's conditions. + // SAFETY: The caller ensures that `world` has permission to read + // any data required by the conditions. let system_conditions_met = evaluate_and_fold_conditions(&mut conditions.system_conditions[system_index], world); @@ -613,7 +624,13 @@ fn apply_system_buffers( } } -fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool { +/// # Safety +/// `world` must have permission to read any world data +/// required by `conditions`. +unsafe fn evaluate_and_fold_conditions( + conditions: &mut [BoxedCondition], + world: UnsafeWorldCell, +) -> bool { // not short-circuiting is intentional #[allow(clippy::unnecessary_fold)] conditions @@ -621,9 +638,9 @@ fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World .map(|condition| { #[cfg(feature = "trace")] let _condition_span = info_span!("condition", name = &*condition.name()).entered(); - // SAFETY: `&World` gives us immutable access to the entire world. - // Since `condition` is `ReadOnlySystem`, it will never mutably access the world. - unsafe { condition.run_unsafe((), world.as_unsafe_world_cell_readonly()) } + // SAFETY: The caller ensures that `world` has permission to + // access any data required by the condition. + unsafe { condition.run_unsafe((), world) } }) .fold(true, |acc, res| acc && res) } From 0cd81e83f9c14cebefdfc26031dc21e8991cb269 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 4 Apr 2023 14:23:35 -0400 Subject: [PATCH 17/29] add `UnsafeWorldCell::id` --- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 14a74d035878b..0a14cc24d06aa 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -1,6 +1,6 @@ #![warn(unsafe_op_in_unsafe_fn)] -use super::{Mut, World}; +use super::{Mut, World, WorldId}; use crate::{ archetype::{Archetype, ArchetypeComponentId, Archetypes}, bundle::Bundles, @@ -151,6 +151,13 @@ impl<'w> UnsafeWorldCell<'w> { unsafe { &*self.0 } } + /// Retrieve's this world's unique [ID](WorldId). + pub fn id(self) -> WorldId { + // SAFETY: + // - we only access world metadata + unsafe { self.world_metadata() }.id() + } + /// Retrieves this world's [Entities] collection #[inline] pub fn entities(self) -> &'w Entities { From 2da604fd66c1cc0c40fbfc307f38051f69e18395 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 4 Apr 2023 14:27:02 -0400 Subject: [PATCH 18/29] use `UnsafeWorldCell` for `update_archetype_component_access` --- crates/bevy_ecs/src/schedule/condition.rs | 2 +- .../bevy_ecs/src/schedule/executor/multi_threaded.rs | 4 ---- crates/bevy_ecs/src/system/combinator.rs | 2 +- .../bevy_ecs/src/system/exclusive_function_system.rs | 2 +- crates/bevy_ecs/src/system/function_system.rs | 2 +- crates/bevy_ecs/src/system/system.rs | 11 ++++++++--- 6 files changed, 12 insertions(+), 11 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index 4dba2a779dea3..7f00e6014e168 100644 --- a/crates/bevy_ecs/src/schedule/condition.rs +++ b/crates/bevy_ecs/src/schedule/condition.rs @@ -996,7 +996,7 @@ where self.condition.initialize(world); } - fn update_archetype_component_access(&mut self, world: &World) { + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { self.condition.update_archetype_component_access(world); } diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 332e4f31715cb..8e3630334e942 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -343,10 +343,6 @@ impl MultiThreadedExecutor { return false; } - // SAFETY: We only use the world to update system archetype - // component accesses, which only accesses metadata. - let world = unsafe { world.world_metadata() }; - // TODO: an earlier out if world's archetypes did not change for set_idx in conditions.sets_with_conditions_of_systems[system_index] .difference(&self.evaluated_sets) diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 07fda362c58e7..9012fc5fb5e64 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -197,7 +197,7 @@ where self.component_access.extend(self.b.component_access()); } - fn update_archetype_component_access(&mut self, world: &World) { + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { self.a.update_archetype_component_access(world); self.b.update_archetype_component_access(world); diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 48d31a9e9f652..bc336a17625ea 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -137,7 +137,7 @@ where self.param_state = Some(F::Param::init(world, &mut self.system_meta)); } - fn update_archetype_component_access(&mut self, _world: &World) {} + fn update_archetype_component_access(&mut self, _world: UnsafeWorldCell) {} #[inline] fn check_change_tick(&mut self, change_tick: Tick) { diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 34ebbbdea69cb..d2866edc72a30 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -515,7 +515,7 @@ where self.param_state = Some(F::Param::init_state(world, &mut self.system_meta)); } - fn update_archetype_component_access(&mut self, world: &World) { + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { assert!(self.world_id == Some(world.id()), "Encountered a mismatched World. A System cannot be used with Worlds other than the one it was initialized with."); let archetypes = world.archetypes(); let new_generation = archetypes.generation(); diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 8a4df6623b1f9..28e0dacf05f68 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -51,15 +51,20 @@ pub trait System: Send + Sync + 'static { unsafe fn run_unsafe(&mut self, input: Self::In, world: UnsafeWorldCell) -> Self::Out; /// Runs the system with the given input in the world. fn run(&mut self, input: Self::In, world: &mut World) -> Self::Out { + let world = world.as_unsafe_world_cell(); self.update_archetype_component_access(world); - // SAFETY: world and resources are exclusively borrowed - unsafe { self.run_unsafe(input, world.as_unsafe_world_cell()) } + // SAFETY: We have exclusive access to the entire world. + unsafe { self.run_unsafe(input, world) } } fn apply_buffers(&mut self, world: &mut World); /// Initialize the system. fn initialize(&mut self, _world: &mut World); /// Update the system's archetype component [`Access`]. - fn update_archetype_component_access(&mut self, world: &World); + /// + /// ## Note for implementors + /// `world` may only be used to access metadata. This can be done in safe code + /// via functions such as [`UnsafeWorldCell::archetypes`]. + fn update_archetype_component_access(&mut self, world: UnsafeWorldCell); fn check_change_tick(&mut self, change_tick: Tick); /// Returns the system's default [system sets](crate::schedule::SystemSet). fn default_system_sets(&self) -> Vec> { From 3eedf5c703debdc2dbd72104fe6ea52c53393ddb Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 4 Apr 2023 14:36:00 -0400 Subject: [PATCH 19/29] update callsites in doctests --- crates/bevy_ecs/src/system/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/system/mod.rs b/crates/bevy_ecs/src/system/mod.rs index 1ce4bfc17f915..595670b46890e 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1209,7 +1209,7 @@ mod tests { // set up system and verify its access is empty system.initialize(&mut world); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); assert_eq!( system .archetype_component_access() @@ -1239,7 +1239,7 @@ mod tests { world.spawn((B, C)); // update system and verify its accesses are correct - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); assert_eq!( system .archetype_component_access() @@ -1257,7 +1257,7 @@ mod tests { .unwrap(), ); world.spawn((A, B, D)); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); assert_eq!( system .archetype_component_access() From 9b934c0e29c32d01a278a44d7ccfe53a9c8eb19b Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 4 Apr 2023 14:46:10 -0400 Subject: [PATCH 20/29] split an unsafe block --- crates/bevy_ecs/src/schedule/executor/multi_threaded.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 8e3630334e942..7de209c1ae939 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -305,10 +305,12 @@ impl MultiThreadedExecutor { self.num_running_systems += 1; if self.system_task_metadata[system_index].is_exclusive { - // SAFETY: `can_run` confirmed that no systems are running. - // Therefore, there is no existing reference to the world. + // SAFETY: `can_run` returned true for this system, which means + // that no other systems currently have access to the world. + let world = unsafe { world_cell.world_mut() }; + // SAFETY: Since no systems are currently running, there are + // no existing borrows to the system at `system_index`. unsafe { - let world = world_cell.world_mut(); self.spawn_exclusive_system_task(scope, system_index, systems, world); } break; From 432d6d77bcca56d11ced3d07f582d23001e23a38 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 4 Apr 2023 14:48:17 -0400 Subject: [PATCH 21/29] update some benchmarks --- benches/benches/bevy_ecs/iteration/heavy_compute.rs | 2 +- benches/benches/bevy_ecs/iteration/iter_simple_system.rs | 2 +- benches/benches/bevy_ecs/world/world_get.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/benches/benches/bevy_ecs/iteration/heavy_compute.rs b/benches/benches/bevy_ecs/iteration/heavy_compute.rs index 99b3fdb7649da..9795c82159342 100644 --- a/benches/benches/bevy_ecs/iteration/heavy_compute.rs +++ b/benches/benches/bevy_ecs/iteration/heavy_compute.rs @@ -45,7 +45,7 @@ pub fn heavy_compute(c: &mut Criterion) { let mut system = IntoSystem::into_system(sys); system.initialize(&mut world); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); b.iter(move || system.run((), &mut world)); }); diff --git a/benches/benches/bevy_ecs/iteration/iter_simple_system.rs b/benches/benches/bevy_ecs/iteration/iter_simple_system.rs index fc90878c58753..2b09ada53eb25 100644 --- a/benches/benches/bevy_ecs/iteration/iter_simple_system.rs +++ b/benches/benches/bevy_ecs/iteration/iter_simple_system.rs @@ -37,7 +37,7 @@ impl Benchmark { let mut system = IntoSystem::into_system(query_system); system.initialize(&mut world); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); Self(world, Box::new(system)) } diff --git a/benches/benches/bevy_ecs/world/world_get.rs b/benches/benches/bevy_ecs/world/world_get.rs index 4fc7ed51046b2..ee63498a095f1 100644 --- a/benches/benches/bevy_ecs/world/world_get.rs +++ b/benches/benches/bevy_ecs/world/world_get.rs @@ -291,7 +291,7 @@ pub fn query_get_component_simple(criterion: &mut Criterion) { let mut system = IntoSystem::into_system(query_system); system.initialize(&mut world); - system.update_archetype_component_access(&world); + system.update_archetype_component_access(world.as_unsafe_world_cell()); bencher.iter(|| system.run(entity, &mut world)); }); From 07f9302d2d87df2886438a71a0a8bf07b9cca46c Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 4 Apr 2023 18:40:43 -0400 Subject: [PATCH 22/29] tweak a safety comment --- crates/bevy_ecs/src/schedule/executor/multi_threaded.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 7de209c1ae939..32188ea576342 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -308,8 +308,8 @@ impl MultiThreadedExecutor { // SAFETY: `can_run` returned true for this system, which means // that no other systems currently have access to the world. let world = unsafe { world_cell.world_mut() }; - // SAFETY: Since no systems are currently running, there are - // no existing borrows to the system at `system_index`. + // SAFETY: `can_run` returned true for this system, + // which means no systems are currently borrowed. unsafe { self.spawn_exclusive_system_task(scope, system_index, systems, world); } From 59dd4b7809a2c24c04c67c788f3fddc3b9b07d8d Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Wed, 12 Apr 2023 20:52:42 -0400 Subject: [PATCH 23/29] simplify `impl SystemParam for WorldId` --- crates/bevy_ecs/src/world/identifier.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs index e5f8d7fde72ae..cd9fa701f83ab 100644 --- a/crates/bevy_ecs/src/world/identifier.rs +++ b/crates/bevy_ecs/src/world/identifier.rs @@ -44,10 +44,10 @@ impl FromWorld for WorldId { } } -// SAFETY: Has read-only access to shared World metadata +// SAFETY: No world data access is required. unsafe impl ReadOnlySystemParam for WorldId {} -// SAFETY: A World's ID is immutable and fetching it from the World does not borrow anything +// SAFETY: No world data access is required. unsafe impl SystemParam for WorldId { type State = (); @@ -61,7 +61,7 @@ unsafe impl SystemParam for WorldId { world: UnsafeWorldCell<'world>, _: Tick, ) -> Self::Item<'world, 'state> { - world.world_metadata().id() + world.id() } } From 840dd950969c2c8538c4ac0cfa55fa43172ab778 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 17 Apr 2023 12:02:06 -0400 Subject: [PATCH 24/29] fix formatting for a comment --- crates/bevy_ecs/src/schedule/executor/multi_threaded.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index bdd1fa712b8c8..03827f0960cc6 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -706,7 +706,7 @@ fn apply_system_buffers( /// # Safety /// - `world` must have permission to read any world data /// required by `conditions`. -/// `update_archetype_component_access` must have been called +/// - `update_archetype_component_access` must have been called /// with `world` for each condition in `conditions`. unsafe fn evaluate_and_fold_conditions( conditions: &mut [BoxedCondition], From 02334b5a0af5488635b300f8e03cd7715c830e50 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 18 Apr 2023 23:14:01 -0400 Subject: [PATCH 25/29] remove an apostrophe --- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 1f6c63154ce27..0fc41b7db4b9d 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -157,7 +157,7 @@ impl<'w> UnsafeWorldCell<'w> { unsafe { &*self.0 } } - /// Retrieve's this world's unique [ID](WorldId). + /// Retrieves this world's unique [ID](WorldId). pub fn id(self) -> WorldId { // SAFETY: // - we only access world metadata From 2615867683e66cdd9e94460b920495db0f036867 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 18 Apr 2023 23:15:52 -0400 Subject: [PATCH 26/29] rephrase a safety comment --- crates/bevy_ecs/src/world/identifier.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs index cd9fa701f83ab..626964577d83f 100644 --- a/crates/bevy_ecs/src/world/identifier.rs +++ b/crates/bevy_ecs/src/world/identifier.rs @@ -44,10 +44,10 @@ impl FromWorld for WorldId { } } -// SAFETY: No world data access is required. +// SAFETY: No world data is accessed. unsafe impl ReadOnlySystemParam for WorldId {} -// SAFETY: No world data access is required. +// SAFETY: No world data is accessed. unsafe impl SystemParam for WorldId { type State = (); From 702f09fb80e615bf56a67685793a8f6468b1ebf8 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 18 Apr 2023 23:17:58 -0400 Subject: [PATCH 27/29] rephrase another safety comment --- crates/bevy_ecs/src/system/system.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index e2618b14d1c39..efd0d754986ff 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -46,8 +46,8 @@ pub trait System: Send + Sync + 'static { /// # Safety /// /// - The caller must ensure that `world` has permission to access any world data - /// registered in [`Self::archetype_component_access`]. No systems with conflicting - /// data access may run at the same time. + /// registered in [`Self::archetype_component_access`]. There must be no conflicing + /// accesses while the system is running. /// - The method [`Self::update_archetype_component_access`] must be called at some /// point before this one, with the same exact [`World`]. If `update_archetype_component_access` /// panics (or otherwise does not return for any reason), this method must not be called. From 5e11ad59c58a07b1584ea61dd617c6fc7caafffb Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 9 May 2023 08:54:57 -0400 Subject: [PATCH 28/29] Apply suggestions from code review Co-authored-by: James Liu --- crates/bevy_ecs/src/system/system.rs | 4 ++-- crates/bevy_ecs/src/world/unsafe_world_cell.rs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index efd0d754986ff..4a578ec6797ec 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -46,8 +46,8 @@ pub trait System: Send + Sync + 'static { /// # Safety /// /// - The caller must ensure that `world` has permission to access any world data - /// registered in [`Self::archetype_component_access`]. There must be no conflicing - /// accesses while the system is running. + /// registered in [`Self::archetype_component_access`]. There must be no conflicting + /// simultaneous accesses while the system is running. /// - The method [`Self::update_archetype_component_access`] must be called at some /// point before this one, with the same exact [`World`]. If `update_archetype_component_access` /// panics (or otherwise does not return for any reason), this method must not be called. diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index 0fc41b7db4b9d..a33188f35d99e 100644 --- a/crates/bevy_ecs/src/world/unsafe_world_cell.rs +++ b/crates/bevy_ecs/src/world/unsafe_world_cell.rs @@ -158,6 +158,7 @@ impl<'w> UnsafeWorldCell<'w> { } /// Retrieves this world's unique [ID](WorldId). + #[inline] pub fn id(self) -> WorldId { // SAFETY: // - we only access world metadata From 4e6630ddfa3d03b14f2e3bccda180f3bba5906df Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 9 May 2023 08:57:03 -0400 Subject: [PATCH 29/29] Update crates/bevy_ecs/src/schedule/executor/multi_threaded.rs --- crates/bevy_ecs/src/schedule/executor/multi_threaded.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 03827f0960cc6..30c711d373f0c 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -347,7 +347,7 @@ impl MultiThreadedExecutor { // SAFETY: // - No other reference to this system exists. // - `can_run` has been called, which calls `update_archetype_component_access` with this system. - // - `can_run` returned true, so no systems with conflicing world access are running. + // - `can_run` returned true, so no systems with conflicting world access are running. unsafe { self.spawn_system_task(scope, system_index, systems, world_cell); }