From 6c68425ed5b03fb4921a84961e40ed210b47c707 Mon Sep 17 00:00:00 2001 From: Cameron <51241057+maniwani@users.noreply.github.com> Date: Wed, 1 Feb 2023 13:55:47 -0800 Subject: [PATCH 1/2] prettier cycle reporting --- crates/bevy_ecs/src/schedule/graph_utils.rs | 118 ++++++++++++++++++- crates/bevy_ecs/src/schedule/schedule.rs | 124 ++++++++++++++------ 2 files changed, 208 insertions(+), 34 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/graph_utils.rs b/crates/bevy_ecs/src/schedule/graph_utils.rs index 4caa5a8b1a316..f7e28e6bb5cd9 100644 --- a/crates/bevy_ecs/src/schedule/graph_utils.rs +++ b/crates/bevy_ecs/src/schedule/graph_utils.rs @@ -1,7 +1,7 @@ use std::fmt::Debug; use bevy_utils::{ - petgraph::{graphmap::NodeTrait, prelude::*}, + petgraph::{algo::TarjanScc, graphmap::NodeTrait, prelude::*}, HashMap, HashSet, }; use fixedbitset::FixedBitSet; @@ -274,3 +274,119 @@ where transitive_closure, } } + +/// Returns the simple cycles in a strongly-connected component of a directed graph. +/// +/// The algorithm implemented comes from +/// ["Finding all the elementary circuits of a directed graph"][1] by D. B. Johnson. +/// +/// [1]: https://doi.org/10.1137/0204007 +pub fn simple_cycles_in_component(graph: &DiGraphMap, scc: &[N]) -> Vec> +where + N: NodeTrait + Debug, +{ + let mut cycles = vec![]; + let mut sccs = vec![scc.to_vec()]; + + while let Some(mut scc) = sccs.pop() { + // only look at nodes and edges in this strongly-connected component + let mut subgraph = DiGraphMap::new(); + for &node in &scc { + subgraph.add_node(node); + } + + for &node in &scc { + for successor in graph.neighbors(node) { + if subgraph.contains_node(successor) { + subgraph.add_edge(node, successor, ()); + } + } + } + + // path of nodes that may form a cycle + let mut path = Vec::with_capacity(subgraph.node_count()); + // we mark nodes as "blocked" to avoid finding permutations of the same cycles + let mut blocked = HashSet::with_capacity(subgraph.node_count()); + // connects nodes along path segments that can't be part of a cycle (given current root) + // those nodes can be unblocked at the same time + let mut unblock_together: HashMap> = + HashMap::with_capacity(subgraph.node_count()); + // stack for unblocking nodes + let mut unblock_stack = Vec::with_capacity(subgraph.node_count()); + // nodes can be involved in multiple cycles + let mut maybe_in_more_cycles: HashSet = HashSet::with_capacity(subgraph.node_count()); + // stack for DFS + let mut stack = Vec::with_capacity(subgraph.node_count()); + + // we're going to look for all cycles that begin and end at this node + let root = scc.pop().unwrap(); + // start a path at the root + path.clear(); + path.push(root); + // mark this node as blocked + blocked.insert(root); + + // DFS + stack.clear(); + stack.push((root, subgraph.neighbors(root))); + while !stack.is_empty() { + let (ref node, successors) = stack.last_mut().unwrap(); + if let Some(next) = successors.next() { + if next == root { + // found a cycle + maybe_in_more_cycles.extend(path.iter()); + cycles.push(path.clone()); + } else if !blocked.contains(&next) { + // first time seeing `next` on this path + maybe_in_more_cycles.remove(&next); + path.push(next); + blocked.insert(next); + stack.push((next, subgraph.neighbors(next))); + continue; + } else { + // not first time seeing `next` on this path + } + } + + if successors.peekable().peek().is_none() { + if maybe_in_more_cycles.contains(node) { + unblock_stack.push(*node); + // unblock this node's ancestors + while let Some(n) = unblock_stack.pop() { + if blocked.remove(&n) { + let unblock_predecessors = + unblock_together.entry(n).or_insert_with(HashSet::new); + unblock_stack.extend(unblock_predecessors.iter()); + unblock_predecessors.clear(); + } + } + } else { + // if its descendants can be unblocked later, this node will be too + for successor in subgraph.neighbors(*node) { + unblock_together + .entry(successor) + .or_insert_with(HashSet::new) + .insert(*node); + } + } + + // remove node from path and DFS stack + path.pop(); + stack.pop(); + } + } + + // remove node from subgraph + subgraph.remove_node(root); + + // divide remainder into smaller SCCs + let mut tarjan_scc = TarjanScc::new(); + tarjan_scc.run(&subgraph, |scc| { + if scc.len() > 1 { + sccs.push(scc.to_vec()); + } + }); + } + + cycles +} diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 78ba800ec32fc..384372d5101cc 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -7,7 +7,7 @@ use bevy_utils::default; #[cfg(feature = "trace")] use bevy_utils::tracing::info_span; use bevy_utils::{ - petgraph::prelude::*, + petgraph::{algo::TarjanScc, prelude::*}, thiserror::Error, tracing::{error, warn}, HashMap, HashSet, @@ -942,7 +942,7 @@ impl ScheduleGraph { // check hierarchy for cycles self.hierarchy.topsort = self - .topsort_graph(&self.hierarchy.graph) + .topsort_graph(&self.hierarchy.graph, ReportCycles::Hierarchy) .map_err(|_| ScheduleBuildError::HierarchyCycle)?; let hier_results = check_graph(&self.hierarchy.graph, &self.hierarchy.topsort); @@ -960,7 +960,7 @@ impl ScheduleGraph { // check dependencies for cycles self.dependency.topsort = self - .topsort_graph(&self.dependency.graph) + .topsort_graph(&self.dependency.graph, ReportCycles::Dependency) .map_err(|_| ScheduleBuildError::DependencyCycle)?; // check for systems or system sets depending on sets they belong to @@ -1083,7 +1083,7 @@ impl ScheduleGraph { // topsort self.dependency_flattened.topsort = self - .topsort_graph(&dependency_flattened) + .topsort_graph(&dependency_flattened, ReportCycles::Dependency) .map_err(|_| ScheduleBuildError::DependencyCycle)?; self.dependency_flattened.graph = dependency_flattened; @@ -1318,6 +1318,13 @@ impl ScheduleGraph { } } +/// Used to select the appropriate reporting function. +enum ReportCycles { + None, + Hierarchy, + Dependency, +} + // methods for reporting errors impl ScheduleGraph { fn get_node_name(&self, id: &NodeId) -> String { @@ -1345,7 +1352,7 @@ impl ScheduleGraph { name } - fn get_node_kind(id: &NodeId) -> &'static str { + fn get_node_kind(&self, id: &NodeId) -> &'static str { match id { NodeId::System(_) => "system", NodeId::Set(_) => "system set", @@ -1366,7 +1373,7 @@ impl ScheduleGraph { writeln!( message, " -- {:?} '{:?}' cannot be child of set '{:?}', longer path exists", - Self::get_node_kind(child), + self.get_node_kind(child), self.get_node_name(child), self.get_node_name(parent), ) @@ -1376,48 +1383,99 @@ impl ScheduleGraph { error!("{}", message); } - /// Get topology sorted [`NodeId`], also ensures the graph contains no cycle - /// returns Err(()) if there are cycles - fn topsort_graph(&self, graph: &DiGraphMap) -> Result, ()> { - // tarjan_scc's run order is reverse topological order - let mut rev_top_sorted_nodes = Vec::::with_capacity(graph.node_count()); - let mut tarjan_scc = bevy_utils::petgraph::algo::TarjanScc::new(); - let mut sccs_with_cycle = Vec::>::new(); + /// Tries to topologically sort `graph`. + /// + /// If the graph is acyclic, returns [`Ok`] with the list of [`NodeId`] in a valid + /// topological order. If the graph contains cycles, returns [`Err`] with the list of + /// strongly-connected components that contain cycles (also in a valid topological order). + /// + /// # Errors + /// + /// If the graph contain cycles, then an error is returned. + fn topsort_graph( + &self, + graph: &DiGraphMap, + report: ReportCycles, + ) -> Result, Vec>> { + // Tarjan's SCC algorithm returns elements in *reverse* topological order. + let mut tarjan_scc = TarjanScc::new(); + let mut top_sorted_nodes = Vec::with_capacity(graph.node_count()); + let mut sccs_with_cycles = Vec::new(); tarjan_scc.run(graph, |scc| { - // by scc's definition, each scc is the cluster of nodes that they can reach each other - // so scc with size larger than 1, means there is/are cycle(s). + // A strongly-connected component is a group of nodes who can all reach each other + // through one or more paths. If an SCC contains more than one node, there must be + // at least one cycle within them. if scc.len() > 1 { - sccs_with_cycle.push(scc.to_vec()); + sccs_with_cycles.push(scc.to_vec()); } - rev_top_sorted_nodes.extend_from_slice(scc); + top_sorted_nodes.extend_from_slice(scc); }); - if sccs_with_cycle.is_empty() { - // reverse the reverted to get topological order - let mut top_sorted_nodes = rev_top_sorted_nodes; + if sccs_with_cycles.is_empty() { + // reverse to get topological order top_sorted_nodes.reverse(); Ok(top_sorted_nodes) } else { - self.report_cycles(&sccs_with_cycle); - Err(()) + let mut cycles = Vec::new(); + for scc in &sccs_with_cycles { + cycles.append(&mut simple_cycles_in_component(graph, scc)); + } + + match report { + ReportCycles::Hierarchy => self.report_hierarchy_cycles(&cycles), + ReportCycles::Dependency => self.report_dependency_cycles(&cycles), + ReportCycles::None => (), + } + + Err(sccs_with_cycles) + } + } + + /// Logs details of cycles in the hierarchy graph. + fn report_hierarchy_cycles(&self, cycles: &[Vec]) { + let mut message = format!("{} `in_set` cycles found in schedule:\n", cycles.len()); + for (i, cycle) in cycles.iter().enumerate() { + let mut names = cycle.iter().map(|id| self.get_node_name(id)); + let first_name = names.next().unwrap(); + writeln!( + message, + "cycle {}: set '{first_name}' contains itself. Cycle:", + i + 1, + ) + .unwrap(); + writeln!(message, "set '{first_name}'").unwrap(); + for name in names.chain(std::iter::once(first_name)) { + writeln!(message, " ... which contains set '{name}'").unwrap(); + } + writeln!(message).unwrap(); } + + error!("{}", message); } - /// Print detailed cycle messages - fn report_cycles(&self, sccs_with_cycles: &[Vec]) { + /// Logs details of cycles in the dependency graph. + fn report_dependency_cycles(&self, cycles: &[Vec]) { let mut message = format!( - "schedule contains at least {} cycle(s)", - sccs_with_cycles.len() + "{} `before`/`after` cycles found in schedule:\n", + cycles.len() ); - - writeln!(message, " -- cycle(s) found within:").unwrap(); - for (i, scc) in sccs_with_cycles.iter().enumerate() { - let names = scc + for (i, cycle) in cycles.iter().enumerate() { + let mut names = cycle .iter() - .map(|id| self.get_node_name(id)) - .collect::>(); - writeln!(message, " ---- {i}: {names:?}").unwrap(); + .map(|id| (self.get_node_kind(id), self.get_node_name(id))); + let (first_kind, first_name) = names.next().unwrap(); + writeln!( + message, + "cycle {}: {first_kind} '{first_name}' must run before itself. Cycle:", + i + 1, + ) + .unwrap(); + writeln!(message, "{first_kind} '{first_name}'").unwrap(); + for (kind, name) in names.chain(std::iter::once((first_kind, first_name))) { + writeln!(message, " ... which must run before {kind} '{name}'").unwrap(); + } + writeln!(message).unwrap(); } error!("{}", message); From 0b954c100a66e3e9a0b4a610dac89952be3350d9 Mon Sep 17 00:00:00 2001 From: Cameron <51241057+maniwani@users.noreply.github.com> Date: Mon, 20 Feb 2023 19:03:37 -0800 Subject: [PATCH 2/2] formatting --- crates/bevy_ecs/src/schedule/schedule.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/schedule/schedule.rs b/crates/bevy_ecs/src/schedule/schedule.rs index 384372d5101cc..b011564bcaa1c 100644 --- a/crates/bevy_ecs/src/schedule/schedule.rs +++ b/crates/bevy_ecs/src/schedule/schedule.rs @@ -1320,7 +1320,6 @@ impl ScheduleGraph { /// Used to select the appropriate reporting function. enum ReportCycles { - None, Hierarchy, Dependency, } @@ -1425,7 +1424,6 @@ impl ScheduleGraph { match report { ReportCycles::Hierarchy => self.report_hierarchy_cycles(&cycles), ReportCycles::Dependency => self.report_dependency_cycles(&cycles), - ReportCycles::None => (), } Err(sccs_with_cycles) @@ -1434,13 +1432,13 @@ impl ScheduleGraph { /// Logs details of cycles in the hierarchy graph. fn report_hierarchy_cycles(&self, cycles: &[Vec]) { - let mut message = format!("{} `in_set` cycles found in schedule:\n", cycles.len()); + let mut message = format!("schedule has {} in_set cycle(s):\n", cycles.len()); for (i, cycle) in cycles.iter().enumerate() { let mut names = cycle.iter().map(|id| self.get_node_name(id)); let first_name = names.next().unwrap(); writeln!( message, - "cycle {}: set '{first_name}' contains itself. Cycle:", + "cycle {}: set '{first_name}' contains itself", i + 1, ) .unwrap(); @@ -1456,10 +1454,7 @@ impl ScheduleGraph { /// Logs details of cycles in the dependency graph. fn report_dependency_cycles(&self, cycles: &[Vec]) { - let mut message = format!( - "{} `before`/`after` cycles found in schedule:\n", - cycles.len() - ); + let mut message = format!("schedule has {} before/after cycle(s):\n", cycles.len()); for (i, cycle) in cycles.iter().enumerate() { let mut names = cycle .iter() @@ -1467,7 +1462,7 @@ impl ScheduleGraph { let (first_kind, first_name) = names.next().unwrap(); writeln!( message, - "cycle {}: {first_kind} '{first_name}' must run before itself. Cycle:", + "cycle {}: {first_kind} '{first_name}' must run before itself", i + 1, ) .unwrap();