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<FunctionSystemState>` 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.
This commit is contained in:
Christian Hughes 2024-12-01 14:09:22 -06:00 committed by GitHub
parent 4282c3fe40
commit 6fe4b1440c
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -395,9 +395,11 @@ impl<Param: SystemParam> SystemState<Param> {
) -> FunctionSystem<Marker, F> { ) -> FunctionSystem<Marker, F> {
FunctionSystem { FunctionSystem {
func, func,
param_state: Some(self.param_state), state: Some(FunctionSystemState {
param: self.param_state,
world_id: self.world_id,
}),
system_meta: self.meta, system_meta: self.meta,
world_id: Some(self.world_id),
archetype_generation: self.archetype_generation, archetype_generation: self.archetype_generation,
marker: PhantomData, marker: PhantomData,
} }
@ -602,14 +604,25 @@ where
F: SystemParamFunction<Marker>, F: SystemParamFunction<Marker>,
{ {
func: F, func: F,
pub(crate) param_state: Option<<F::Param as SystemParam>::State>, state: Option<FunctionSystemState<F::Param>>,
pub(crate) system_meta: SystemMeta, system_meta: SystemMeta,
world_id: Option<WorldId>,
archetype_generation: ArchetypeGeneration, archetype_generation: ArchetypeGeneration,
// NOTE: PhantomData<fn()-> T> gives this safe Send/Sync impls // NOTE: PhantomData<fn()-> T> gives this safe Send/Sync impls
marker: PhantomData<fn() -> Marker>, marker: PhantomData<fn() -> 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<P: SystemParam> {
/// 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<Marker, F> FunctionSystem<Marker, F> impl<Marker, F> FunctionSystem<Marker, F>
where where
F: SystemParamFunction<Marker>, F: SystemParamFunction<Marker>,
@ -631,9 +644,8 @@ where
fn clone(&self) -> Self { fn clone(&self) -> Self {
Self { Self {
func: self.func.clone(), func: self.func.clone(),
param_state: None, state: None,
system_meta: SystemMeta::new::<F>(), system_meta: SystemMeta::new::<F>(),
world_id: None,
archetype_generation: ArchetypeGeneration::initial(), archetype_generation: ArchetypeGeneration::initial(),
marker: PhantomData, marker: PhantomData,
} }
@ -653,9 +665,8 @@ where
fn into_system(func: Self) -> Self::System { fn into_system(func: Self) -> Self::System {
FunctionSystem { FunctionSystem {
func, func,
param_state: None, state: None,
system_meta: SystemMeta::new::<F>(), system_meta: SystemMeta::new::<F>(),
world_id: None,
archetype_generation: ArchetypeGeneration::initial(), archetype_generation: ArchetypeGeneration::initial(),
marker: PhantomData, marker: PhantomData,
} }
@ -669,7 +680,8 @@ where
/// Message shown when a system isn't initialized /// Message shown when a system isn't initialized
// When lines get too long, rustfmt can sometimes refuse to format them. // When lines get too long, rustfmt can sometimes refuse to format them.
// Work around this by storing the message separately. // 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<Marker, F> System for FunctionSystem<Marker, F> impl<Marker, F> System for FunctionSystem<Marker, F>
@ -721,19 +733,14 @@ where
let change_tick = world.increment_change_tick(); let change_tick = world.increment_change_tick();
let param_state = &mut self.state.as_mut().expect(Self::ERROR_UNINITIALIZED).param;
// SAFETY: // SAFETY:
// - The caller has invoked `update_archetype_component_access`, which will panic // - The caller has invoked `update_archetype_component_access`, which will panic
// if the world does not match. // if the world does not match.
// - All world accesses used by `F::Param` have been registered, so the caller // - All world accesses used by `F::Param` have been registered, so the caller
// will ensure that there are no data access conflicts. // will ensure that there are no data access conflicts.
let params = unsafe { let params =
F::Param::get_param( unsafe { F::Param::get_param(param_state, &self.system_meta, world, change_tick) };
self.param_state.as_mut().expect(Self::PARAM_MESSAGE),
&self.system_meta,
world,
change_tick,
)
};
let out = self.func.run(input, params); let out = self.func.run(input, params);
self.system_meta.last_run = change_tick; self.system_meta.last_run = change_tick;
out out
@ -741,19 +748,19 @@ where
#[inline] #[inline]
fn apply_deferred(&mut self, world: &mut World) { 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); F::Param::apply(param_state, &self.system_meta, world);
} }
#[inline] #[inline]
fn queue_deferred(&mut self, world: DeferredWorld) { 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); F::Param::queue(param_state, &self.system_meta, world);
} }
#[inline] #[inline]
unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool { 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: // SAFETY:
// - The caller has invoked `update_archetype_component_access`, which will panic // - The caller has invoked `update_archetype_component_access`, which will panic
// if the world does not match. // if the world does not match.
@ -768,29 +775,32 @@ where
#[inline] #[inline]
fn initialize(&mut self, world: &mut World) { fn initialize(&mut self, world: &mut World) {
if let Some(id) = self.world_id { if let Some(state) = &self.state {
assert_eq!( assert_eq!(
id, state.world_id,
world.id(), world.id(),
"System built with a different world than the one it was added to.", "System built with a different world than the one it was added to.",
); );
} else { } else {
self.world_id = Some(world.id()); self.state = Some(FunctionSystemState {
self.param_state = Some(F::Param::init_state(world, &mut self.system_meta)); 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); self.system_meta.last_run = world.change_tick().relative_to(Tick::MAX);
} }
fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) { 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 archetypes = world.archetypes();
let old_generation = let old_generation =
core::mem::replace(&mut self.archetype_generation, archetypes.generation()); core::mem::replace(&mut self.archetype_generation, archetypes.generation());
for archetype in &archetypes[old_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`. // 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) };
} }
} }