Various ObligationForest improvements

src/librustc_data_structures/indexed_vec.rs

@@ -149,7 +149,7 @@ macro_rules! newtype_index {
 
             #[inline]
             $v const unsafe fn from_u32_unchecked(value: u32) -> Self {
-                unsafe { $type { private: value } }
+                $type { private: value }
             }
 
             /// Extracts the value of this index as an integer.

src/librustc_data_structures/obligation_forest/graphviz.rs

@@ -74,9 +74,9 @@ impl<'a, O: ForestObligation + 'a> dot::GraphWalk<'a> for &'a ObligationForest<O
             .flat_map(|i| {
                 let node = &self.nodes[i];
 
-                node.parent.iter().map(|p| p.get())
-                    .chain(node.dependents.iter().map(|p| p.get()))
-                    .map(move |p| (p, i))
+                node.parent.iter()
+                    .chain(node.dependents.iter())
+                    .map(move |p| (p.index(), i))
             })
             .collect()
     }

src/librustc_data_structures/obligation_forest/mod.rs

@@ -81,21 +81,24 @@
 //! nodes, which aren't needed anymore.
 
 use crate::fx::{FxHashMap, FxHashSet};
+use crate::indexed_vec::Idx;
+use crate::newtype_index;
 
 use std::cell::Cell;
 use std::collections::hash_map::Entry;
 use std::fmt::Debug;
 use std::hash;
 use std::marker::PhantomData;
 
-mod node_index;
-use self::node_index::NodeIndex;
-
 mod graphviz;
 
 #[cfg(test)]
 mod tests;
 
