Fast and correct one-shot systems with a system registry resource

[alice-i-cecile]

Objective

• Working with exclusive world access is not always easy: in many cases, a standard system or three is more ergonomic to write, and more modularly maintainable.
• For small, one-off tasks (commonly handled with scripting), running an event-reader system incurs a small but flat overhead cost and muddies the schedule.
• Certain forms of logic (e.g. turn-based games) want very fine-grained linear and/or branching control over logic.
• SystemState is not automatically cached, and so performance can suffer and change detection breaks.
• Fixes One-shot systems via Commands for scripting-like logic #2192.
• Partial workaround for API for dynamic management of systems (adding and removing at runtime) #279.

Solution

• Adds a SystemRegistry resource to the World, which stores initialized systems keyed by their TypeId.
• Allows users to call world.run_system(my_system) and commands.run_system(my_system), without re-initializing or losing state (essential for change detection).
• Allow users to run systems based on their TypeId, enabling more complex user-made abstractions
• Add a Callback type to enable convenient use of dynamic one shot systems and reduce the mental overhead of working with Box<dyn SystemLabel>.

Status

• Get sign-off from @cart on this direction, following previous discussions.
• Migrate to a "run all labels" approach: users pass in something that impls Into and all all labels are run in sequential insertion order
• Update this PR to account for changes in [Merged by Bors] - Make System responsible for updating its own archetypes #4115.
• Verify tests pass and are complete
• Add an example
• Simplify example
• Adapt to the changes in the labelling system, ideally waiting on Optimize label comparisons and remove restrictions on types that can be labels #5377.

Future work

• I could not figure out a clean way to implement system chaining, even though it would be useful. Storing heterogenous types is rather nasty.
• Exclusive systems do not work, for the same reason. This should be easily resolved once Add &mut World as a system param and make .exclusive_system() optional #4166 is merged.
• Add benchmarks to see how this compares to ordinary schedules (and enable future optimization)
• Enable ordering constraints and parallel execution of system graphs. Probably better to tackle with schedule v3 ("stageless") #4391 in mind.
• Use this API to store all systems, regardless of whether or not they belong to a schedule, and solve System Lifecycle Rework: all Systems are initialized by definition #2777.
• Enable recursive run_system usage, likely by moving SystemRegistry into its own field on World.

Prior attempts

• run a system from a command #2234
• Run system from &mut World #2417

[MiniaczQ]

Is there a good reason why an unregistered system should be allowed (through implicit registration) to run?

[alice-i-cecile]

Yes: it makes the API dramatically simpler to use.

With implicit registration, the user can call world.run_system(my_system) on an ad-hoc basis anywhere in their code, which is fantastic for prototyping and scripting.

Without it, they have to remember to register it, and then remove that registration when the code changes.

Manual registration is only there to enable TypeId driven workflows. These are much easier to store and work with than an instance of the function, but I would expect manual registration to be hidden in external user-facing code as well :)

[MiniaczQ]

My original argument against this was that users could accidentally run arbitrary systems, which would lead to weird behavior, but the ease of use definitely beats that.
On the second thought, centralizing which functions can be run ad hoc would be helpful for the editor in far future.
Going with that idea, maybe throwing warnings would be a decent middle ground? That way users can still prototype easily, but registration would be enforced.

[alice-i-cecile]

I don't think the warnings are helpful enough to warrant the level of noise. I'm not sure we're protecting against a real class of errors: this isn't any different than just adding systems to a schedule.

[MiniaczQ]

Yeah but all schedule systems are known at app launch,
from what I understand the one-shot systems can be registered at any point in runtime.
So in editors case it won't be aware of this until it is ran at least once, right?

[alice-i-cecile]

Yeah but all schedule systems are known at app launch,

This is only true for now. See #279. I suppose we could opt-in to warnings, but IMO that sort of design is much more easily explored once we have a way to display all of the systems in the schedule.

[cart]

This does seem like a reasonable design. Some assorted thoughts:

• When I pitched my idea for this, it involved "run by label", as that is how we encourage people to refer to systems. Encouraging users to think in "type ids" is a "new" and inconsistent way to reference systems. Given that (as of today) we now auto-label system functions, using actual labels would be better i think (and equally ergonomic).
• I think we should consider embracing the "one label to many systems" design, as that maps best to the label mental model and enables new and interesting call patterns (ex: "run all event handler systems now"). It would require decoupling "registering systems" from "running systems", but imo this would be an improvement. The current implicit "overwrite systems with the same type id" approach feels a bit footgunny. And "pre-defining" systems is already how we do things.

Ultimately I think this pattern could also be embraced by the scheduler (ex: expose a way to queue a run-by-label for the current schedule, using the schedule as the source of systems instead of a SystemRegistry resource ... or merging the two concepts). SystemRegistry is really just a very simple schedule impl. In a "running many labels" scenario, it would be cool to (optionally) do that using "parallel system scheduling".

[alice-i-cecile]

When I pitched my idea for this, it involved "run by label", as that is how we encourage people to refer to systems. Encouraging users to think in "type ids" is a "new" and inconsistent way to reference systems. Given that (as of today) we now auto-label system functions, using actual labels would be better i think (and equally ergonomic).

Agreed :) This was not the case when I made this PR, but I'm happy to swap over.

