From 5a9b70462a885ea8a4522c07f3fb22412dbe1878 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Wed, 13 Nov 2019 08:35:52 +1100 Subject: [PATCH 1/2] Improve comments about NodeStates. This commit clarifies some comments, fixes some minor errors in comments, and adds a state transition diagram. --- .../obligation_forest/mod.rs | 73 +++++++++++++------ 1 file changed, 52 insertions(+), 21 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 958ab617cb315..75ffa076b0848 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -130,12 +130,11 @@ type ObligationTreeIdGenerator = pub struct ObligationForest { /// The list of obligations. In between calls to /// `process_obligations`, this list only contains nodes in the - /// `Pending` or `Success` state (with a non-zero number of + /// `Pending` or `Waiting` state (with a non-zero number of /// incomplete children). During processing, some of those nodes /// may be changed to the error state, or we may find that they - /// are completed (That is, `num_incomplete_children` drops to 0). - /// At the end of processing, those nodes will be removed by a - /// call to `compress`. + /// are completed. At the end of processing, those nodes will be + /// removed by a call to `compress`. /// /// `usize` indices are used here and throughout this module, rather than /// `rustc_index::newtype_index!` indices, because this code is hot enough that the @@ -211,28 +210,56 @@ impl Node { /// represents the current state of processing for the obligation (of /// type `O`) associated with this node. /// -/// Outside of ObligationForest methods, nodes should be either Pending -/// or Waiting. +/// The non-`Error` state transitions are as follows. +/// ``` +/// (Pre-creation) +/// | +/// | register_obligation_at() (called by process_obligations() and +/// v from outside the crate) +/// Pending +/// | +/// | process_obligations() +/// v +/// Success +/// | ^ +/// | | mark_as_waiting() +/// | v +/// | Waiting +/// | +/// | process_cycles() +/// v +/// Done +/// | +/// | compress() +/// v +/// (Removed) +/// ``` +/// The `Error` state can be introduced in several places, via `error_at()`. +/// +/// Outside of `ObligationForest` methods, nodes should be either `Pending` or +/// `Waiting`. #[derive(Debug, Copy, Clone, PartialEq, Eq)] enum NodeState { - /// Obligations for which selection had not yet returned a - /// non-ambiguous result. + /// This obligation has not yet been selected successfully. Cannot have + /// subobligations. Pending, - /// This obligation was selected successfully, but may or - /// may not have subobligations. + /// This obligation was selected successfully, but the state of any + /// subobligations are current unknown. It will be converted to `Waiting` + /// or `Done` once the states of the subobligations become known. Success, - /// This obligation was selected successfully, but it has - /// a pending subobligation. + /// This obligation was selected successfully, but it has one or more + /// pending subobligations. Waiting, - /// This obligation, along with its subobligations, are complete, - /// and will be removed in the next collection. + /// This obligation was selected successfully, as were all of its + /// subobligations (of which there may be none). It will be removed by the + /// next compression step. Done, - /// This obligation was resolved to an error. Error nodes are - /// removed from the vector by the compression step. + /// This obligation was resolved to an error. It will be removed by the + /// next compression step. Error, } @@ -466,10 +493,9 @@ impl ObligationForest { } } - /// Mark all `NodeState::Success` nodes as `NodeState::Done` and - /// report all cycles between them. This should be called - /// after `mark_as_waiting` marks all nodes with pending - /// subobligations as NodeState::Waiting. + /// Mark all `Success` nodes as `Done` and report all cycles between them. + /// This should be called after `mark_as_waiting` updates the status of all + /// `Waiting` and `Success` nodes. fn process_cycles

(&self, processor: &mut P) where P: ObligationProcessor { @@ -575,14 +601,19 @@ impl ObligationForest { self.inlined_mark_neighbors_as_waiting_from(node) } - /// Marks all nodes that depend on a pending node as `NodeState::Waiting`. + /// Updates the states of all `Waiting` and `Success` nodes. Upon + /// completion, all such nodes that depend on a pending node will be marked + /// as `Waiting`, and all others will be marked as `Success`. fn mark_as_waiting(&self) { + // Optimistically mark all `Waiting` nodes as `Success`. for node in &self.nodes { if node.state.get() == NodeState::Waiting { node.state.set(NodeState::Success); } } + // Convert all `Success` nodes that still depend on a pending node to + // `Waiting`. This may undo some of the changes done in the loop above. for node in &self.nodes { if node.state.get() == NodeState::Pending { // This call site is hot. From c45fc6b104549fbcfc8819a6218be367d312cb4b Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Thu, 14 Nov 2019 17:08:22 +1100 Subject: [PATCH 2/2] Give two functions clearer names. --- .../obligation_forest/mod.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs index 75ffa076b0848..1ffa279b3aa8f 100644 --- a/src/librustc_data_structures/obligation_forest/mod.rs +++ b/src/librustc_data_structures/obligation_forest/mod.rs @@ -222,7 +222,7 @@ impl Node { /// v /// Success /// | ^ -/// | | mark_as_waiting() +/// | | update_waiting_and_success_states() /// | v /// | Waiting /// | @@ -480,7 +480,7 @@ impl ObligationForest { }; } - self.mark_as_waiting(); + self.update_waiting_and_success_states(); self.process_cycles(processor); let completed = self.compress(do_completed); @@ -494,8 +494,8 @@ impl ObligationForest { } /// Mark all `Success` nodes as `Done` and report all cycles between them. - /// This should be called after `mark_as_waiting` updates the status of all - /// `Waiting` and `Success` nodes. + /// This should be called after `update_waiting_and_success_states` updates + /// the status of all `Waiting` and `Success` nodes. fn process_cycles

(&self, processor: &mut P) where P: ObligationProcessor { @@ -577,7 +577,7 @@ impl ObligationForest { // This always-inlined function is for the hot call site. #[inline(always)] - fn inlined_mark_neighbors_as_waiting_from(&self, node: &Node) { + fn inlined_mark_dependents_as_waiting(&self, node: &Node) { for &index in node.dependents.iter() { let node = &self.nodes[index]; match node.state.get() { @@ -585,11 +585,11 @@ impl ObligationForest { NodeState::Success => { node.state.set(NodeState::Waiting); // This call site is cold. - self.uninlined_mark_neighbors_as_waiting_from(node); + self.uninlined_mark_dependents_as_waiting(node); } NodeState::Pending | NodeState::Done => { // This call site is cold. - self.uninlined_mark_neighbors_as_waiting_from(node); + self.uninlined_mark_dependents_as_waiting(node); } } } @@ -597,14 +597,14 @@ impl ObligationForest { // This never-inlined function is for the cold call site. #[inline(never)] - fn uninlined_mark_neighbors_as_waiting_from(&self, node: &Node) { - self.inlined_mark_neighbors_as_waiting_from(node) + fn uninlined_mark_dependents_as_waiting(&self, node: &Node) { + self.inlined_mark_dependents_as_waiting(node) } /// Updates the states of all `Waiting` and `Success` nodes. Upon /// completion, all such nodes that depend on a pending node will be marked /// as `Waiting`, and all others will be marked as `Success`. - fn mark_as_waiting(&self) { + fn update_waiting_and_success_states(&self) { // Optimistically mark all `Waiting` nodes as `Success`. for node in &self.nodes { if node.state.get() == NodeState::Waiting { @@ -617,7 +617,7 @@ impl ObligationForest { for node in &self.nodes { if node.state.get() == NodeState::Pending { // This call site is hot. - self.inlined_mark_neighbors_as_waiting_from(node); + self.inlined_mark_dependents_as_waiting(node); } } }