Auto insert sync points (#9822)

# Objective - Users are often confused when their command effects are not visible in the next system. This PR auto inserts sync points if there are deferred buffers on a system and there are dependents on that system (systems with after relationships). - Manual sync points can lead to users adding more than needed and it's hard for the user to have a global understanding of their system graph to know which sync points can be merged. However we can easily calculate which sync points can be merged automatically. ## Solution 1. Add new edge types to allow opting out of new behavior 2. Insert an sync point for each edge whose initial node has deferred system params. 3. Reuse nodes if they're at the number of sync points away. * add opt outs for specific edges with `after_ignore_deferred`, `before_ignore_deferred` and `chain_ignore_deferred`. The `auto_insert_apply_deferred` boolean on `ScheduleBuildSettings` can be set to false to opt out for the whole schedule. ## Perf This has a small negative effect on schedule build times. ```text group auto-sync main-for-auto-sync ----- ----------- ------------------ build_schedule/1000_schedule 1.06 2.8±0.15s ? ?/sec 1.00 2.7±0.06s ? ?/sec build_schedule/1000_schedule_noconstraints 1.01 26.2±0.88ms ? ?/sec 1.00 25.8±0.36ms ? ?/sec build_schedule/100_schedule 1.02 13.1±0.33ms ? ?/sec 1.00 12.9±0.28ms ? ?/sec build_schedule/100_schedule_noconstraints 1.08 505.3±29.30µs ? ?/sec 1.00 469.4±12.48µs ? ?/sec build_schedule/500_schedule 1.00 485.5±6.29ms ? ?/sec 1.00 485.5±9.80ms ? ?/sec build_schedule/500_schedule_noconstraints 1.00 6.8±0.10ms ? ?/sec 1.02 6.9±0.16ms ? ?/sec ``` --- ## Changelog - Auto insert sync points and added `after_ignore_deferred`, `before_ignore_deferred`, `chain_no_deferred` and `auto_insert_apply_deferred` APIs to opt out of this behavior ## Migration Guide - `apply_deferred` points are added automatically when there is ordering relationship with a system that has deferred parameters like `Commands`. If you want to opt out of this you can switch from `after`, `before`, and `chain` to the corresponding `ignore_deferred` API, `after_ignore_deferred`, `before_ignore_deferred` or `chain_ignore_deferred` for your system/set ordering. - You can also set `ScheduleBuildSettings::auto_insert_sync_points` to `false` if you want to do it for the whole schedule. Note that in this mode you can still add `apply_deferred` points manually. - For most manual insertions of `apply_deferred` you should remove them as they cannot be merged with the automatically inserted points and might reduce parallelizability of the system graph. ## TODO - [x] remove any apply_deferred used in the engine - [x] ~~decide if we should deprecate manually using apply_deferred.~~ We'll still allow inserting manual sync points for now for whatever edge cases users might have. - [x] Update migration guide - [x] rerun schedule build benchmarks --------- Co-authored-by: Joseph <[email protected]>
bevyengine · Dec 14, 2023 · 6b84ba9 · 6b84ba9
1 parent 720d6da
commit 6b84ba9
Show file tree

Hide file tree

Showing 13 changed files with 837 additions and 95 deletions.
diff --git a/crates/bevy_ecs/src/schedule/config.rs b/crates/bevy_ecs/src/schedule/config.rs
@@ -5,6 +5,7 @@ use crate::{
  condition::{BoxedCondition, Condition},
  graph_utils::{Ambiguity, Dependency, DependencyKind, GraphInfo},
  set::{InternedSystemSet, IntoSystemSet, SystemSet},
+ Chain,
  },
  system::{BoxedSystem, IntoSystem, System},
 };
@@ -67,8 +68,8 @@ pub enum NodeConfigs<T> {
  configs: Vec<NodeConfigs<T>>,
  /// Run conditions applied to everything in the tuple.
  collective_conditions: Vec<BoxedCondition>,
- /// If `true`, adds `before -> after` ordering constraints between the successive elements.
- chained: bool,
+ /// See [`Chain`] for usage.
+ chained: Chain,
  },
 }
 
@@ -137,6 +138,38 @@ impl<T> NodeConfigs<T> {
  }
  }
 
