Stop storing access for all systems #19477
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…e parameter to init_access.
Create wrapper types in the scheduling module to store the access where necessary.
alice-i-cecile
added
A-ECS
…cess-ii # Conflicts: # crates/bevy_ecs/src/system/builder.rs
re0312
approved these changes
Jun 8, 2025
Trashtalk217
reviewed
Jun 8, 2025
| type Target = str; | ||
| fn deref(&self) -> &Self::Target { | ||
| self.name() | ||
| } | ||
| } | ||
|
|
||
| // SAFETY: no component value access | ||
| unsafe impl SystemParam for SystemName<'_> { | ||
| type State = Cow<'static, str>; |
There was a problem hiding this comment.
Why did you change this? I'm curious.
There was a problem hiding this comment.
Why did you change this? I'm curious.
Oh, right, I meant to leave a note about that!
I removed the SystemMeta parameter from init_state since I thought it would only be used by the parts that had moved to init_access. This turned out to be the one place that actually initialized the state from the SystemMeta! Note that for most systems, name is a Cow::Borrowed, so this won't be any slower.
I can restore the SystemMeta parameter if anyone is opposed to this change. We might want to do something else to ensure that existing calls to init_state fail to compile, though, since users will need to change those to add calls to init_access.
I think the ideal solution here would be to let get_param borrow from SystemMeta. That would also avoid needing to clone() it in ParamSet. But that would touch a lot of code for a fairly minor performance improvement, especially since this PR already saves having to clone the access.
There was a problem hiding this comment.
No, this seems good. Thanks for the explanation.
…cess-ii # Conflicts: # crates/bevy_ecs/src/schedule/executor/mod.rs # crates/bevy_ecs/src/system/adapter_system.rs # crates/bevy_ecs/src/system/combinator.rs # crates/bevy_ecs/src/system/exclusive_function_system.rs # crates/bevy_ecs/src/system/function_system.rs # crates/bevy_ecs/src/system/observer_system.rs # crates/bevy_ecs/src/system/schedule_system.rs # crates/bevy_ecs/src/system/system.rs
Trashtalk217
approved these changes
Jun 9, 2025
…cess-ii # Conflicts: # crates/bevy_ecs/src/schedule/executor/mod.rs # crates/bevy_ecs/src/schedule/schedule.rs # crates/bevy_ecs/src/system/adapter_system.rs # crates/bevy_ecs/src/system/combinator.rs # crates/bevy_ecs/src/system/exclusive_function_system.rs # crates/bevy_ecs/src/system/function_system.rs # crates/bevy_ecs/src/system/mod.rs # crates/bevy_ecs/src/system/observer_system.rs # crates/bevy_ecs/src/system/schedule_system.rs # crates/bevy_ecs/src/system/system.rs # crates/bevy_ecs/src/world/unsafe_world_cell.rs
Update PR number in migration guide.
…cess-ii # Conflicts: # crates/bevy_ecs/src/removal_detection.rs
Trashtalk217
added
S-Ready-For-Final-Review
urben1680
approved these changes
Jun 13, 2025
hukasu
pushed a commit
to hukasu/bevy
that referenced
this pull request
Jun 15, 2025
# 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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Reduce memory usage by storing fewer copies of
FilteredAccessSet<ComponentId>.Currently, the
Systemtrait exposes thecomponent_access_setfor 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 aPipeSystemorCombinatorSystem.Solution
Instead of exposing the access from a method on
System, return it fromSystem::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
SystemParamBuilders, though. Those calculate the access inSystemParamBuilder::build, which happens earlier thanSystem::initialize. To handle those, we separateSystemParam::init_stateintoinit_state, which creates the state value, andinit_access, which calculates the access. This letsSystem::initializecallinit_accesson 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::buildandSystemParam::init_state.