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)); }); diff --git a/crates/bevy_ecs/src/schedule/condition.rs b/crates/bevy_ecs/src/schedule/condition.rs index 24d80040cf2af..52630acc2b37b 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>; @@ -990,7 +991,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) } @@ -1007,7 +1008,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 f6ba57e5329f3..569320c03117d 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -21,7 +21,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; @@ -184,7 +184,6 @@ impl SystemExecutor for MultiThreadedExecutor { .map(|e| e.0.clone()); let thread_executor = thread_executor.as_deref(); - let world = SyncUnsafeCell::from_mut(world); let SyncUnsafeSchedule { systems, mut conditions, @@ -197,10 +196,13 @@ 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 < self.num_systems { - // SAFETY: self.ready_systems does not contain running systems + // 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); + self.spawn_system_tasks(scope, systems, &mut conditions, world_cell); } if self.num_running_systems > 0 { @@ -231,7 +233,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 - let res = apply_system_buffers(&self.unapplied_systems, systems, world.get_mut()); + let res = apply_system_buffers(&self.unapplied_systems, systems, world); if let Err(payload) = res { let mut panic_payload = self.panic_payload.lock().unwrap(); *panic_payload = Some(payload); @@ -283,14 +285,16 @@ 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<'scope>( &mut self, scope: &Scope<'_, 'scope, ()>, systems: &'scope [SyncUnsafeCell], conditions: &mut Conditions, - cell: &'scope SyncUnsafeCell, + world_cell: UnsafeWorldCell<'scope>, ) { if self.exclusive_running { return; @@ -307,10 +311,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 { &*cell.get() }; - 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 @@ -320,9 +321,10 @@ impl MultiThreadedExecutor { self.ready_systems.set(system_index, false); - // SAFETY: Since `self.can_run` returned true earlier, it must have called - // `update_archetype_component_access` for each run condition. - if !self.should_run(system_index, system, conditions, world) { + // SAFETY: `can_run` returned true, which means that: + // - It must have called `update_archetype_component_access` for each run condition. + // - 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; } @@ -331,10 +333,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: `can_run` returned true for this system, + // which means no systems are currently borrowed. unsafe { - let world = &mut *cell.get(); self.spawn_exclusive_system_task(scope, system_index, systems, world); } break; @@ -342,9 +346,10 @@ impl MultiThreadedExecutor { // SAFETY: // - No other reference to this system exists. - // - `self.can_run` has been called, which calls `update_archetype_component_access` with this system. + // - `can_run` has been called, which calls `update_archetype_component_access` with this system. + // - `can_run` returned true, so no systems with conflicting world access are running. unsafe { - self.spawn_system_task(scope, system_index, systems, world); + self.spawn_system_task(scope, system_index, systems, world_cell); } } @@ -357,7 +362,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 { @@ -413,15 +418,17 @@ impl MultiThreadedExecutor { } /// # Safety - /// - /// `update_archetype_component` must have been called with `world` - /// for each run condition in `conditions`. + /// * `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. + /// * `update_archetype_component` must have been called with `world` + /// for each run condition in `conditions`. 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() { @@ -430,7 +437,10 @@ impl MultiThreadedExecutor { } // Evaluate the system set's conditions. - // SAFETY: `update_archetype_component_access` has been called for each run condition. + // SAFETY: + // - The caller ensures that `world` has permission to read any data + // required by the conditions. + // - `update_archetype_component_access` has been called for each run condition. let set_conditions_met = evaluate_and_fold_conditions(&mut conditions.set_conditions[set_idx], world); @@ -444,7 +454,10 @@ impl MultiThreadedExecutor { } // Evaluate the system's conditions. - // SAFETY: `update_archetype_component_access` has been called for each run condition. + // SAFETY: + // - The caller ensures that `world` has permission to read any data + // required by the conditions. + // - `update_archetype_component_access` has been called for each run condition. let system_conditions_met = evaluate_and_fold_conditions(&mut conditions.system_conditions[system_index], world); @@ -459,6 +472,8 @@ impl MultiThreadedExecutor { /// # Safety /// - Caller must not alias systems that are running. + /// - `world` must have permission to access the world data + /// used by the specified system. /// - `update_archetype_component_access` must have been called with `world` /// on the system assocaited with `system_index`. unsafe fn spawn_system_task<'scope>( @@ -466,7 +481,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() }; @@ -483,7 +498,8 @@ impl MultiThreadedExecutor { let system_guard = system_span.enter(); let res = std::panic::catch_unwind(AssertUnwindSafe(|| { // SAFETY: - // - Access: TODO. + // - The caller ensures that we have permission to + // access the world data used by the system. // - `update_archetype_component_access` has been called. unsafe { system.run_unsafe((), world) }; })); @@ -688,10 +704,14 @@ fn apply_system_buffers( } /// # Safety -/// -/// `update_archetype_component_access` must have been called -/// with `world` for each condition in `conditions`. -unsafe fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool { +/// - `world` must have permission to read any world data +/// required by `conditions`. +/// - `update_archetype_component_access` must have been called +/// with `world` for each condition in `conditions`. +unsafe fn evaluate_and_fold_conditions( + conditions: &mut [BoxedCondition], + world: UnsafeWorldCell, +) -> bool { // not short-circuiting is intentional #[allow(clippy::unnecessary_fold)] conditions @@ -699,7 +719,8 @@ unsafe fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: .map(|condition| { #[cfg(feature = "trace")] let _condition_span = info_span!("condition", name = &*condition.name()).entered(); - // SAFETY: caller ensures system access is compatible + // 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) diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 32ca444137654..c058a78f35b9d 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, @@ -198,7 +199,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 c1c4bff3a2a86..9ba43b4015fec 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, + world::{unsafe_world_cell::UnsafeWorldCell, World}, }; use bevy_utils::all_tuples; @@ -86,7 +86,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"); } @@ -134,7 +134,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 27593cfef98af..ed73d75b8f60d 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; @@ -417,7 +417,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 { let change_tick = world.increment_change_tick(); // SAFETY: @@ -428,7 +428,7 @@ where 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); @@ -457,7 +457,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/mod.rs b/crates/bevy_ecs/src/system/mod.rs index c322e1686142c..9bb99138da733 100644 --- a/crates/bevy_ecs/src/system/mod.rs +++ b/crates/bevy_ecs/src/system/mod.rs @@ -1610,7 +1610,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() @@ -1640,7 +1640,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() @@ -1658,7 +1658,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() diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index ae4a9fc074033..4a578ec6797ec 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; @@ -39,26 +40,24 @@ 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 - /// 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 /// - /// 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()`]. - /// - /// Additionally, 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. - unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> Self::Out; + /// - The caller must ensure that `world` has permission to access any world data + /// 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. + 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, which ensures no data access conflicts. + // - We have exclusive access to the entire world. // - `update_archetype_component_access` has been called. unsafe { self.run_unsafe(input, world) } } @@ -66,7 +65,11 @@ pub trait System: Send + Sync + 'static { /// 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> { diff --git a/crates/bevy_ecs/src/world/identifier.rs b/crates/bevy_ecs/src/world/identifier.rs index e5f8d7fde72ae..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: Has read-only access to shared World metadata +// SAFETY: No world data is accessed. 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 is accessed. 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() } } diff --git a/crates/bevy_ecs/src/world/unsafe_world_cell.rs b/crates/bevy_ecs/src/world/unsafe_world_cell.rs index fe14ed8bfc974..9cbab05d276b3 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, @@ -190,6 +190,14 @@ impl<'w> UnsafeWorldCell<'w> { unsafe { &*self.0 } } + /// Retrieves this world's unique [ID](WorldId). + #[inline] + 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 {