+newtype_index! {
+    pub struct NodeIndex { .. }
+}
+
 pub trait ForestObligation : Clone + Debug {
     type Predicate : Clone + hash::Hash + Eq + Debug;
 
@@ -151,6 +154,10 @@ pub struct ObligationForest<O: ForestObligation> {
     /// At all times we maintain the invariant that every node appears
     /// at a higher index than its parent. This is needed by the
     /// backtrace iterator (which uses `split_at`).
+    ///
+    /// Ideally, this would be an `IndexVec<NodeIndex, Node<O>>`. But that is
+    /// slower, because this vector is accessed so often that the
+    /// `u32`-to-`usize` conversions required for accesses are significant.
     nodes: Vec<Node<O>>,
 
     /// A cache of predicates that have been successfully completed.
@@ -178,13 +185,19 @@ struct Node<O> {
     obligation: O,
     state: Cell<NodeState>,
 
-    /// The parent of a node - the original obligation of
-    /// which it is a subobligation. Except for error reporting,
-    /// it is just like any member of `dependents`.
+    /// The parent of a node - the original obligation of which it is a
+    /// subobligation. Except for error reporting, it is just like any member
+    /// of `dependents`.
+    ///
+    /// Unlike `ObligationForest::nodes`, this uses `NodeIndex` rather than
+    /// `usize` for the index, because keeping the size down is more important
+    /// than the cost of converting to a `usize` for indexing.
     parent: Option<NodeIndex>,
 
-    /// Obligations that depend on this obligation for their
-    /// completion. They must all be in a non-pending state.
+    /// Obligations that depend on this obligation for their completion. They
+    /// must all be in a non-pending state.
+    ///
+    /// This uses `NodeIndex` for the same reason as `parent`.
     dependents: Vec<NodeIndex>,
 
     /// Identifier of the obligation tree to which this node belongs.
@@ -294,15 +307,16 @@ impl<O: ForestObligation> ObligationForest<O> {
             Entry::Occupied(o) => {
                 debug!("register_obligation_at({:?}, {:?}) - duplicate of {:?}!",
                        obligation, parent, o.get());
-                let node = &mut self.nodes[o.get().get()];
-                if let Some(parent) = parent {
+                let node = &mut self.nodes[o.get().index()];
+                if let Some(parent_index) = parent {
                     // If the node is already in `waiting_cache`, it's already
                     // been marked with a parent. (It's possible that parent
                     // has been cleared by `apply_rewrites`, though.) So just
                     // dump `parent` into `node.dependents`... unless it's
                     // already in `node.dependents` or `node.parent`.
-                    if !node.dependents.contains(&parent) && Some(parent) != node.parent {
-                        node.dependents.push(parent);
+                    if !node.dependents.contains(&parent_index) &&
+                       Some(parent_index) != node.parent {
+                        node.dependents.push(parent_index);
                     }
                 }
                 if let NodeState::Error = node.state.get() {
@@ -316,9 +330,8 @@ impl<O: ForestObligation> ObligationForest<O> {
                        obligation, parent, self.nodes.len());
 
                 let obligation_tree_id = match parent {
-                    Some(p) => {
-                        let parent_node = &self.nodes[p.get()];
-                        parent_node.obligation_tree_id
+                    Some(parent_index) => {
+                        self.nodes[parent_index.index()].obligation_tree_id
                     }
                     None => self.obligation_tree_id_generator.next().unwrap()
                 };
@@ -346,9 +359,9 @@ impl<O: ForestObligation> ObligationForest<O> {
     /// This cannot be done during a snapshot.
     pub fn to_errors<E: Clone>(&mut self, error: E) -> Vec<Error<O, E>> {
         let mut errors = vec![];
-        for index in 0..self.nodes.len() {
-            if let NodeState::Pending = self.nodes[index].state.get() {
-                let backtrace = self.error_at(index);
+        for i in 0..self.nodes.len() {
+            if let NodeState::Pending = self.nodes[i].state.get() {
+                let backtrace = self.error_at(i);
                 errors.push(Error {
                     error: error.clone(),
                     backtrace,
@@ -393,16 +406,16 @@ impl<O: ForestObligation> ObligationForest<O> {
         let mut errors = vec![];
         let mut stalled = true;
 
-        for index in 0..self.nodes.len() {
-            debug!("process_obligations: node {} == {:?}", index, self.nodes[index]);
+        for i in 0..self.nodes.len() {
+            debug!("process_obligations: node {} == {:?}", i, self.nodes[i]);
 
-            let result = match self.nodes[index] {
+            let result = match self.nodes[i] {
                 Node { ref state, ref mut obligation, .. } if state.get() == NodeState::Pending =>
                     processor.process_obligation(obligation),
                 _ => continue
             };
 
-            debug!("process_obligations: node {} got result {:?}", index, result);
+            debug!("process_obligations: node {} got result {:?}", i, result);
 
             match result {
                 ProcessResult::Unchanged => {
@@ -411,23 +424,23 @@ impl<O: ForestObligation> ObligationForest<O> {
                 ProcessResult::Changed(children) => {
                     // We are not (yet) stalled.
                     stalled = false;
-                    self.nodes[index].state.set(NodeState::Success);
+                    self.nodes[i].state.set(NodeState::Success);
 
                     for child in children {
                         let st = self.register_obligation_at(
                             child,
-                            Some(NodeIndex::new(index))
+                            Some(NodeIndex::new(i))
                         );
                         if let Err(()) = st {
                             // error already reported - propagate it
                             // to our node.
-                            self.error_at(index);
+                            self.error_at(i);
                         }
                     }
                 }
                 ProcessResult::Error(err) => {
                     stalled = false;
-                    let backtrace = self.error_at(index);
+                    let backtrace = self.error_at(i);
                     errors.push(Error {
                         error: err,
                         backtrace,
@@ -473,15 +486,15 @@ impl<O: ForestObligation> ObligationForest<O> {
 
         debug!("process_cycles()");
 
-        for index in 0..self.nodes.len() {
+        for i 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();
+            let state = self.nodes[i].state.get();
             match state {
                 NodeState::Waiting | NodeState::Pending | NodeState::Done | NodeState::Error => {},
-                _ => self.find_cycles_from_node(&mut stack, processor, index),
+                _ => self.find_cycles_from_node(&mut stack, processor, i),
             }
         }
 
@@ -491,24 +504,22 @@ impl<O: ForestObligation> ObligationForest<O> {
         self.scratch = Some(stack);
     }
 
-    fn find_cycles_from_node<P>(&self, stack: &mut Vec<usize>,
-                                processor: &mut P, index: usize)
+    fn find_cycles_from_node<P>(&self, stack: &mut Vec<usize>, processor: &mut P, i: usize)
         where P: ObligationProcessor<Obligation=O>
     {
-        let node = &self.nodes[index];
+        let node = &self.nodes[i];
         let state = node.state.get();
         match state {
             NodeState::OnDfsStack => {
-                let index =
-                    stack.iter().rposition(|n| *n == index).unwrap();
-                processor.process_backedge(stack[index..].iter().map(GetObligation(&self.nodes)),
+                let i = stack.iter().rposition(|n| *n == i).unwrap();
+                processor.process_backedge(stack[i..].iter().map(GetObligation(&self.nodes)),
                                            PhantomData);
             }
             NodeState::Success => {
                 node.state.set(NodeState::OnDfsStack);
-                stack.push(index);
-                for dependent in node.parent.iter().chain(node.dependents.iter()) {
-                    self.find_cycles_from_node(stack, processor, dependent.get());
+                stack.push(i);
+                for index in node.parent.iter().chain(node.dependents.iter()) {
+                    self.find_cycles_from_node(stack, processor, index.index());
                 }
                 stack.pop();
                 node.state.set(NodeState::Done);
@@ -525,33 +536,32 @@ impl<O: ForestObligation> ObligationForest<O> {
 
     /// Returns a vector of obligations for `p` and all of its
     /// ancestors, putting them into the error state in the process.
-    fn error_at(&mut self, p: usize) -> Vec<O> {
+    fn error_at(&mut self, mut i: usize) -> Vec<O> {
         let mut error_stack = self.scratch.take().unwrap();
         let mut trace = vec![];
 
-        let mut n = p;
         loop {
-            self.nodes[n].state.set(NodeState::Error);
-            trace.push(self.nodes[n].obligation.clone());
-            error_stack.extend(self.nodes[n].dependents.iter().map(|x| x.get()));
+            let node = &self.nodes[i];
+            node.state.set(NodeState::Error);
+            trace.push(node.obligation.clone());
+            error_stack.extend(node.dependents.iter().map(|index| index.index()));
 
-            // loop to the parent
-            match self.nodes[n].parent {
-                Some(q) => n = q.get(),
+            // Loop to the parent.
+            match node.parent {
+                Some(parent_index) => i = parent_index.index(),
                 None => break
             }
         }
 
         while let Some(i) = error_stack.pop() {
-            match self.nodes[i].state.get() {
+            let node = &self.nodes[i];
+            match node.state.get() {
                 NodeState::Error => continue,
                 _ => self.nodes[i].state.set(NodeState::Error),
             }
 
-            let node = &self.nodes[i];
-
             error_stack.extend(
-                node.parent.iter().chain(node.dependents.iter()).map(|x| x.get())
+                node.parent.iter().chain(node.dependents.iter()).map(|index| index.index())
             );
         }
 
@@ -563,7 +573,7 @@ impl<O: ForestObligation> ObligationForest<O> {
     #[inline(always)]
     fn inlined_mark_neighbors_as_waiting_from(&self, node: &Node<O>) {
         for dependent in node.parent.iter().chain(node.dependents.iter()) {
-            self.mark_as_waiting_from(&self.nodes[dependent.get()]);
+            self.mark_as_waiting_from(&self.nodes[dependent.index()]);
         }
     }
 
@@ -689,34 +699,34 @@ impl<O: ForestObligation> ObligationForest<O> {
 
         for node in &mut self.nodes {
             if let Some(index) = node.parent {
-                let new_index = node_rewrites[index.get()];
-                if new_index >= nodes_len {
+                let new_i = node_rewrites[index.index()];
+                if new_i >= nodes_len {
                     // parent dead due to error
                     node.parent = None;
                 } else {
-                    node.parent = Some(NodeIndex::new(new_index));
+                    node.parent = Some(NodeIndex::new(new_i));
                 }
             }
 
             let mut i = 0;
             while i < node.dependents.len() {
-                let new_index = node_rewrites[node.dependents[i].get()];
-                if new_index >= nodes_len {
+                let new_i = node_rewrites[node.dependents[i].index()];
+                if new_i >= nodes_len {
                     node.dependents.swap_remove(i);
                 } else {
-                    node.dependents[i] = NodeIndex::new(new_index);
+                    node.dependents[i] = NodeIndex::new(new_i);
                     i += 1;
                 }
             }
         }
 
         let mut kill_list = vec![];
         for (predicate, index) in &mut self.waiting_cache {
-            let new_index = node_rewrites[index.get()];
-            if new_index >= nodes_len {
+            let new_i = node_rewrites[index.index()];
+            if new_i >= nodes_len {
                 kill_list.push(predicate.clone());
             } else {
-                *index = NodeIndex::new(new_index);
+                *index = NodeIndex::new(new_i);
             }
         }