Stop storing access for all systems (#19477)

# Objective

Reduce memory usage by storing fewer copies of
`FilteredAccessSet<ComponentId>`.

Currently, the `System` trait exposes the `component_access_set` for the
system, which is used by the multi-threaded executor to determine which
systems can run concurrently. But because it is available on the trait,
it needs to be stored for *every* system, even ones that are not run by
the executor! In particular, it is never needed for observers, or for
the inner systems in a `PipeSystem` or `CombinatorSystem`.


## Solution

Instead of exposing the access from a method on `System`, return it from
`System::initialize`. Since it is still needed during scheduling, store
the access alongside the boxed system in the schedule.

That's not quite enough for systems built using `SystemParamBuilder`s,
though. Those calculate the access in `SystemParamBuilder::build`, which
happens earlier than `System::initialize`. To handle those, we separate
`SystemParam::init_state` into `init_state`, which creates the state
value, and `init_access`, which calculates the access. This lets
`System::initialize` call `init_access` on a state that was provided by
the builder.

An additional benefit of that separation is that it removes the need to
duplicate access checks between `SystemParamBuilder::build` and
`SystemParam::init_state`.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Author: chescock

crates/bevy_ecs/macros/src/lib.rs

@@ -440,10 +440,10 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
             > #path::system::SystemParamBuilder<#generic_struct> for #builder_name<#(#builder_type_parameters,)*>
                 #where_clause
             {
-                fn build(self, world: &mut #path::world::World, meta: &mut #path::system::SystemMeta) -> <#generic_struct as #path::system::SystemParam>::State {
+                fn build(self, world: &mut #path::world::World) -> <#generic_struct as #path::system::SystemParam>::State {
                     let #builder_name { #(#fields: #field_locals,)* } = self;
                     #state_struct_name {
-                        state: #path::system::SystemParamBuilder::build((#(#tuple_patterns,)*), world, meta)
+                        state: #path::system::SystemParamBuilder::build((#(#tuple_patterns,)*), world)
                     }
                 }
             }
@@ -472,12 +472,16 @@ pub fn derive_system_param(input: TokenStream) -> TokenStream {
                 type State = #state_struct_name<#punctuated_generic_idents>;
                 type Item<'w, 's> = #struct_name #ty_generics;
 
-                fn init_state(world: &mut #path::world::World, system_meta: &mut #path::system::SystemMeta) -> Self::State {
+                fn init_state(world: &mut #path::world::World) -> Self::State {
                     #state_struct_name {
-                        state: <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::init_state(world, system_meta),
+                        state: <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::init_state(world),
                     }
                 }
 
+                fn init_access(state: &Self::State, system_meta: &mut #path::system::SystemMeta, component_access_set: &mut #path::query::FilteredAccessSet<#path::component::ComponentId>, world: &mut #path::world::World) {
+                    <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::init_access(&state.state, system_meta, component_access_set, world);
+                }
+
                 fn apply(state: &mut Self::State, system_meta: &#path::system::SystemMeta, world: &mut #path::world::World) {
                     <#fields_alias::<'_, '_, #punctuated_generic_idents> as #path::system::SystemParam>::apply(&mut state.state, system_meta, world);
                 }

crates/bevy_ecs/src/lifecycle.rs

@@ -54,6 +54,7 @@ use crate::{
     component::{Component, ComponentId, ComponentIdFor, Tick},
     entity::Entity,
     event::{Event, EventCursor, EventId, EventIterator, EventIteratorWithId, Events},
+    query::FilteredAccessSet,
     relationship::RelationshipHookMode,
     storage::SparseSet,
     system::{Local, ReadOnlySystemParam, SystemMeta, SystemParam},
@@ -617,7 +618,15 @@ unsafe impl<'a> SystemParam for &'a RemovedComponentEvents {
     type State = ();
     type Item<'w, 's> = &'w RemovedComponentEvents;
 
-    fn init_state(_world: &mut World, _system_meta: &mut SystemMeta) -> Self::State {}
+    fn init_state(_world: &mut World) -> Self::State {}
+
+    fn init_access(
+        _state: &Self::State,
+        _system_meta: &mut SystemMeta,
+        _component_access_set: &mut FilteredAccessSet<ComponentId>,
+        _world: &mut World,
+    ) {
+    }
 
     #[inline]
     unsafe fn get_param<'w, 's>(

crates/bevy_ecs/src/schedule/auto_insert_apply_deferred.rs

@@ -102,7 +102,7 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
         let mut system_has_conditions_cache = HashMap::<usize, bool>::default();
         let mut is_valid_explicit_sync_point = |system: NodeId| {
             let index = system.index();
-            is_apply_deferred(graph.systems[index].get().unwrap())
+            is_apply_deferred(&graph.systems[index].get().unwrap().system)
                 && !*system_has_conditions_cache
                     .entry(index)
                     .or_insert_with(|| system_has_conditions(graph, system))
@@ -138,7 +138,11 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
             } else if !node_needs_sync {
                 // No previous node has postponed sync points to add so check if the system itself
                 // has deferred params that require a sync point to apply them.
-                node_needs_sync = graph.systems[node.index()].get().unwrap().has_deferred();
+                node_needs_sync = graph.systems[node.index()]
+                    .get()
+                    .unwrap()
+                    .system
+                    .has_deferred();
             }
 
             for target in dependency_flattened.neighbors_directed(*node, Direction::Outgoing) {
@@ -148,7 +152,11 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
 
                 let mut edge_needs_sync = node_needs_sync;
                 if node_needs_sync
-                    && !graph.systems[target.index()].get().unwrap().is_exclusive()
+                    && !graph.systems[target.index()]
+                        .get()
+                        .unwrap()
+                        .system
+                        .is_exclusive()
                     && self.no_sync_edges.contains(&(*node, target))
                 {
                     // The node has deferred params to apply, but this edge is ignoring sync points.
@@ -190,7 +198,7 @@ impl ScheduleBuildPass for AutoInsertApplyDeferredPass {
                     continue;
                 }
 
-                if is_apply_deferred(graph.systems[target.index()].get().unwrap()) {
+                if is_apply_deferred(&graph.systems[target.index()].get().unwrap().system) {
                     // We don't need to insert a sync point since ApplyDeferred is a sync point
                     // already!
                     continue;

crates/bevy_ecs/src/schedule/executor/mod.rs

@@ -19,7 +19,7 @@ use crate::{
     error::{BevyError, ErrorContext, Result},
     prelude::{IntoSystemSet, SystemSet},
     query::FilteredAccessSet,
-    schedule::{BoxedCondition, InternedSystemSet, NodeId, SystemTypeSet},
+    schedule::{ConditionWithAccess, InternedSystemSet, NodeId, SystemTypeSet, SystemWithAccess},
     system::{ScheduleSystem, System, SystemIn, SystemParamValidationError, SystemStateFlags},
     world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World},
 };
@@ -74,9 +74,9 @@ pub struct SystemSchedule {
     /// List of system node ids.
     pub(super) system_ids: Vec<NodeId>,
     /// Indexed by system node id.
-    pub(super) systems: Vec<ScheduleSystem>,
+    pub(super) systems: Vec<SystemWithAccess>,
     /// Indexed by system node id.
-    pub(super) system_conditions: Vec<Vec<BoxedCondition>>,
+    pub(super) system_conditions: Vec<Vec<ConditionWithAccess>>,
     /// Indexed by system node id.
     /// Number of systems that the system immediately depends on.
     #[cfg_attr(
@@ -97,7 +97,7 @@ pub struct SystemSchedule {
     /// List of system set node ids.
     pub(super) set_ids: Vec<NodeId>,
     /// Indexed by system set node id.
-    pub(super) set_conditions: Vec<Vec<BoxedCondition>>,
+    pub(super) set_conditions: Vec<Vec<ConditionWithAccess>>,
     /// Indexed by system set node id.
     /// List of systems that are in sets that have conditions.
     ///
@@ -162,11 +162,6 @@ impl System for ApplyDeferred {
         Cow::Borrowed("bevy_ecs::apply_deferred")
     }
 
-    fn component_access_set(&self) -> &FilteredAccessSet<ComponentId> {
-        // This system accesses no components.
-        const { &FilteredAccessSet::new() }
-    }
-
     fn flags(&self) -> SystemStateFlags {
         // non-send , exclusive , no deferred
         SystemStateFlags::NON_SEND | SystemStateFlags::EXCLUSIVE
@@ -205,7 +200,9 @@ impl System for ApplyDeferred {
         Ok(())
     }
 
-    fn initialize(&mut self, _world: &mut World) {}
+    fn initialize(&mut self, _world: &mut World) -> FilteredAccessSet<ComponentId> {
+        FilteredAccessSet::new()
+    }
 
     fn check_change_tick(&mut self, _change_tick: Tick) {}
 

crates/bevy_ecs/src/schedule/executor/multi_threaded.rs

@@ -15,7 +15,10 @@ use tracing::{info_span, Span};
 use crate::{
     error::{ErrorContext, ErrorHandler, Result},
     prelude::Resource,
-    schedule::{is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule},
+    schedule::{
+        is_apply_deferred, ConditionWithAccess, ExecutorKind, SystemExecutor, SystemSchedule,
+        SystemWithAccess,
+    },
     system::ScheduleSystem,
     world::{unsafe_world_cell::UnsafeWorldCell, World},
 };
@@ -27,14 +30,14 @@ use super::__rust_begin_short_backtrace;
 /// Borrowed data used by the [`MultiThreadedExecutor`].
 struct Environment<'env, 'sys> {
     executor: &'env MultiThreadedExecutor,
-    systems: &'sys [SyncUnsafeCell<ScheduleSystem>],
+    systems: &'sys [SyncUnsafeCell<SystemWithAccess>],
     conditions: SyncUnsafeCell<Conditions<'sys>>,
     world_cell: UnsafeWorldCell<'env>,
 }
 
 struct Conditions<'a> {
-    system_conditions: &'a mut [Vec<BoxedCondition>],
-    set_conditions: &'a mut [Vec<BoxedCondition>],
+    system_conditions: &'a mut [Vec<ConditionWithAccess>],
+    set_conditions: &'a mut [Vec<ConditionWithAccess>],
     sets_with_conditions_of_systems: &'a [FixedBitSet],
     systems_in_sets_with_conditions: &'a [FixedBitSet],
 }
@@ -172,8 +175,8 @@ impl SystemExecutor for MultiThreadedExecutor {
                 conflicting_systems: FixedBitSet::with_capacity(sys_count),
                 condition_conflicting_systems: FixedBitSet::with_capacity(sys_count),
                 dependents: schedule.system_dependents[index].clone(),
-                is_send: schedule.systems[index].is_send(),
-                is_exclusive: schedule.systems[index].is_exclusive(),
+                is_send: schedule.systems[index].system.is_send(),
+                is_exclusive: schedule.systems[index].system.is_exclusive(),
             });
             if schedule.system_dependencies[index] == 0 {
                 self.starting_systems.insert(index);
@@ -187,10 +190,7 @@ impl SystemExecutor for MultiThreadedExecutor {
                 let system1 = &schedule.systems[index1];
                 for index2 in 0..index1 {
                     let system2 = &schedule.systems[index2];
-                    if !system2
-                        .component_access_set()
-                        .is_compatible(system1.component_access_set())
-                    {
+                    if !system2.access.is_compatible(&system1.access) {
                         state.system_task_metadata[index1]
                             .conflicting_systems
                             .insert(index2);
@@ -202,11 +202,10 @@ impl SystemExecutor for MultiThreadedExecutor {
 
                 for index2 in 0..sys_count {
                     let system2 = &schedule.systems[index2];
-                    if schedule.system_conditions[index1].iter().any(|condition| {
-                        !system2
-                            .component_access_set()
-                            .is_compatible(condition.component_access_set())
-                    }) {
+                    if schedule.system_conditions[index1]
+                        .iter()
+                        .any(|condition| !system2.access.is_compatible(&condition.access))
+                    {
                         state.system_task_metadata[index1]
                             .condition_conflicting_systems
                             .insert(index2);
@@ -220,11 +219,10 @@ impl SystemExecutor for MultiThreadedExecutor {
                 let mut conflicting_systems = FixedBitSet::with_capacity(sys_count);
                 for sys_index in 0..sys_count {
                     let system = &schedule.systems[sys_index];
-                    if schedule.set_conditions[set_idx].iter().any(|condition| {
-                        !system
-                            .component_access_set()
-                            .is_compatible(condition.component_access_set())
-                    }) {
+                    if schedule.set_conditions[set_idx]
+                        .iter()
+                        .any(|condition| !system.access.is_compatible(&condition.access))
+                    {
                         conflicting_systems.insert(sys_index);
                     }
                 }
@@ -468,7 +466,8 @@ impl ExecutorState {
                 debug_assert!(!self.running_systems.contains(system_index));
                 // SAFETY: Caller assured that these systems are not running.
                 // Therefore, no other reference to this system exists and there is no aliasing.
-                let system = unsafe { &mut *context.environment.systems[system_index].get() };
+                let system =
+                    &mut unsafe { &mut *context.environment.systems[system_index].get() }.system;
 
                 #[cfg(feature = "hotpatching")]
                 if should_update_hotpatch {
@@ -661,7 +660,7 @@ impl ExecutorState {
     ///   used by the specified system.
     unsafe fn spawn_system_task(&mut self, context: &Context, system_index: usize) {
         // SAFETY: this system is not running, no other reference exists
-        let system = unsafe { &mut *context.environment.systems[system_index].get() };
+        let system = &mut unsafe { &mut *context.environment.systems[system_index].get() }.system;
         // Move the full context object into the new future.
         let context = *context;
 
@@ -703,7 +702,7 @@ impl ExecutorState {
     /// Caller must ensure no systems are currently borrowed.
     unsafe fn spawn_exclusive_system_task(&mut self, context: &Context, system_index: usize) {
         // SAFETY: this system is not running, no other reference exists
-        let system = unsafe { &mut *context.environment.systems[system_index].get() };
+        let system = &mut unsafe { &mut *context.environment.systems[system_index].get() }.system;
         // Move the full context object into the new future.
         let context = *context;
 
@@ -785,12 +784,12 @@ impl ExecutorState {
 
 fn apply_deferred(
     unapplied_systems: &FixedBitSet,
-    systems: &[SyncUnsafeCell<ScheduleSystem>],
+    systems: &[SyncUnsafeCell<SystemWithAccess>],
     world: &mut World,
 ) -> Result<(), Box<dyn Any + Send>> {
     for system_index in unapplied_systems.ones() {
         // SAFETY: none of these systems are running, no other references exist
-        let system = unsafe { &mut *systems[system_index].get() };
+        let system = &mut unsafe { &mut *systems[system_index].get() }.system;
         let res = std::panic::catch_unwind(AssertUnwindSafe(|| {
             system.apply_deferred(world);
         }));
@@ -813,7 +812,7 @@ fn apply_deferred(
 /// - `world` must have permission to read any world data
 ///   required by `conditions`.
 unsafe fn evaluate_and_fold_conditions(
-    conditions: &mut [BoxedCondition],
+    conditions: &mut [ConditionWithAccess],
     world: UnsafeWorldCell,
     error_handler: ErrorHandler,
 ) -> bool {
@@ -823,7 +822,7 @@ unsafe fn evaluate_and_fold_conditions(
     )]
     conditions
         .iter_mut()
-        .map(|condition| {
+        .map(|ConditionWithAccess { condition, .. }| {
             // SAFETY:
             // - The caller ensures that `world` has permission to read any data
             //   required by the condition.

crates/bevy_ecs/src/schedule/executor/simple.rs

@@ -12,7 +12,8 @@ use std::eprintln;
 use crate::{
     error::{ErrorContext, ErrorHandler},
     schedule::{
-        executor::is_apply_deferred, BoxedCondition, ExecutorKind, SystemExecutor, SystemSchedule,
+        executor::is_apply_deferred, ConditionWithAccess, ExecutorKind, SystemExecutor,
+        SystemSchedule,
     },
     world::World,
 };
@@ -70,7 +71,7 @@ impl SystemExecutor for SimpleExecutor {
 
         for system_index in 0..schedule.systems.len() {
             #[cfg(feature = "trace")]
-            let name = schedule.systems[system_index].name();
+            let name = schedule.systems[system_index].system.name();
             #[cfg(feature = "trace")]
             let should_run_span = info_span!("check_conditions", name = &*name).entered();
 
@@ -105,7 +106,7 @@ impl SystemExecutor for SimpleExecutor {
 
             should_run &= system_conditions_met;
 
-            let system = &mut schedule.systems[system_index];
+            let system = &mut schedule.systems[system_index].system;
             if should_run {
                 let valid_params = match system.validate_param(world) {
                     Ok(()) => true,
@@ -195,7 +196,7 @@ impl SimpleExecutor {
     note = "Use SingleThreadedExecutor instead. See https://github.com/bevyengine/bevy/issues/18453 for motivation."
 )]
 fn evaluate_and_fold_conditions(
-    conditions: &mut [BoxedCondition],
+    conditions: &mut [ConditionWithAccess],
     world: &mut World,
     error_handler: ErrorHandler,
 ) -> bool {
@@ -211,7 +212,7 @@ fn evaluate_and_fold_conditions(
     )]
     conditions
         .iter_mut()
-        .map(|condition| {
+        .map(|ConditionWithAccess { condition, .. }| {
             match condition.validate_param(world) {
                 Ok(()) => (),
                 Err(e) => {