Simplify self-edge checking in schedule building (#20015)

# Objective

Make the schedule graph code more understandable, and replace some
panics with `Result`s.

I found the `check_edges` and `check_hierarchy` functions [a little
confusing](https://github.com/bevyengine/bevy/pull/19352#discussion_r2181486099),
as they combined two concerns: Initializing graph nodes for system sets,
and checking for self-edges on system sets. It was hard to understand
the self-edge checks because it wasn't clear what `NodeId` was being
checked against! So, let's separate those concerns, and move them to
more appropriate locations.

Fix a bug where `schedule.configure_sets((SameSet, SameSet).chain());`
would panic with an unhelpful message: `assertion failed: index_a <
index_b`.

## Solution

Remove the `check_edges` and `check_hierarchy` functions, separating the
initialization behavior and the checking behavior and moving them where
they are easier to understand.

For initializing graph nodes, do this on-demand using the `entry` API by
replacing later `self.system_set_ids[&set]` calls with a
`self.system_sets.get_or_add_set(set)` method. This should avoid the
need for an extra pass over the graph and an extra lookup.

Unfortunately, implementing that method directly on `ScheduleGraph`
leads to borrowing errors as it borrows the entire `struct`. So, split
out the collections managing system sets into a separate `struct`.

For checking self-edges, move this check later so that it can be
reported by returning a `Result` from `Schedule::initialize` instead of
having to panic in `configure_set_inner`. The issue was that `iter_sccs`
does not report self-edges as cycles, since the resulting components
only have one node, but that later code assumes all edges point forward.
So, check for self-edges directly, immediately before calling
`iter_sccs`.

This also ensures we catch *every* way that self-edges can be added. The
previous code missed an edge case where `chain()`ing a set to itself
would create a self-edge and would trigger a `debug_assert`.
This commit is contained in:
Chris Russell 2025-07-08 14:09:45 -04:00 committed by GitHub
parent 9bb41a8309
commit 0ee937784e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 88 additions and 131 deletions

View File

@ -563,10 +563,21 @@ mod tests {
use super::*; use super::*;
#[test] #[test]
#[should_panic]
fn dependency_loop() { fn dependency_loop() {
let mut schedule = Schedule::default(); let mut schedule = Schedule::default();
schedule.configure_sets(TestSystems::X.after(TestSystems::X)); schedule.configure_sets(TestSystems::X.after(TestSystems::X));
let mut world = World::new();
let result = schedule.initialize(&mut world);
assert!(matches!(result, Err(ScheduleBuildError::DependencyLoop(_))));
}
#[test]
fn dependency_loop_from_chain() {
let mut schedule = Schedule::default();
schedule.configure_sets((TestSystems::X, TestSystems::X).chain());
let mut world = World::new();
let result = schedule.initialize(&mut world);
assert!(matches!(result, Err(ScheduleBuildError::DependencyLoop(_))));
} }
#[test] #[test]
@ -598,10 +609,12 @@ mod tests {
} }
#[test] #[test]
#[should_panic]
fn hierarchy_loop() { fn hierarchy_loop() {
let mut schedule = Schedule::default(); let mut schedule = Schedule::default();
schedule.configure_sets(TestSystems::X.in_set(TestSystems::X)); schedule.configure_sets(TestSystems::X.in_set(TestSystems::X));
let mut world = World::new();
let result = schedule.initialize(&mut world);
assert!(matches!(result, Err(ScheduleBuildError::HierarchyLoop(_))));
} }
#[test] #[test]

View File

@ -390,14 +390,14 @@ impl Schedule {
let a = a.into_system_set(); let a = a.into_system_set();
let b = b.into_system_set(); let b = b.into_system_set();
let Some(&a_id) = self.graph.system_set_ids.get(&a.intern()) else { let Some(&a_id) = self.graph.system_sets.ids.get(&a.intern()) else {
panic!( panic!(
"Could not mark system as ambiguous, `{:?}` was not found in the schedule. "Could not mark system as ambiguous, `{:?}` was not found in the schedule.
Did you try to call `ambiguous_with` before adding the system to the world?", Did you try to call `ambiguous_with` before adding the system to the world?",
a a
); );
}; };
let Some(&b_id) = self.graph.system_set_ids.get(&b.intern()) else { let Some(&b_id) = self.graph.system_sets.ids.get(&b.intern()) else {
panic!( panic!(
"Could not mark system as ambiguous, `{:?}` was not found in the schedule. "Could not mark system as ambiguous, `{:?}` was not found in the schedule.
Did you try to call `ambiguous_with` before adding the system to the world?", Did you try to call `ambiguous_with` before adding the system to the world?",
@ -760,6 +760,27 @@ enum UninitializedId {
}, },
} }
/// Metadata for system sets in a schedule.
#[derive(Default)]
struct SystemSets {
/// List of system sets in the schedule
sets: SlotMap<SystemSetKey, SystemSetNode>,
/// List of conditions for each system set, in the same order as `system_sets`
conditions: SecondaryMap<SystemSetKey, Vec<ConditionWithAccess>>,
/// Map from system set to node id
ids: HashMap<InternedSystemSet, SystemSetKey>,
}
impl SystemSets {
fn get_or_add_set(&mut self, set: InternedSystemSet) -> SystemSetKey {
*self.ids.entry(set).or_insert_with(|| {
let key = self.sets.insert(SystemSetNode::new(set));
self.conditions.insert(key, Vec::new());
key
})
}
}
/// Metadata for a [`Schedule`]. /// Metadata for a [`Schedule`].
/// ///
/// The order isn't optimized; calling `ScheduleGraph::build_schedule` will return a /// The order isn't optimized; calling `ScheduleGraph::build_schedule` will return a
@ -770,12 +791,8 @@ pub struct ScheduleGraph {
pub systems: SlotMap<SystemKey, SystemNode>, pub systems: SlotMap<SystemKey, SystemNode>,
/// List of conditions for each system, in the same order as `systems` /// List of conditions for each system, in the same order as `systems`
pub system_conditions: SecondaryMap<SystemKey, Vec<ConditionWithAccess>>, pub system_conditions: SecondaryMap<SystemKey, Vec<ConditionWithAccess>>,
/// List of system sets in the schedule /// Data about system sets in the schedule
system_sets: SlotMap<SystemSetKey, SystemSetNode>, system_sets: SystemSets,
/// List of conditions for each system set, in the same order as `system_sets`
system_set_conditions: SecondaryMap<SystemSetKey, Vec<ConditionWithAccess>>,
/// Map from system set to node id
system_set_ids: HashMap<InternedSystemSet, SystemSetKey>,
/// Systems that have not been initialized yet; for system sets, we store the index of the first uninitialized condition /// Systems that have not been initialized yet; for system sets, we store the index of the first uninitialized condition
/// (all the conditions after that index still need to be initialized) /// (all the conditions after that index still need to be initialized)
uninit: Vec<UninitializedId>, uninit: Vec<UninitializedId>,
@ -800,9 +817,7 @@ impl ScheduleGraph {
Self { Self {
systems: SlotMap::with_key(), systems: SlotMap::with_key(),
system_conditions: SecondaryMap::new(), system_conditions: SecondaryMap::new(),
system_sets: SlotMap::with_key(), system_sets: SystemSets::default(),
system_set_conditions: SecondaryMap::new(),
system_set_ids: HashMap::default(),
uninit: Vec::new(), uninit: Vec::new(),
hierarchy: Dag::new(), hierarchy: Dag::new(),
dependency: Dag::new(), dependency: Dag::new(),
@ -826,7 +841,7 @@ impl ScheduleGraph {
/// Returns `true` if the given system set is part of the graph. Otherwise, returns `false`. /// Returns `true` if the given system set is part of the graph. Otherwise, returns `false`.
pub fn contains_set(&self, set: impl SystemSet) -> bool { pub fn contains_set(&self, set: impl SystemSet) -> bool {
self.system_set_ids.contains_key(&set.intern()) self.system_sets.ids.contains_key(&set.intern())
} }
/// Returns the system at the given [`NodeId`]. /// Returns the system at the given [`NodeId`].
@ -840,7 +855,7 @@ impl ScheduleGraph {
/// Returns the set at the given [`NodeId`], if it exists. /// Returns the set at the given [`NodeId`], if it exists.
pub fn get_set_at(&self, key: SystemSetKey) -> Option<&dyn SystemSet> { pub fn get_set_at(&self, key: SystemSetKey) -> Option<&dyn SystemSet> {
self.system_sets.get(key).map(|set| &*set.inner) self.system_sets.sets.get(key).map(|set| &*set.inner)
} }
/// Returns the set at the given [`NodeId`]. /// Returns the set at the given [`NodeId`].
@ -854,7 +869,7 @@ impl ScheduleGraph {
/// Returns the conditions for the set at the given [`SystemSetKey`], if it exists. /// Returns the conditions for the set at the given [`SystemSetKey`], if it exists.
pub fn get_set_conditions_at(&self, key: SystemSetKey) -> Option<&[ConditionWithAccess]> { pub fn get_set_conditions_at(&self, key: SystemSetKey) -> Option<&[ConditionWithAccess]> {
self.system_set_conditions.get(key).map(Vec::as_slice) self.system_sets.conditions.get(key).map(Vec::as_slice)
} }
/// Returns the conditions for the set at the given [`SystemSetKey`]. /// Returns the conditions for the set at the given [`SystemSetKey`].
@ -882,9 +897,9 @@ impl ScheduleGraph {
pub fn system_sets( pub fn system_sets(
&self, &self,
) -> impl Iterator<Item = (SystemSetKey, &dyn SystemSet, &[ConditionWithAccess])> { ) -> impl Iterator<Item = (SystemSetKey, &dyn SystemSet, &[ConditionWithAccess])> {
self.system_sets.iter().filter_map(|(key, set_node)| { self.system_sets.sets.iter().filter_map(|(key, set_node)| {
let set = &*set_node.inner; let set = &*set_node.inner;
let conditions = self.system_set_conditions.get(key)?.as_slice(); let conditions = self.system_sets.conditions.get(key)?.as_slice();
Some((key, set, conditions)) Some((key, set, conditions))
}) })
} }
@ -946,7 +961,7 @@ impl ScheduleGraph {
} }
let mut set_config = InternedSystemSet::into_config(set.intern()); let mut set_config = InternedSystemSet::into_config(set.intern());
set_config.conditions.extend(collective_conditions); set_config.conditions.extend(collective_conditions);
self.configure_set_inner(set_config).unwrap(); self.configure_set_inner(set_config);
} }
} }
} }
@ -1047,10 +1062,7 @@ impl ScheduleGraph {
} }
/// Add a [`ScheduleConfig`] to the graph, including its dependencies and conditions. /// Add a [`ScheduleConfig`] to the graph, including its dependencies and conditions.
fn add_system_inner( fn add_system_inner(&mut self, config: ScheduleConfig<ScheduleSystem>) -> SystemKey {
&mut self,
config: ScheduleConfig<ScheduleSystem>,
) -> Result<NodeId, ScheduleBuildError> {
let key = self.systems.insert(SystemNode::new(config.node)); let key = self.systems.insert(SystemNode::new(config.node));
self.system_conditions.insert( self.system_conditions.insert(
key, key,
@ -1064,9 +1076,9 @@ impl ScheduleGraph {
self.uninit.push(UninitializedId::System(key)); self.uninit.push(UninitializedId::System(key));
// graph updates are immediate // graph updates are immediate
self.update_graphs(NodeId::System(key), config.metadata)?; self.update_graphs(NodeId::System(key), config.metadata);
Ok(NodeId::System(key)) key
} }
#[track_caller] #[track_caller]
@ -1075,39 +1087,26 @@ impl ScheduleGraph {
} }
/// Add a single `ScheduleConfig` to the graph, including its dependencies and conditions. /// Add a single `ScheduleConfig` to the graph, including its dependencies and conditions.
fn configure_set_inner( fn configure_set_inner(&mut self, set: ScheduleConfig<InternedSystemSet>) -> SystemSetKey {
&mut self,
set: ScheduleConfig<InternedSystemSet>,
) -> Result<NodeId, ScheduleBuildError> {
let ScheduleConfig { let ScheduleConfig {
node: set, node: set,
metadata, metadata,
conditions, conditions,
} = set; } = set;
let key = match self.system_set_ids.get(&set) { let key = self.system_sets.get_or_add_set(set);
Some(&id) => id,
None => self.add_set(set),
};
// graph updates are immediate // graph updates are immediate
self.update_graphs(NodeId::Set(key), metadata)?; self.update_graphs(NodeId::Set(key), metadata);
// system init has to be deferred (need `&mut World`) // system init has to be deferred (need `&mut World`)
let system_set_conditions = self.system_set_conditions.entry(key).unwrap().or_default(); let system_set_conditions = self.system_sets.conditions.entry(key).unwrap().or_default();
self.uninit.push(UninitializedId::Set { self.uninit.push(UninitializedId::Set {
key, key,
first_uninit_condition: system_set_conditions.len(), first_uninit_condition: system_set_conditions.len(),
}); });
system_set_conditions.extend(conditions.into_iter().map(ConditionWithAccess::new)); system_set_conditions.extend(conditions.into_iter().map(ConditionWithAccess::new));
Ok(NodeId::Set(key))
}
fn add_set(&mut self, set: InternedSystemSet) -> SystemSetKey {
let key = self.system_sets.insert(SystemSetNode::new(set));
self.system_set_conditions.insert(key, Vec::new());
self.system_set_ids.insert(set, key);
key key
} }
@ -1117,78 +1116,8 @@ impl ScheduleGraph {
AnonymousSet::new(id) AnonymousSet::new(id)
} }
/// Check that no set is included in itself.
/// Add all the sets from the [`GraphInfo`]'s hierarchy to the graph.
fn check_hierarchy_sets(
&mut self,
id: NodeId,
graph_info: &GraphInfo,
) -> Result<(), ScheduleBuildError> {
for &set in &graph_info.hierarchy {
if let Some(&set_id) = self.system_set_ids.get(&set) {
if let NodeId::Set(key) = id
&& set_id == key
{
{
return Err(ScheduleBuildError::HierarchyLoop(
self.get_node_name(&NodeId::Set(key)),
));
}
}
} else {
// If the set is not in the graph, we add it
self.add_set(set);
}
}
Ok(())
}
/// Checks that no system set is dependent on itself.
/// Add all the sets from the [`GraphInfo`]'s dependencies to the graph.
fn check_edges(
&mut self,
id: NodeId,
graph_info: &GraphInfo,
) -> Result<(), ScheduleBuildError> {
for Dependency { set, .. } in &graph_info.dependencies {
if let Some(&set_id) = self.system_set_ids.get(set) {
if let NodeId::Set(key) = id
&& set_id == key
{
return Err(ScheduleBuildError::DependencyLoop(
self.get_node_name(&NodeId::Set(key)),
));
}
} else {
// If the set is not in the graph, we add it
self.add_set(*set);
}
}
Ok(())
}
/// Add all the sets from the [`GraphInfo`]'s ambiguity to the graph.
fn add_ambiguities(&mut self, graph_info: &GraphInfo) {
if let Ambiguity::IgnoreWithSet(ambiguous_with) = &graph_info.ambiguous_with {
for set in ambiguous_with {
if !self.system_set_ids.contains_key(set) {
self.add_set(*set);
}
}
}
}
/// Update the internal graphs (hierarchy, dependency, ambiguity) by adding a single [`GraphInfo`] /// Update the internal graphs (hierarchy, dependency, ambiguity) by adding a single [`GraphInfo`]
fn update_graphs( fn update_graphs(&mut self, id: NodeId, graph_info: GraphInfo) {
&mut self,
id: NodeId,
graph_info: GraphInfo,
) -> Result<(), ScheduleBuildError> {
self.check_hierarchy_sets(id, &graph_info)?;
self.check_edges(id, &graph_info)?;
self.add_ambiguities(&graph_info);
self.changed = true; self.changed = true;
let GraphInfo { let GraphInfo {
@ -1201,16 +1130,22 @@ impl ScheduleGraph {
self.hierarchy.graph.add_node(id); self.hierarchy.graph.add_node(id);
self.dependency.graph.add_node(id); self.dependency.graph.add_node(id);
for key in sets.into_iter().map(|set| self.system_set_ids[&set]) { for key in sets
.into_iter()
.map(|set| self.system_sets.get_or_add_set(set))
{
self.hierarchy.graph.add_edge(NodeId::Set(key), id); self.hierarchy.graph.add_edge(NodeId::Set(key), id);
// ensure set also appears in dependency graph // ensure set also appears in dependency graph
self.dependency.graph.add_node(NodeId::Set(key)); self.dependency.graph.add_node(NodeId::Set(key));
} }
for (kind, key, options) in dependencies for (kind, key, options) in
.into_iter() dependencies
.map(|Dependency { kind, set, options }| (kind, self.system_set_ids[&set], options)) .into_iter()
.map(|Dependency { kind, set, options }| {
(kind, self.system_sets.get_or_add_set(set), options)
})
{ {
let (lhs, rhs) = match kind { let (lhs, rhs) = match kind {
DependencyKind::Before => (id, NodeId::Set(key)), DependencyKind::Before => (id, NodeId::Set(key)),
@ -1230,7 +1165,7 @@ impl ScheduleGraph {
Ambiguity::IgnoreWithSet(ambiguous_with) => { Ambiguity::IgnoreWithSet(ambiguous_with) => {
for key in ambiguous_with for key in ambiguous_with
.into_iter() .into_iter()
.map(|set| self.system_set_ids[&set]) .map(|set| self.system_sets.get_or_add_set(set))
{ {
self.ambiguous_with.add_edge(id, NodeId::Set(key)); self.ambiguous_with.add_edge(id, NodeId::Set(key));
} }
@ -1239,8 +1174,6 @@ impl ScheduleGraph {
self.ambiguous_with_all.insert(id); self.ambiguous_with_all.insert(id);
} }
} }
Ok(())
} }
/// Initializes any newly-added systems and conditions by calling [`System::initialize`](crate::system::System) /// Initializes any newly-added systems and conditions by calling [`System::initialize`](crate::system::System)
@ -1258,7 +1191,7 @@ impl ScheduleGraph {
key, key,
first_uninit_condition, first_uninit_condition,
} => { } => {
for condition in self.system_set_conditions[key] for condition in self.system_sets.conditions[key]
.iter_mut() .iter_mut()
.skip(first_uninit_condition) .skip(first_uninit_condition)
{ {
@ -1358,9 +1291,9 @@ impl ScheduleGraph {
HashMap<SystemSetKey, HashSet<SystemKey>>, HashMap<SystemSetKey, HashSet<SystemKey>>,
) { ) {
let mut set_systems: HashMap<SystemSetKey, Vec<SystemKey>> = let mut set_systems: HashMap<SystemSetKey, Vec<SystemKey>> =
HashMap::with_capacity_and_hasher(self.system_sets.len(), Default::default()); HashMap::with_capacity_and_hasher(self.system_sets.sets.len(), Default::default());
let mut set_system_sets: HashMap<SystemSetKey, HashSet<SystemKey>> = let mut set_system_sets: HashMap<SystemSetKey, HashSet<SystemKey>> =
HashMap::with_capacity_and_hasher(self.system_sets.len(), Default::default()); HashMap::with_capacity_and_hasher(self.system_sets.sets.len(), Default::default());
for &id in hierarchy_topsort.iter().rev() { for &id in hierarchy_topsort.iter().rev() {
let NodeId::Set(set_key) = id else { let NodeId::Set(set_key) = id else {
continue; continue;
@ -1559,7 +1492,7 @@ impl ScheduleGraph {
// ignore system sets that have no conditions // ignore system sets that have no conditions
// ignore system type sets (already covered, they don't have conditions) // ignore system type sets (already covered, they don't have conditions)
let key = id.as_set()?; let key = id.as_set()?;
(!self.system_set_conditions[key].is_empty()).then_some((i, key)) (!self.system_sets.conditions[key].is_empty()).then_some((i, key))
}) })
.unzip(); .unzip();
@ -1659,7 +1592,7 @@ impl ScheduleGraph {
.drain(..) .drain(..)
.zip(schedule.set_conditions.drain(..)) .zip(schedule.set_conditions.drain(..))
{ {
self.system_set_conditions[key] = conditions; self.system_sets.conditions[key] = conditions;
} }
*schedule = self.build_schedule(world, schedule_label, ignored_ambiguities)?; *schedule = self.build_schedule(world, schedule_label, ignored_ambiguities)?;
@ -1673,7 +1606,7 @@ impl ScheduleGraph {
} }
for &key in &schedule.set_ids { for &key in &schedule.set_ids {
let conditions = core::mem::take(&mut self.system_set_conditions[key]); let conditions = core::mem::take(&mut self.system_sets.conditions[key]);
schedule.set_conditions.push(conditions); schedule.set_conditions.push(conditions);
} }
@ -1700,13 +1633,13 @@ trait ProcessScheduleConfig: Schedulable + Sized {
impl ProcessScheduleConfig for ScheduleSystem { impl ProcessScheduleConfig for ScheduleSystem {
fn process_config(schedule_graph: &mut ScheduleGraph, config: ScheduleConfig<Self>) -> NodeId { fn process_config(schedule_graph: &mut ScheduleGraph, config: ScheduleConfig<Self>) -> NodeId {
schedule_graph.add_system_inner(config).unwrap() NodeId::System(schedule_graph.add_system_inner(config))
} }
} }
impl ProcessScheduleConfig for InternedSystemSet { impl ProcessScheduleConfig for InternedSystemSet {
fn process_config(schedule_graph: &mut ScheduleGraph, config: ScheduleConfig<Self>) -> NodeId { fn process_config(schedule_graph: &mut ScheduleGraph, config: ScheduleConfig<Self>) -> NodeId {
schedule_graph.configure_set_inner(config).unwrap() NodeId::Set(schedule_graph.configure_set_inner(config))
} }
} }
@ -1748,7 +1681,7 @@ impl ScheduleGraph {
} }
} }
NodeId::Set(key) => { NodeId::Set(key) => {
let set = &self.system_sets[key]; let set = &self.system_sets.sets[key];
if set.is_anonymous() { if set.is_anonymous() {
self.anonymous_set_name(id) self.anonymous_set_name(id)
} else { } else {
@ -1833,6 +1766,17 @@ impl ScheduleGraph {
graph: &DiGraph, graph: &DiGraph,
report: ReportCycles, report: ReportCycles,
) -> Result<Vec<NodeId>, ScheduleBuildError> { ) -> Result<Vec<NodeId>, ScheduleBuildError> {
// Check explicitly for self-edges.
// `iter_sccs` won't report them as cycles because they still form components of one node.
if let Some((node, _)) = graph.all_edges().find(|(left, right)| left == right) {
let name = self.get_node_name(&node);
let error = match report {
ReportCycles::Hierarchy => ScheduleBuildError::HierarchyLoop(name),
ReportCycles::Dependency => ScheduleBuildError::DependencyLoop(name),
};
return Err(error);
}
// Tarjan's SCC algorithm returns elements in *reverse* topological order. // Tarjan's SCC algorithm returns elements in *reverse* topological order.
let mut top_sorted_nodes = Vec::with_capacity(graph.node_count()); let mut top_sorted_nodes = Vec::with_capacity(graph.node_count());
let mut sccs_with_cycles = Vec::new(); let mut sccs_with_cycles = Vec::new();
@ -1963,7 +1907,7 @@ impl ScheduleGraph {
set_systems: &HashMap<SystemSetKey, Vec<SystemKey>>, set_systems: &HashMap<SystemSetKey, Vec<SystemKey>>,
) -> Result<(), ScheduleBuildError> { ) -> Result<(), ScheduleBuildError> {
for (&key, systems) in set_systems { for (&key, systems) in set_systems {
let set = &self.system_sets[key]; let set = &self.system_sets.sets[key];
if set.is_system_type() { if set.is_system_type() {
let instances = systems.len(); let instances = systems.len();
let ambiguous_with = self.ambiguous_with.edges(NodeId::Set(key)); let ambiguous_with = self.ambiguous_with.edges(NodeId::Set(key));
@ -2070,7 +2014,7 @@ impl ScheduleGraph {
fn names_of_sets_containing_node(&self, id: &NodeId) -> Vec<String> { fn names_of_sets_containing_node(&self, id: &NodeId) -> Vec<String> {
let mut sets = <HashSet<_>>::default(); let mut sets = <HashSet<_>>::default();
self.traverse_sets_containing_node(*id, &mut |key| { self.traverse_sets_containing_node(*id, &mut |key| {
!self.system_sets[key].is_system_type() && sets.insert(key) !self.system_sets.sets[key].is_system_type() && sets.insert(key)
}); });
let mut sets: Vec<_> = sets let mut sets: Vec<_> = sets
.into_iter() .into_iter()