Use explicitly added ApplyDeferred stages when computing automatically inserted sync points.

[andriyDev]

Objective

• The previous implementation of automatically inserting sync points did not consider explicitly added sync points. This created additional sync points. For example:

A-B
C-D-E

If A and B needed a sync point, and D was an ApplyDeferred, an additional sync point would be generated between A and B.

A-D2-B
C-D -E

This can result in the following system ordering:

A-D2-(B-C)-D-E

Where only B and C run in parallel. If we could reuse D as the sync point, we would get the following ordering:

(A-C)-D-(B-E)

Now we have two more opportunities for parallelism!

Solution

• In the first pass, we:
  • Compute the number of sync points before each node
    • This was already happening but now we consider ApplyDeferred nodes as creating a sync point.
  • Pick an arbitrary explicit ApplyDeferred node for each "sync point index" that we can (some required sync points may be missing!)
• In the second pass, we:
  • For each edge, if two nodes have a different number of sync points before them then there must be a sync point between them.
  • Look for an explicit ApplyDeferred. If one exists, use it as the sync point.
  • Otherwise, generate a new sync point.

I believe this should also gracefully handle changes to the ScheduleGraph. Since automatically inserted sync points are inserted as systems, they won't look any different to explicit sync points, so they are also candidates for "reusing" sync points.