I think we should consider embracing the "one label to many systems" design, as that maps best to the label mental model and enables new and interesting call patterns (ex: "run all event handler systems now"). It would require decoupling "registering systems" from "running systems", but imo this would be an improvement. The current implicit "overwrite systems with the same type id" approach feels a bit footgunny. And "pre-defining" systems is already how we do things.

Yep, this lines up very nicely with some of the ideas in Stageless about "subgraphs" and "multiple schedules".

Ultimately I think this pattern could also be embraced by the scheduler (ex: expose a way to queue a run-by-label for the current schedule, using the schedule as the source of systems instead of a SystemRegistry resource ... or merging the two concepts). SystemRegistry is really just a very simple schedule impl. In a "running many labels" scenario, it would be cool to (optionally) do that using "parallel system scheduling".

Yep, this was @maniwani's thinking too: eventually I'd like to converge this.

[maniwani]

@maniwani you've been considering related designs; I'd be curious about your opinion on this implementation if you have the time.

Sorry for the late response. I can describe the setup I have in #4391.

@cart
When I pitched my idea for this, it involved "run by label", as that is how we encourage people to refer to systems.

I think we should consider embracing the "one label to many systems" design.

Ultimately, I think this pattern could also be embraced by the scheduler.

[Running a one-shot system] is really just a very simple schedule impl.

I basically came to the same conclusions as cart. This registry data structure is mentioned in the RFC and included in #4391. The "one label to many systems" design is exactly what system sets have become.

I've taken to just calling it Systems (not reflected in my PR yet). All systems and system sets go in the Systems resource, and it can generate a dependency graph and pre-allocate an empty Schedule for each system set. My impl of Systems is designed to let you mutate it while a system or schedule is running.

These are (latest versions of) the important types.

pub struct Systems {
    // used for lookup
    index: HashMap<SystemLabelId, NodeId>,
    next_id: u64,
    // main storage
    systems: HashMap<NodeId, Option<BoxedSystem>>,
    conditions: HashMap<NodeId, Option<Vec<BoxedRunCondition>>>,
    schedules: HashMap<NodeId, Option<Schedule>>,
    graphs: HashMap<NodeId, DependencyGraph>,
    // needed for graph updates
    hierarchy: DiGraph<NodeId>,
    uninit_nodes: Vec<NodeInfo>,
}

pub enum NodeId {
    System(u64),
    Set(u64),
}

struct DependencyGraph {
    /// dependency graph
    base: DiGraph<NodeId>,
    /// flattened version of the dependency graph (only system nodes and system-system edges)
    flat: DiGraph<NodeId>,
    /// cached topological ordering of `flat`
    topsort: Vec<NodeId>,
    /// `Graph` if the graph (and schedule) need to be rebuilt, `Schedule` if just the schedule, `None` otherwise
    rebuild: Rebuild,
}

pub struct Schedule {
    // All elements are sorted in topological order.
    // NOTE: Might change all the `RefCell` to `Cell<Option<T>>`.
    systems: Vec<RefCell<BoxedSystem>>,
    system_conditions: Vec<RefCell<Vec<BoxedRunCondition>>>,
    set_conditions: Vec<RefCell<Vec<BoxedRunCondition>>>,
    system_ids: Vec<NodeId>,
    set_ids: Vec<NodeId>,
    system_deps: Vec<(usize, Vec<usize>)>,
    sets_of_systems: Vec<FixedBitSet>,
    systems_of_sets: Vec<FixedBitSet>,
}

Summarizing:

• Systems is a resource on the World, and I've made it so you have to take systems and schedules out of Systems (using their corresponding label) in order to run them. Using resource_scope would prevent accessing Systems again.
• Adding and removing things does not immediately rebuild any schedules. The changes are only propagated up to flag the system sets whose dependency graphs need to be rebuilt (top-sorted again).
• The function to check out a Schedule will apply a pending rebuild if the schedule has one, then take the now up-to-date Schedule out of its Option, move all the required systems and conditions from theirs into it, and return it.
• All the graph stuff stays with Systems, so you can still "remove" systems by label even if they're extracted. You have to return extracted schedules to Systems to update them, so if you've removed systems, those will just be dropped at that time.

So basically, you only incur the costs of schedules you use, there's no ownership dilemma, and any &mut World context like exclusive systems or commands can extract something from Systems and run it.

(Edit: Also, just want to highlight how World-like Systems is. Maybe in the far future, NodeId could be Entity and the mapped values could just be components.)

[alice-i-cecile]

FYI if anyone wants to pick this up while I'm on vacation this month feel free! Otherwise I'll wrap this up in September.

[cart]

I think we should close this out now that stageless / #6587 is about to land. Running systems on demand will be possible via Schedules:

app
  .add_system_to(OnDemand, foo)
  .add_system(bar);

fn bar(world: &mut World) {
  world.run_schedule(OnDemand);
} 

[cart]

(FYI I'm not trying to shutting the conversation down. Happy to open this back up if anyone wants. Stageless doesn't currently include "run systems from commands", but I think that impl would be different (and much smaller) in the new world).

[cart]

Also one of the concerns in this PR was that systemstate isn't cacheable, but it is now!

fn bar(world: &mut World, state: &mut SystemState<Res<Time>>) {
  let delta = state.get(world).delta_seconds();
  if delta > 2.0 {
      world.run_schedule(OnDemand);
  }
} 

[alice-i-cecile]

Yep! I agree with this decision!