Simplify self-edge checking in schedule building #20015
Conversation
Create system set nodes on-demand so we don't need a second pass to check for them. Check for self edges in `topsort_graph` so that we can allow them through without panicking elsewhere.
alice-i-cecile
added
A-ECS
alice-i-cecile
added
C-Bug
| @@ -826,7 +841,7 @@ impl ScheduleGraph { | |||
|
|
|||
| /// Returns `true` if the given system set is part of the graph. Otherwise, returns `false`. | |||
| pub fn contains_set(&self, set: impl SystemSet) -> bool { | |||
| self.system_set_ids.contains_key(&set.intern()) | |||
| self.system_sets.system_set_ids.contains_key(&set.intern()) | |||
There was a problem hiding this comment.
Non-blocking, candidate for a follow up: The self.system_sets.system_setxxx stutter is mildly annoying; it would be nice to add functions on SystemSets that forwards to these.
| @@ -760,6 +760,27 @@ enum UninitializedId { | |||
| }, | |||
| } | |||
|
|
|||
| /// Metadata for system sets in a schedule. | |||
| #[derive(Default)] | |||
| struct SystemSets { | |||
There was a problem hiding this comment.
The design I have in mind for generalizing schedules includes splitting this out, glad you've taken the step for me :)
| @@ -1833,6 +1783,17 @@ impl ScheduleGraph { | |||
| graph: &DiGraph, | |||
| report: ReportCycles, | |||
| ) -> 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) { | |||
There was a problem hiding this comment.
Non-blocking, candidate for a follow up: We should report all self-edges rather than just the first one encountered.
There was a problem hiding this comment.
Non-blocking, candidate for a follow up: We should report all self-edges rather than just the first one encountered.
That is a good idea, but I'm not feeling enthusiastic about implementing it, so I'll just offer to review it if someone else does :).
alice-i-cecile
added
S-Ready-For-Final-Review
|
@chescock, let me know your call on the rename suggestions, then I'll do a final pass before merging. |
Objective
Make the schedule graph code more understandable, and replace some panics with
Results.I found the
check_edgesandcheck_hierarchyfunctions a little confusing, 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 whatNodeIdwas 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_edgesandcheck_hierarchyfunctions, 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
entryAPI by replacing laterself.system_set_ids[&set]calls with aself.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
ScheduleGraphleads to borrowing errors as it borrows the entirestruct. So, split out the collections managing system sets into a separatestruct.For checking self-edges, move this check later so that it can be reported by returning a
ResultfromSchedule::initializeinstead of having to panic inconfigure_set_inner. The issue was thatiter_sccsdoes 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 callingiter_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 adebug_assert.