Stop storing access for all systems#19477
Conversation
…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
left a comment
re0312
left a comment
There was a problem hiding this comment.
Absolutely stellar implementation! The code is remarkably clean and well-structured.
|
|
||
| // 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
…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
left a comment
urben1680
left a comment
There was a problem hiding this comment.
Well done, I like this! Getting rid of both accesses in System will also make things easier for my manual impls. 😄
# 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>
5 participants
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.