+ fn before_ignore_deferred_inner(&mut self, set: InternedSystemSet) {
+ match self {
+ Self::NodeConfig(config) => {
+ config
+ .graph_info
+ .dependencies
+ .push(Dependency::new(DependencyKind::BeforeNoSync, set));
+ }
+ Self::Configs { configs, .. } => {
+ for config in configs {
+ config.before_ignore_deferred_inner(set.intern());
+ }
+ }
+ }
+ }
+
+ fn after_ignore_deferred_inner(&mut self, set: InternedSystemSet) {
+ match self {
+ Self::NodeConfig(config) => {
+ config
+ .graph_info
+ .dependencies
+ .push(Dependency::new(DependencyKind::AfterNoSync, set));
+ }
+ Self::Configs { configs, .. } => {
+ for config in configs {
+ config.after_ignore_deferred_inner(set.intern());
+ }
+ }
+ }
+ }
+
  fn distributive_run_if_inner<M>(&mut self, condition: impl Condition<M> + Clone) {
  match self {
  Self::NodeConfig(config) => {
@@ -198,7 +231,17 @@ impl<T> NodeConfigs<T> {
  match &mut self {
  Self::NodeConfig(_) => { /* no op */ }
  Self::Configs { chained, .. } => {
- *chained = true;
+ *chained = Chain::Yes;
+ }
+ }
+ self
+ }
+
+ fn chain_ignore_deferred_inner(mut self) -> Self {
+ match &mut self {
+ Self::NodeConfig(_) => { /* no op */ }
+ Self::Configs { chained, .. } => {
+ *chained = Chain::YesIgnoreDeferred;
  }
  }
  self
@@ -252,22 +295,46 @@ where
  self.into_configs().in_set(set)
  }
 
- /// Run before all systems in `set`.
+ /// Runs before all systems in `set`. If `self` has any systems that produce [`Commands`](crate::system::Commands)
+ /// or other [`Deferred`](crate::system::Deferred) operations, all systems in `set` will see their effect.
+ ///
+ /// If automatically inserting [`apply_deferred`](crate::schedule::apply_deferred) like
+ /// this isn't desired, use [`before_ignore_deferred`](Self::before_ignore_deferred) instead.
  ///
  /// Note: The given set is not implicitly added to the schedule when this system set is added.
  /// It is safe, but no dependencies will be created.
  fn before<M>(self, set: impl IntoSystemSet<M>) -> SystemConfigs {
  self.into_configs().before(set)
  }
 
- /// Run after all systems in `set`.
+ /// Run after all systems in `set`. If `set` has any systems that produce [`Commands`](crate::system::Commands)
+ /// or other [`Deferred`](crate::system::Deferred) operations, all systems in `self` will see their effect.
+ ///
+ /// If automatically inserting [`apply_deferred`](crate::schedule::apply_deferred) like
+ /// this isn't desired, use [`after_ignore_deferred`](Self::after_ignore_deferred) instead.
  ///
  /// Note: The given set is not implicitly added to the schedule when this system set is added.
  /// It is safe, but no dependencies will be created.
  fn after<M>(self, set: impl IntoSystemSet<M>) -> SystemConfigs {
  self.into_configs().after(set)
  }
 
+ /// Run before all systems in `set`.
+ ///
+ /// Unlike [`before`](Self::before), this will not cause the systems in
+ /// `set` to wait for the deferred effects of `self` to be applied.
+ fn before_ignore_deferred<M>(self, set: impl IntoSystemSet<M>) -> SystemConfigs {
+ self.into_configs().before_ignore_deferred(set)
+ }
+
+ /// Run after all systems in `set`.
+ ///
+ /// Unlike [`after`](Self::after), this will not wait for the deferred
+ /// effects of systems in `set` to be applied.
+ fn after_ignore_deferred<M>(self, set: impl IntoSystemSet<M>) -> SystemConfigs {
+ self.into_configs().after_ignore_deferred(set)
+ }
+
  /// Add a run condition to each contained system.
  ///
  /// Each system will receive its own clone of the [`Condition`] and will only run
@@ -351,9 +418,22 @@ where
  /// Treat this collection as a sequence of systems.
  ///
  /// Ordering constraints will be applied between the successive elements.
+ ///
+ /// If the preceeding node on a edge has deferred parameters, a [`apply_deferred`](crate::schedule::apply_deferred)
+ /// will be inserted on the edge. If this behavior is not desired consider using
+ /// [`chain_ignore_deferred`](Self::chain_ignore_deferred) instead.
  fn chain(self) -> SystemConfigs {
  self.into_configs().chain()
  }
+
+ /// Treat this collection as a sequence of systems.
+ ///
+ /// Ordering constraints will be applied between the successive elements.
+ ///
+ /// Unlike [`chain`](Self::chain) this will **not** add [`apply_deferred`](crate::schedule::apply_deferred) on the edges.
+ fn chain_ignore_deferred(self) -> SystemConfigs {
+ self.into_configs().chain_ignore_deferred()
+ }
 }
 
 impl IntoSystemConfigs<()> for SystemConfigs {
@@ -385,6 +465,18 @@ impl IntoSystemConfigs<()> for SystemConfigs {
  self
  }
 
+ fn before_ignore_deferred<M>(mut self, set: impl IntoSystemSet<M>) -> Self {
+ let set = set.into_system_set();
+ self.before_ignore_deferred_inner(set.intern());
+ self
+ }
+
+ fn after_ignore_deferred<M>(mut self, set: impl IntoSystemSet<M>) -> Self {
+ let set = set.into_system_set();
+ self.after_ignore_deferred_inner(set.intern());
+ self
+ }
+
  fn distributive_run_if<M>(mut self, condition: impl Condition<M> + Clone) -> SystemConfigs {
  self.distributive_run_if_inner(condition);
  self
@@ -409,6 +501,10 @@ impl IntoSystemConfigs<()> for SystemConfigs {
  fn chain(self) -> Self {
  self.chain_inner()
  }
+
+ fn chain_ignore_deferred(self) -> Self {
+ self.chain_ignore_deferred_inner()
+ }
 }
 
 #[doc(hidden)]
@@ -426,7 +522,7 @@ macro_rules! impl_system_collection {
  SystemConfigs::Configs {
  configs: vec![$($sys.into_configs(),)*],
  collective_conditions: Vec::new(),
- chained: false,
+ chained: Chain::No,
  }
  }
  }
@@ -474,16 +570,40 @@ where
  self.into_configs().in_set(set)
  }
 
- /// Run before all systems in `set`.
+ /// Runs before all systems in `set`. If `self` has any systems that produce [`Commands`](crate::system::Commands)
+ /// or other [`Deferred`](crate::system::Deferred) operations, all systems in `set` will see their effect.
+ ///
+ /// If automatically inserting [`apply_deferred`](crate::schedule::apply_deferred) like
+ /// this isn't desired, use [`before_ignore_deferred`](Self::before_ignore_deferred) instead.
  fn before<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfigs {
  self.into_configs().before(set)
  }
 
- /// Run after all systems in `set`.
+ /// Runs before all systems in `set`. If `set` has any systems that produce [`Commands`](crate::system::Commands)
+ /// or other [`Deferred`](crate::system::Deferred) operations, all systems in `self` will see their effect.
+ ///
+ /// If automatically inserting [`apply_deferred`](crate::schedule::apply_deferred) like
+ /// this isn't desired, use [`after_ignore_deferred`](Self::after_ignore_deferred) instead.
  fn after<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfigs {
  self.into_configs().after(set)
  }
 
+ /// Run before all systems in `set`.
+ ///
+ /// Unlike [`before`](Self::before), this will not cause the systems in `set` to wait for the
+ /// deferred effects of `self` to be applied.
+ fn before_ignore_deferred<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfigs {
+ self.into_configs().before_ignore_deferred(set)
+ }
+
+ /// Run after all systems in `set`.
+ ///
+ /// Unlike [`after`](Self::after), this may not see the deferred
+ /// effects of systems in `set` to be applied.
+ fn after_ignore_deferred<M>(self, set: impl IntoSystemSet<M>) -> SystemSetConfigs {
+ self.into_configs().after_ignore_deferred(set)
+ }
+
  /// Run the systems in this set(s) only if the [`Condition`] is `true`.
  ///
  /// The `Condition` will be evaluated at most once (per schedule run),
@@ -510,6 +630,15 @@ where
  fn chain(self) -> SystemSetConfigs {
  self.into_configs().chain()
  }
+
+ /// Treat this collection as a sequence of systems.
+ ///
+ /// Ordering constraints will be applied between the successive elements.
+ ///
+ /// Unlike [`chain`](Self::chain) this will **not** add [`apply_deferred`](crate::schedule::apply_deferred) on the edges.
+ fn chain_ignore_deferred(self) -> SystemConfigs {
+ self.into_configs().chain_ignore_deferred()
+ }
 }
 
 impl IntoSystemSetConfigs for SystemSetConfigs {
@@ -542,6 +671,20 @@ impl IntoSystemSetConfigs for SystemSetConfigs {
  self
  }
 
+ fn before_ignore_deferred<M>(mut self, set: impl IntoSystemSet<M>) -> Self {
+ let set = set.into_system_set();
+ self.before_ignore_deferred_inner(set.intern());
+
+ self
+ }
+
+ fn after_ignore_deferred<M>(mut self, set: impl IntoSystemSet<M>) -> Self {
+ let set = set.into_system_set();
+ self.after_ignore_deferred_inner(set.intern());
+
+ self
+ }
+
  fn run_if<M>(mut self, condition: impl Condition<M>) -> SystemSetConfigs {
  self.run_if_dyn(new_condition(condition));
 
@@ -588,7 +731,7 @@ macro_rules! impl_system_set_collection {
  SystemSetConfigs::Configs {
  configs: vec![$($set.into_configs(),)*],
  collective_conditions: Vec::new(),
- chained: false,
+ chained: Chain::No,
  }
  }
  }

diff --git a/crates/bevy_ecs/src/schedule/graph_utils.rs b/crates/bevy_ecs/src/schedule/graph_utils.rs
@@ -45,6 +45,10 @@ pub(crate) enum DependencyKind {
  Before,
  /// A node that should be succeeded.
  After,
+ /// A node that should be preceded and will **not** automatically insert an instance of `apply_deferred` on the edge.
+ BeforeNoSync,
+ /// A node that should be succeeded and will **not** automatically insert an instance of `apply_deferred` on the edge.
+ AfterNoSync,
 }
 
 /// An edge to be added to the dependency graph.