From f9f7928234f7179d451c7354cb44a5f566ac43a1 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 21 Feb 2023 11:13:11 -0500 Subject: [PATCH 01/10] use safety invariants to validate the world --- crates/bevy_ecs/src/system/function_system.rs | 9 +++++---- crates/bevy_ecs/src/system/system.rs | 8 +++++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 07e801898a011..c572758e080d0 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -457,10 +457,11 @@ where unsafe fn run_unsafe(&mut self, input: Self::In, world: &World) -> 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: + // - All world accesses have been registered, so the caller will ensure that + // there are no data access conflicts. + // - The caller has invoked `update_archetype_component_access`, which will panic + // if the world does not match. let params = F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 78980e9063b81..987d7eb90a3d6 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -51,11 +51,17 @@ 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()`]. + /// + /// 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; /// 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 + // SAFETY: + // - World and resources are exclusively borrowed, which ensures no data access conflicts. + // - `update_archetype_component_access` has been called. unsafe { self.run_unsafe(input, world) } } fn apply_buffers(&mut self, world: &mut World); From ac6ab969ed80a980c2c7dc7c9ea52a16d3fb3d52 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 21 Feb 2023 11:22:10 -0500 Subject: [PATCH 02/10] fix safety invariants for parallel systems --- .../bevy_ecs/src/schedule/executor/multi_threaded.rs | 12 +++++++++--- 1 file changed, 9 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 16ab5f789675d..801b0c46bbeec 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -309,7 +309,9 @@ impl MultiThreadedExecutor { break; } - // SAFETY: No other reference to this system exists. + // SAFETY: + // - No other reference to this system exists. + // - `self.can_run` has been called, which calls `update_archetype_component_access` with this system. unsafe { self.spawn_system_task(scope, system_index, systems, world); } @@ -419,7 +421,9 @@ impl MultiThreadedExecutor { } /// # Safety - /// Caller must not alias systems that are running. + /// - Caller must not alias systems that are running. + /// - `update_archetype_component_access` must have been called with `world` + /// on the system assocaited with `system_index`. unsafe fn spawn_system_task<'scope>( &mut self, scope: &Scope<'_, 'scope, ()>, @@ -440,7 +444,9 @@ impl MultiThreadedExecutor { #[cfg(feature = "trace")] let system_guard = system_span.enter(); let res = std::panic::catch_unwind(AssertUnwindSafe(|| { - // SAFETY: access is compatible + // SAFETY: + // - Access is compatible. + // - `update_archetype_component_access` has been called. unsafe { system.run_unsafe((), world) }; })); #[cfg(feature = "trace")] From d859112242c5419e84e02c161a410c0926fff821 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 21 Feb 2023 11:32:31 -0500 Subject: [PATCH 03/10] fix safety invariants for run conditions --- .../src/schedule/executor/multi_threaded.rs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs index 801b0c46bbeec..095f76177f8b3 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -291,6 +291,8 @@ impl MultiThreadedExecutor { self.ready_systems.set(system_index, false); + // SAFETY: If `self.can_run` returns true, then it must have called + // `update_archetype_component_access` for each run condition. if !self.should_run(system_index, system, conditions, world) { self.skip_system_and_signal_dependents(system_index); continue; @@ -381,7 +383,11 @@ impl MultiThreadedExecutor { true } - fn should_run( + /// # Safety + /// + /// `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, @@ -394,7 +400,8 @@ impl MultiThreadedExecutor { continue; } - // evaluate system set's conditions + // Evaluate the system set's conditions. + // SAFETY: `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); @@ -407,7 +414,8 @@ impl MultiThreadedExecutor { self.evaluated_sets.insert(set_idx); } - // evaluate system's conditions + // Evaluate the system's conditions. + // SAFETY: `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); @@ -610,7 +618,11 @@ fn apply_system_buffers( } } -fn evaluate_and_fold_conditions(conditions: &mut [BoxedCondition], world: &World) -> bool { +/// # 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 { // not short-circuiting is intentional #[allow(clippy::unnecessary_fold)] conditions From 8d33e13760dcf176a463a26e34a44d35d52337fb Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 21 Feb 2023 11:33:37 -0500 Subject: [PATCH 04/10] make some phrasing more clear --- 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 095f76177f8b3..3c6c983b6af4d 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -291,7 +291,7 @@ impl MultiThreadedExecutor { self.ready_systems.set(system_index, false); - // SAFETY: If `self.can_run` returns true, then it must have called + // SAFETY: Since `self.can_run` returned true, it must have called // `update_archetype_component_access` for each run condition. if !self.should_run(system_index, system, conditions, world) { self.skip_system_and_signal_dependents(system_index); From 5f3a0c3c1f61e67569417ff0ef0dfde54f021d3d Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 21 Feb 2023 11:46:38 -0500 Subject: [PATCH 05/10] make a safety comment more precise --- crates/bevy_ecs/src/system/function_system.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index c572758e080d0..0ab269e1891cd 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -458,10 +458,10 @@ where let change_tick = world.increment_change_tick(); // SAFETY: - // - All world accesses have been registered, so the caller will ensure that - // there are no data access conflicts. // - The caller has invoked `update_archetype_component_access`, which will panic // if the world does not match. + // - All world accesses used by `F::Param` have been registered, so the caller + // will ensure that there are no data access conflicts. let params = F::Param::get_param( self.param_state.as_mut().expect(Self::PARAM_MESSAGE), &self.system_meta, From d968a99178e1e54aedc9a6169e3db44726778952 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 21 Feb 2023 11:48:46 -0500 Subject: [PATCH 06/10] add a safety invariant to `CombinatorSystem` --- crates/bevy_ecs/src/system/combinator.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/bevy_ecs/src/system/combinator.rs b/crates/bevy_ecs/src/system/combinator.rs index 7de7b9b053868..b064c3e026a2a 100644 --- a/crates/bevy_ecs/src/system/combinator.rs +++ b/crates/bevy_ecs/src/system/combinator.rs @@ -161,6 +161,8 @@ where // so the caller will guarantee that no other systems will conflict with `a` or `b`. // Since these closures are `!Send + !Synd + !'static`, they can never be called // in parallel, so their world accesses will not conflict with each other. + // Additionally, `update_archetype_component_access` has been called, + // which forwards to the implementations for `self.a` and `self.b`. |input| self.a.run_unsafe(input, world), |input| self.b.run_unsafe(input, world), ) From a62659ae1db82c10cd9facee84b32f7af2d42973 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Tue, 21 Feb 2023 11:54:29 -0500 Subject: [PATCH 07/10] remove unused `WorldId` from `ExclusiveFunctionSystem` --- crates/bevy_ecs/src/system/exclusive_function_system.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index 4ba4a15418655..6165c9dae2164 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -7,7 +7,7 @@ use crate::{ check_system_change_tick, ExclusiveSystemParam, ExclusiveSystemParamItem, In, InputMarker, IntoSystem, System, SystemMeta, }, - world::{World, WorldId}, + world::World, }; use bevy_utils::all_tuples; @@ -26,7 +26,6 @@ where func: F, param_state: Option<::State>, system_meta: SystemMeta, - world_id: Option, // NOTE: PhantomData T> gives this safe Send/Sync impls marker: PhantomData Marker>, } @@ -44,7 +43,6 @@ where func, param_state: None, system_meta: SystemMeta::new::(), - world_id: None, marker: PhantomData, } } @@ -133,7 +131,6 @@ where #[inline] fn initialize(&mut self, world: &mut World) { - self.world_id = Some(world.id()); self.system_meta.last_change_tick = world.change_tick().wrapping_sub(MAX_CHANGE_AGE); self.param_state = Some(F::Param::init(world, &mut self.system_meta)); } From 3edc0e8db9f1f7193c27c47397969e6f107d7bc9 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Mon, 27 Feb 2023 10:48:19 -0500 Subject: [PATCH 08/10] add a TODO --- 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 3c6c983b6af4d..df0e8ec131ea7 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -453,7 +453,7 @@ impl MultiThreadedExecutor { let system_guard = system_span.enter(); let res = std::panic::catch_unwind(AssertUnwindSafe(|| { // SAFETY: - // - Access is compatible. + // - Access: TODO. // - `update_archetype_component_access` has been called. unsafe { system.run_unsafe((), world) }; })); From 846f91274a615e59ee9df43ca859fcce1db5ce40 Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 12:53:19 -0500 Subject: [PATCH 09/10] clarify 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 635e07fe7a789..55c9ca20001ec 100644 --- a/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs +++ b/crates/bevy_ecs/src/schedule/executor/multi_threaded.rs @@ -295,7 +295,7 @@ impl MultiThreadedExecutor { self.ready_systems.set(system_index, false); - // SAFETY: Since `self.can_run` returned true, it must have called + // 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) { self.skip_system_and_signal_dependents(system_index); From 6333985bbd3310710721091840d9756da957affd Mon Sep 17 00:00:00 2001 From: JoJoJet <21144246+JoJoJet@users.noreply.github.com> Date: Thu, 9 Mar 2023 12:57:07 -0500 Subject: [PATCH 10/10] fix a merge error --- crates/bevy_ecs/src/system/exclusive_function_system.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/system/exclusive_function_system.rs b/crates/bevy_ecs/src/system/exclusive_function_system.rs index af047cc6f96ef..97304332af7e6 100644 --- a/crates/bevy_ecs/src/system/exclusive_function_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_function_system.rs @@ -131,6 +131,7 @@ where #[inline] fn initialize(&mut self, world: &mut World) { self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX); + self.param_state = Some(F::Param::init(world, &mut self.system_meta)); } fn update_archetype_component_access(&mut self, _world: &World) {}