One thing this solution does not handle is "deduping" sync points. If you add 10 sync points explicitly, there will be at least 10 sync points. You could keep track of all the sync points at the same "distance" and then hack apart the graph to dedup those, but that could be a follow-up step (and it's more complicated since you have to worry about transferring edges between nodes).

Testing

• Added a test to test the feature.
• The existing tests from all our crates still pass.

Showcase

• Automatically inserted sync points can now reuse explicitly inserted ApplyDeferred systems! Previously, Bevy would add new sync points between systems, ignoring the explicitly added sync points. This would reduce parallelism of systems in some situations. Now, the parallelism has been improved!

[andriyDev]

One thing I'm not fully certain of is whether this unlocks more parallelism. The naive answer is fewer sync points = more parallelism. However this depends on how the multi-threaded executor works. AFAIK, the systems are topologically sorted, and then when a system finishes, an executor thread wakes up, iterates through the list of unfinished systems, finds the first one whose dependencies are satisfied (before systems), and runs that.

I'm not certain how system accesses relate to this. Perhaps a systems dependencies just be satisfied AND its accesses must be satisfied, otherwise it is skipped. This would imply that ApplyDeferred systems would always be "pushed back" as far as they can be since it obviously requires full world access. So even if we have two sync points, the prior systems would end up "pushing them" next to each other in many cases. This could lead to "starvation" though, so maybe that's not how it works? I need to dig in to how the executor works!

[alice-i-cecile]

Good: this was part of the future work that we put off when merging the initial PR.

[hymm]

ran the build_schedule benches and seems about the same. The extra pass doesn't seem too expensive. I guess it was probably the extra topo sort that was expensive in the original pr.

group                                         apply_deferred                         main_for_apply_deferred
-----                                         --------------                         -----------------------
build_schedule/1000_schedule                  1.00       2.1±0.15s        ? ?/sec    1.00       2.1±0.15s        ? ?/sec
build_schedule/1000_schedule_noconstraints    1.00     27.1±2.29ms        ? ?/sec    1.00     27.2±2.33ms        ? ?/sec
build_schedule/100_schedule                   1.00      8.6±1.01ms        ? ?/sec    1.00      8.6±0.83ms        ? ?/sec
build_schedule/100_schedule_noconstraints     1.00   437.9±40.73µs        ? ?/sec    1.08   473.8±41.74µs        ? ?/sec
build_schedule/500_schedule                   1.00   387.5±35.70ms        ? ?/sec    1.01   390.5±37.29ms        ? ?/sec
build_schedule/500_schedule_noconstraints     1.00      7.1±0.65ms        ? ?/sec    1.00      7.1±0.61ms        ? ?/sec

[andriyDev]

@hymm How are you generating that benchmark? I tried running the benchmark locally, once with main and once with this PR and got the following:

~/bevy/benches (deferred) $ cargo bench build_schedule
   Compiling benches v0.0.0 (E:\Content\bevy\benches)
    Finished `bench` profile [optimized] target(s) in 4m 15s
     Running benches/bevy_ecs/main.rs (target\release\deps\ecs-d2da3142124a85f3.exe)
Gnuplot not found or not usable, using plotters backend
`gnuplot --version` returned an unparseable version string: gnuplot 6.0 patchlevel 1

build_schedule/100_schedule_noconstraints
                        time:   [726.73 µs 729.60 µs 732.60 µs]
                        change: [+1.5341% +2.0926% +2.6640%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 2 outliers among 100 measurements (2.00%)
  2 (2.00%) high mild
build_schedule/100_schedule
                        time:   [10.889 ms 11.275 ms 11.697 ms]
                        change: [+11.464% +15.498% +20.594%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 6 outliers among 100 measurements (6.00%)
  5 (5.00%) high mild
  1 (1.00%) high severe
build_schedule/500_schedule_noconstraints
                        time:   [12.233 ms 12.428 ms 12.626 ms]
                        change: [+22.118% +24.134% +26.229%] (p = 0.00 < 0.05)
                        Performance has regressed.
Benchmarking build_schedule/500_schedule: Warming up for 500.00 ms
Warning: Unable to complete 100 samples in 15.0s. You may wish to increase target time to 53.4s, or reduce sample count to 20.
build_schedule/500_schedule
                        time:   [468.93 ms 477.26 ms 485.64 ms]
                        change: [+19.192% +21.547% +24.130%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild
build_schedule/1000_schedule_noconstraints
                        time:   [45.864 ms 46.657 ms 47.519 ms]
                        change: [+25.115% +27.296% +29.666%] (p = 0.00 < 0.05)
                        Performance has regressed.
Found 5 outliers among 100 measurements (5.00%)
  3 (3.00%) high mild
  2 (2.00%) high severe
Benchmarking build_schedule/1000_schedule: Warming up for 500.00 ms
Warning: Unable to complete 100 samples in 15.0s. You may wish to increase target time to 277.9s, or reduce sample count to 10.
build_schedule/1000_schedule
                        time:   [2.4158 s 2.4411 s 2.4681 s]
                        change: [+6.4100% +7.6148% +8.8132%] (p = 0.00 < 0.05)
                        Performance has regressed.

Our delta seems to be about +7%. I'm on Windows, but I'm surprised if there's such a substantial difference between OSes (that's my guess).

Edit: To be clear, I'm doing:

$ git switch main
$ cargo bench build_schedule
$ git switch deferred
$ cargo bench build_schedule

[hymm]

pretty much what you're doing except using critcmp to make a nice table. https://github.com/BurntSushi/critcmp

I do run a few times to make sure I'm getting consistent result and making sure the diffs aren't in the noise of my cpu perf. For these result I ran on my laptop I changed to silent mode, since the normal mode was too noisy.

[andriyDev]

Ah after using critcmp I got similar results:

group                                         deferred                               main
-----                                         --------                               ----
build_schedule/1000_schedule                  1.00       2.3±0.06s        ? ?/sec    1.05       2.4±0.11s        ? ?/sec
build_schedule/1000_schedule_noconstraints    1.07     41.5±3.04ms        ? ?/sec    1.00     39.0±1.29ms        ? ?/sec
build_schedule/100_schedule                   1.00      9.6±0.34ms        ? ?/sec    1.03     10.0±0.33ms        ? ?/sec
build_schedule/100_schedule_noconstraints     1.01   723.7±15.73µs        ? ?/sec    1.00   713.1±22.68µs        ? ?/sec
build_schedule/500_schedule                   1.01   403.0±43.40ms        ? ?/sec    1.00   399.9±12.32ms        ? ?/sec
build_schedule/500_schedule_noconstraints     1.00     10.5±0.61ms        ? ?/sec    1.03     10.8±0.69ms        ? ?/sec

That's very surprising that there's such a big difference between the measuring techniques. I wonder why. Based on this chart though it seems like the runtime is basically unchanged and more importantly within variance (sometimes main wins and sometimes this PR wins). This PR technically should be slower, but I don't imagine by much at all.

[andriyDev]

One thing I'm not fully certain of is whether this unlocks more parallelism.

After digging through the multi_threaded executor, I found this:

// NOTE: exclusive systems with ambiguities are susceptible to
// being significantly displaced here (compared to single-threaded order)
// if systems after them in topological order can run
// if that becomes an issue, `break;` if exclusive system

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

Line 406 in 00722b8

// NOTE: exclusive systems with ambiguities are susceptible to

This somewhat confirms my suspicions. That said, I realized that we still unlock additional parallelism by having fewer sync points. I was initially under the impression that there would always be another system accessing the world which would prevent exclusive systems (like sync points) from running until there were literally no non-exclusive systems left. However if all the currently running systems finish at about the same time (which seems plausible if you have several "idle systems" - systems that iterate an empty query), that gives a chance for an exclusive system to run (and so fewer exclusive systems is better).

So for once, the naive answer is correct haha.

[andriyDev]

I rebased. It was not entirely trivial due to #11094 being merged, so many of the changes had to be moved to auto_insert_apply_deferred.rs, but for the most part it was just a move.

[hymm]

whoops. Thought I had approved this already.

[urben1680]

Just smaller nits but I like the state of it now.

[urben1680]

What is the concern here for this assert? Since the other function has no inverse assert?

[andriyDev]

The other one does this implicitly. set_conditions_at panics if the node is not a set.

For the systems, there's no corresponding function, but we can implement it ourselves pretty simply. But we need to make sure this only happens for systems. In practice, we will never call this with a set, but better to be safe than sorry.

[urben1680]

You need to do this in a second iteration because until then you don't necessarily have all explicit sync points collected before adding new ones, right?

[andriyDev]

Not quite. The real problem is that we don't know the "distance" for the explicit sync node until it is "expanded" (the outer loop has it as node). Until then we're not sure which distance the sync node applies to since another edge may come along and "bump it".

[urben1680]

This and the other two functions kinda bloat this up. It might be worth it making them methods of ScheduleGraph since that is the first argument for all of them. Though the third and it's HashMap parameter make it probably even less useful in any other (future) context.

[andriyDev]

I kinda don't want to do this. If in the future this is a desirable thing to know for other cases, then we should probably use some dynamic programming to figure out which sets/systems overall have conditions. This is only "efficient" because we expect few ApplyDeferred stages in the first place, so querying for those in particular is probably cheaper.