Skip to content

Conversation

@ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Nov 12, 2025

Objective

Solution

  • Upgraded and exposed Dag<N> as a fully-fledged directed acyclic graph type.
    • The validity of the cached topological order is now tracked by a dirty flag. All modifications to the graph (via DerefMut or Dag::graph_mut) cause the DAG to be marked as dirty.
    • Added Dag::toposort: if the DAG is dirty, computes topological order, caches it, and marks the DAG as clean. If already clean, returns the cached topological order. We now also reuse the previous toposort Vec allocation.
    • Added Dag::get_toposort: can be used to access the topological order with &self, with the stipulation that it returns an Option, and None if the DAG is dirty.
    • Replaced check_graph with Dag::analyze, and made it publicly accessible.
    • Added Dag::remove_redundant_edges, which uses the output of Dag::analyze.
  • Renamed CheckGraphResults to DagAnalysis.
    • Added DagAnalysis::check_for_redundant_edges, replacing ScheduleGraph::optionally_check_hierarchy_conflicts.
    • Added DagAnalysis::check_for_cross_dependencies, replacing ScheduleGraph::check_for_cross_dependencies. It now takes two full DagAnalysis for comparison.
    • Added DagAnalysis::check_for_cross_intersection, replacing ScheduleGraph::check_order_but_intersect.
  • Added DagGroups to encapsulate the HashMap<SystemSetKey, Vec<SystemKey>> with additional capabilities:
    • DagGroups::flatten and DagGroups::flatten_undirected handle the graph reduction operations previously performed by functions on ScheduleGraph.
  • Added ConflictingSystems to encapsulate Vec<(SystemKey, SystemKey, ComponentId)> with additional capabilities and type safety.

See the included migration guide for breaking changes.

Testing

  • Ran examples
  • Added new tests for Dag, DagAnalysis, and DagGroups functionality.

Future work

  • Consider replacing HashSet<SystemKey> with a FixedBitSet-like type for better performance.

- Moved some functions to `CheckGraphResults` which don't make use of `ScheduleGraph` internals.
- Encapsulated conflicting systems into its own struct and moved related functions off of `ScheduleGraph`.
@ItsDoot ItsDoot added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 12, 2025
@ItsDoot ItsDoot mentioned this pull request Nov 12, 2025
7 tasks
@hymm hymm self-requested a review November 12, 2025 23:16
@ItsDoot ItsDoot added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 13, 2025
@alice-i-cecile alice-i-cecile self-requested a review November 14, 2025 05:40
@alice-i-cecile alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 16, 2025
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments on the code are all minor. I'd like to see a run of the schedule_build benchmarks as a sanity check before approving.

I'm not sure how many other changes you have planned, but we should maybe make a pr to update bevy_mod_debug_dump to main when you're done to make sure we haven't broken anything serious for that crate.

@ItsDoot
Copy link
Contributor Author

ItsDoot commented Nov 18, 2025

After that latest commit (which was making the _no_constraints benches perform badly), here's the benchmarks:

group                                          pr                                     main
-----                                          ------                                 ------
build_schedule/1000_schedule                   1.00   992.2±17.74ms        ? ?/sec    1.04  1028.5±18.70ms        ? ?/sec
build_schedule/1000_schedule_no_constraints    1.05     19.8±0.15ms        ? ?/sec    1.00     19.0±0.12ms        ? ?/sec
build_schedule/100_schedule                    1.00      6.1±0.03ms        ? ?/sec    1.01      6.2±0.04ms        ? ?/sec
build_schedule/100_schedule_no_constraints     1.02   380.9±11.24µs        ? ?/sec    1.00   372.3±14.00µs        ? ?/sec
build_schedule/500_schedule                    1.00    196.8±4.25ms        ? ?/sec    1.03    201.9±4.39ms        ? ?/sec
build_schedule/500_schedule_no_constraints     1.04      5.4±0.15ms        ? ?/sec    1.00      5.2±0.15ms        ? ?/sec
run_empty_schedule/MultiThreaded               1.05      7.0±0.33ns        ? ?/sec    1.00      6.6±0.38ns        ? ?/sec
run_empty_schedule/SingleThreaded              1.41     13.3±0.78ns        ? ?/sec    1.00      9.4±0.74ns        ? ?/sec
schedule/base                                  1.00     20.4±1.73µs        ? ?/sec    1.03     21.0±3.68µs        ? ?/sec

EDIT: Yea, that SingleThreaded bench is just noise; here's another run:

group                                          pr                                     main
-----                                          ------                                 ------
build_schedule/1000_schedule                   1.00   991.8±15.46ms        ? ?/sec    1.04  1028.5±18.70ms        ? ?/sec
build_schedule/1000_schedule_no_constraints    1.05     19.9±0.12ms        ? ?/sec    1.00     19.0±0.12ms        ? ?/sec
build_schedule/100_schedule                    1.00      6.1±0.10ms        ? ?/sec    1.01      6.2±0.04ms        ? ?/sec
build_schedule/100_schedule_no_constraints     1.02   380.2±10.87µs        ? ?/sec    1.00   372.3±14.00µs        ? ?/sec
build_schedule/500_schedule                    1.00    200.3±3.90ms        ? ?/sec    1.01    201.9±4.39ms        ? ?/sec
build_schedule/500_schedule_no_constraints     1.04      5.4±0.17ms        ? ?/sec    1.00      5.2±0.15ms        ? ?/sec
run_empty_schedule/MultiThreaded               1.05      6.9±0.34ns        ? ?/sec    1.00      6.6±0.38ns        ? ?/sec
run_empty_schedule/SingleThreaded              1.00      9.4±0.62ns        ? ?/sec    1.01      9.4±0.74ns        ? ?/sec
schedule/base                                  1.00     20.6±2.32µs        ? ?/sec    1.02     21.0±3.68µs        ? ?/sec

Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiousity how much slower was the HashSet?

@hymm hymm added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Nov 18, 2025
@ItsDoot
Copy link
Contributor Author

ItsDoot commented Nov 18, 2025

Out of curiousity how much slower was the HashSet?

Just a tad /s

group                                          pr                                      main
-----                                          -----                                  ------
build_schedule/1000_schedule                   1.06  1094.7±15.67ms        ? ?/sec    1.00  1028.5±18.70ms        ? ?/sec
build_schedule/1000_schedule_no_constraints    2.67     50.7±0.37ms        ? ?/sec    1.00     19.0±0.12ms        ? ?/sec
build_schedule/100_schedule                    1.07      6.6±0.09ms        ? ?/sec    1.00      6.2±0.04ms        ? ?/sec
build_schedule/100_schedule_no_constraints     1.51   562.2±25.96µs        ? ?/sec    1.00   372.3±14.00µs        ? ?/sec
build_schedule/500_schedule                    1.06    214.4±2.44ms        ? ?/sec    1.00    201.9±4.39ms        ? ?/sec
build_schedule/500_schedule_no_constraints     2.27     11.9±0.04ms        ? ?/sec    1.00      5.2±0.15ms        ? ?/sec
run_empty_schedule/MultiThreaded               1.07      7.1±0.32ns        ? ?/sec    1.00      6.6±0.38ns        ? ?/sec
run_empty_schedule/SingleThreaded              1.08     10.2±0.66ns        ? ?/sec    1.00      9.4±0.74ns        ? ?/sec
schedule/base                                  1.00     20.9±3.19µs        ? ?/sec    1.01     21.0±3.68µs        ? ?/sec

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 20, 2025
Merged via the queue into bevyengine:main with commit 4809a3c Nov 20, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants