From 6fe4b1440c3a0557a762e4a8850cb5735f694d39 Mon Sep 17 00:00:00 2001 From: Christian Hughes <9044780+ItsDoot@users.noreply.github.com> Date: Sun, 1 Dec 2024 14:09:22 -0600 Subject: [PATCH] Refactor `FunctionSystem` to use a single `Option` (#16514) # Objective Combine the `Option<_>` state in `FunctionSystem` into a single `Option` to provide clarity and save space. ## Solution Simplifies `FunctionSystem`'s layout by using a single `Option` for state that must be initialized before running, and saves a byte by removing the need to store an enum tag. Additionally, calling `System::run` on an uninitialized `System` will now give a more descriptive message prior to verifying the `WorldId`. ## Testing Ran CI checks locally. --- crates/bevy_ecs/src/system/function_system.rs | 66 +++++++++++-------- 1 file changed, 38 insertions(+), 28 deletions(-) diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 78f310ccc7..43f6a33502 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -395,9 +395,11 @@ impl SystemState { ) -> FunctionSystem { FunctionSystem { func, - param_state: Some(self.param_state), + state: Some(FunctionSystemState { + param: self.param_state, + world_id: self.world_id, + }), system_meta: self.meta, - world_id: Some(self.world_id), archetype_generation: self.archetype_generation, marker: PhantomData, } @@ -602,14 +604,25 @@ where F: SystemParamFunction, { func: F, - pub(crate) param_state: Option<::State>, - pub(crate) system_meta: SystemMeta, - world_id: Option, + state: Option>, + system_meta: SystemMeta, archetype_generation: ArchetypeGeneration, // NOTE: PhantomData T> gives this safe Send/Sync impls marker: PhantomData Marker>, } +/// The state of a [`FunctionSystem`], which must be initialized with +/// [`System::initialize`] before the system can be run. A panic will occur if +/// the system is run without being initialized. +struct FunctionSystemState { + /// The cached state of the system's [`SystemParam`]s. + param: P::State, + /// The id of the [`World`] this system was initialized with. If the world + /// passed to [`System::update_archetype_component_access`] does not match + /// this id, a panic will occur. + world_id: WorldId, +} + impl FunctionSystem where F: SystemParamFunction, @@ -631,9 +644,8 @@ where fn clone(&self) -> Self { Self { func: self.func.clone(), - param_state: None, + state: None, system_meta: SystemMeta::new::(), - world_id: None, archetype_generation: ArchetypeGeneration::initial(), marker: PhantomData, } @@ -653,9 +665,8 @@ where fn into_system(func: Self) -> Self::System { FunctionSystem { func, - param_state: None, + state: None, system_meta: SystemMeta::new::(), - world_id: None, archetype_generation: ArchetypeGeneration::initial(), marker: PhantomData, } @@ -669,7 +680,8 @@ where /// Message shown when a system isn't initialized // When lines get too long, rustfmt can sometimes refuse to format them. // Work around this by storing the message separately. - const PARAM_MESSAGE: &'static str = "System's param_state was not found. Did you forget to initialize this system before running it?"; + const ERROR_UNINITIALIZED: &'static str = + "System's state was not found. Did you forget to initialize this system before running it?"; } impl System for FunctionSystem @@ -721,19 +733,14 @@ where let change_tick = world.increment_change_tick(); + let param_state = &mut self.state.as_mut().expect(Self::ERROR_UNINITIALIZED).param; // SAFETY: // - 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 = unsafe { - F::Param::get_param( - self.param_state.as_mut().expect(Self::PARAM_MESSAGE), - &self.system_meta, - world, - change_tick, - ) - }; + let params = + unsafe { F::Param::get_param(param_state, &self.system_meta, world, change_tick) }; let out = self.func.run(input, params); self.system_meta.last_run = change_tick; out @@ -741,19 +748,19 @@ where #[inline] fn apply_deferred(&mut self, world: &mut World) { - let param_state = self.param_state.as_mut().expect(Self::PARAM_MESSAGE); + let param_state = &mut self.state.as_mut().expect(Self::ERROR_UNINITIALIZED).param; F::Param::apply(param_state, &self.system_meta, world); } #[inline] fn queue_deferred(&mut self, world: DeferredWorld) { - let param_state = self.param_state.as_mut().expect(Self::PARAM_MESSAGE); + let param_state = &mut self.state.as_mut().expect(Self::ERROR_UNINITIALIZED).param; F::Param::queue(param_state, &self.system_meta, world); } #[inline] unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { - let param_state = self.param_state.as_ref().expect(Self::PARAM_MESSAGE); + let param_state = &self.state.as_ref().expect(Self::ERROR_UNINITIALIZED).param; // SAFETY: // - The caller has invoked `update_archetype_component_access`, which will panic // if the world does not match. @@ -768,29 +775,32 @@ where #[inline] fn initialize(&mut self, world: &mut World) { - if let Some(id) = self.world_id { + if let Some(state) = &self.state { assert_eq!( - id, + state.world_id, world.id(), "System built with a different world than the one it was added to.", ); } else { - self.world_id = Some(world.id()); - self.param_state = Some(F::Param::init_state(world, &mut self.system_meta)); + self.state = Some(FunctionSystemState { + param: F::Param::init_state(world, &mut self.system_meta), + world_id: world.id(), + }); } self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX); } fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { - assert_eq!(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 state = self.state.as_mut().expect(Self::ERROR_UNINITIALIZED); + assert_eq!(state.world_id, 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 old_generation = core::mem::replace(&mut self.archetype_generation, archetypes.generation()); for archetype in &archetypes[old_generation..] { - let param_state = self.param_state.as_mut().unwrap(); // SAFETY: The assertion above ensures that the param_state was initialized from `world`. - unsafe { F::Param::new_archetype(param_state, archetype, &mut self.system_meta) }; + unsafe { F::Param::new_archetype(&mut state.param, archetype, &mut self.system_meta) }; } }