Auto merge of #36993 - nnethercote:obligation, r=nikomatsakis

Optimize ObligationForest's NodeState handling.

This commit does the following.
- Changes `NodeState` from an enum to a `bitflags`. This makes it
  possible to check against multiple possible values in a single bitwise
  operation.
- Replaces all the hot `match`es involving `NodeState` with `if`/`else`
  chains that ensure that cases are handled in the order of frequency.
- Partially inlines two functions, `find_cycles_from_node` and
  `mark_as_waiting_from`, at two call sites in order to avoid function
  unnecessary function calls on hot paths.
- Fully inlines and removes `is_popped`.

These changes speeds up rustc-benchmarks/inflate-0.1.0 by about 7% when
doing debug builds with a stage1 compiler.

r? @arielb1

1 files changed, 37 insertions, 34 deletions

diff --git a/src/librustc_data_structures/obligation_forest/mod.rs b/src/librustc_data_structures/obligation_forest/mod.rs
index 5e590cb445f..a2bfa784e8a 100644
--- a/src/librustc_data_structures/obligation_forest/mod.rs
+++ b/src/librustc_data_structures/obligation_forest/mod.rs
@@ -377,8 +377,16 @@ impl<O: ForestObligation> ObligationForest<O> {
     {
         let mut stack = self.scratch.take().unwrap();
 
-        for node in 0..self.nodes.len() {
-            self.find_cycles_from_node(&mut stack, processor, node);
+        for index in 0..self.nodes.len() {
+            // For rustc-benchmarks/inflate-0.1.0 this state test is extremely
+            // hot and the state is almost always `Pending` or `Waiting`. It's
+            // a win to handle the no-op cases immediately to avoid the cost of
+            // the function call.
+            let state = self.nodes[index].state.get();
+            match state {
+                NodeState::Waiting | NodeState::Pending | NodeState::Done | NodeState::Error => {},
+                _ => self.find_cycles_from_node(&mut stack, processor, index),
+            }
         }
 
         self.scratch = Some(stack);
@@ -476,7 +484,18 @@ impl<O: ForestObligation> ObligationForest<O> {
         trace
     }
 
-    /// Marks all nodes that depend on a pending node as NodeState;:Waiting.
+    #[inline]
+    fn mark_neighbors_as_waiting_from(&self, node: &Node<O>) {
+        if let Some(parent) = node.parent {
+            self.mark_as_waiting_from(&self.nodes[parent.get()]);
+        }
+
+        for dependent in &node.dependents {
+            self.mark_as_waiting_from(&self.nodes[dependent.get()]);
+        }
+    }
+
+    /// Marks all nodes that depend on a pending node as NodeState::Waiting.
     fn mark_as_waiting(&self) {
         for node in &self.nodes {
             if node.state.get() == NodeState::Waiting {
@@ -486,27 +505,19 @@ impl<O: ForestObligation> ObligationForest<O> {
 
         for node in &self.nodes {
             if node.state.get() == NodeState::Pending {
-                self.mark_as_waiting_from(node)
+                self.mark_neighbors_as_waiting_from(node);
             }
         }
     }
 
     fn mark_as_waiting_from(&self, node: &Node<O>) {
         match node.state.get() {
-            NodeState::Pending | NodeState::Done => {},
             NodeState::Waiting | NodeState::Error | NodeState::OnDfsStack => return,
-            NodeState::Success => {
-                node.state.set(NodeState::Waiting);
-            }
-        }
-
-        if let Some(parent) = node.parent {
-            self.mark_as_waiting_from(&self.nodes[parent.get()]);
+            NodeState::Success => node.state.set(NodeState::Waiting),
+            NodeState::Pending | NodeState::Done => {},
         }
 
-        for dependent in &node.dependents {
-            self.mark_as_waiting_from(&self.nodes[dependent.get()]);
-        }
+        self.mark_neighbors_as_waiting_from(node);
     }
 
     /// Compresses the vector, removing all popped nodes. This adjusts
@@ -532,28 +543,28 @@ impl<O: ForestObligation> ObligationForest<O> {
         //     self.nodes[i..] are unchanged
         for i in 0..self.nodes.len() {
             match self.nodes[i].state.get() {
+                NodeState::Pending | NodeState::Waiting => {
+                    if dead_nodes > 0 {
+                        self.nodes.swap(i, i - dead_nodes);
+                        node_rewrites[i] -= dead_nodes;
+                    }
+                }
                 NodeState::Done => {
                     self.waiting_cache.remove(self.nodes[i].obligation.as_predicate());
                     // FIXME(HashMap): why can't I get my key back?
                     self.done_cache.insert(self.nodes[i].obligation.as_predicate().clone());
+                    node_rewrites[i] = nodes_len;
+                    dead_nodes += 1;
                 }
                 NodeState::Error => {
                     // We *intentionally* remove the node from the cache at this point. Otherwise
                     // tests must come up with a different type on every type error they
                     // check against.
                     self.waiting_cache.remove(self.nodes[i].obligation.as_predicate());
+                    node_rewrites[i] = nodes_len;
+                    dead_nodes += 1;
                 }
-                _ => {}
-            }
-
-            if self.nodes[i].is_popped() {
-                node_rewrites[i] = nodes_len;
-                dead_nodes += 1;
-            } else {
-                if dead_nodes > 0 {
-                    self.nodes.swap(i, i - dead_nodes);
-                    node_rewrites[i] -= dead_nodes;
-                }
+                NodeState::OnDfsStack | NodeState::Success => unreachable!()
             }
         }
 
@@ -633,12 +644,4 @@ impl<O> Node<O> {
             dependents: vec![],
         }
     }
-
-    fn is_popped(&self) -> bool {
-        match self.state.get() {
-            NodeState::Pending | NodeState::Waiting => false,
-            NodeState::Error | NodeState::Done => true,
-            NodeState::OnDfsStack | NodeState::Success => unreachable!()
-        }
-    }
 }