Auto merge of #51394 - nnethercote:NCA-depths, r=nikomatsakis

Use scope tree depths to speed up `nearest_common_ancestor`.

This patch adds depth markings to all entries in the `ScopeTree`'s
`parent_map`. This change increases memory usage somewhat, but permits a
much faster algorithm to be used:

- If one scope has a greater depth than the other, the deeper scope is
  moved upward until they are at equal depths.

- Then we move the two scopes upward in lockstep until they match.

This avoids the need to keep track of which scopes have already been
seen, which was the major part of the cost of the old algorithm. It also
reduces the number of child-to-parent moves (which are hash table
lookups) when the scopes start at different levels, because it never
goes past the nearest common ancestor the way the old algorithm did.

Finally, the case where one of the scopes is the root is now handled in
advance, because that is moderately common and lets us skip everything.

This change speeds up runs of several rust-perf benchmarks, the best by
6%.

A selection of the bigger improvements:
```
clap-rs-check
        avg: -2.6%      min: -6.6%      max: 0.0%
syn-check
        avg: -2.2%      min: -5.0%      max: 0.0%
style-servo-check
        avg: -2.9%?     min: -4.8%?     max: 0.0%?
cargo-check
        avg: -1.3%      min: -2.8%      max: 0.0%
sentry-cli-check
        avg: -1.0%      min: -2.1%      max: 0.0%
webrender-check
        avg: -0.9%      min: -2.0%      max: 0.0%
style-servo
        avg: -0.9%?     min: -1.8%?     max: -0.0%?
ripgrep-check
        avg: -0.7%      min: -1.8%      max: 0.1%
clap-rs
        avg: -0.9%      min: -1.6%      max: -0.2%
regex-check
        avg: -0.2%      min: -1.3%      max: 0.1%
syn
        avg: -0.6%      min: -1.3%      max: 0.1%
hyper-check
        avg: -0.5%      min: -1.1%      max: 0.0%
```
The idea came from multiple commenters on my blog and on Reddit. Thank you!

r? @nikomatsakis

Author: bors

src/librustc/middle/region.rs

@@ -22,7 +22,6 @@ use ty;
 
 use std::fmt;
 use std::mem;
-use rustc_data_structures::small_vec::SmallVec;
 use rustc_data_structures::sync::Lrc;
 use syntax::codemap;
 use syntax::ast;
@@ -280,6 +279,8 @@ impl Scope {
     }
 }
 
+pub type ScopeDepth = u32;
+
 /// The region scope tree encodes information about region relationships.
 #[derive(Default, Debug)]
 pub struct ScopeTree {
@@ -297,7 +298,7 @@ pub struct ScopeTree {
     /// conditional expression or repeating block. (Note that the
     /// enclosing scope id for the block associated with a closure is
     /// the closure itself.)
-    parent_map: FxHashMap<Scope, Scope>,
+    parent_map: FxHashMap<Scope, (Scope, ScopeDepth)>,
 
     /// `var_map` maps from a variable or binding id to the block in
     /// which that variable is declared.
@@ -415,11 +416,12 @@ pub struct Context {
     /// details.
     root_id: Option<hir::ItemLocalId>,
 
-    /// the scope that contains any new variables declared
-    var_parent: Option<Scope>,
+    /// The scope that contains any new variables declared, plus its depth in
+    /// the scope tree.
+    var_parent: Option<(Scope, ScopeDepth)>,
 
-    /// region parent of expressions etc
-    parent: Option<Scope>,
+    /// Region parent of expressions, etc., plus its depth in the scope tree.
+    parent: Option<(Scope, ScopeDepth)>,
 }
 
 struct RegionResolutionVisitor<'a, 'tcx: 'a> {
@@ -499,7 +501,7 @@ impl<'tcx> Visitor<'tcx> for ExprLocatorVisitor {
 }
 
 impl<'tcx> ScopeTree {
-    pub fn record_scope_parent(&mut self, child: Scope, parent: Option<Scope>) {
+    pub fn record_scope_parent(&mut self, child: Scope, parent: Option<(Scope, ScopeDepth)>) {
         debug!("{:?}.parent = {:?}", child, parent);
 
         if let Some(p) = parent {
@@ -515,7 +517,7 @@ impl<'tcx> ScopeTree {
 
     pub fn each_encl_scope<E>(&self, mut e:E) where E: FnMut(Scope, Scope) {
         for (&child, &parent) in &self.parent_map {
-            e(child, parent)
+            e(child, parent.0)
         }
     }
 
@@ -558,7 +560,7 @@ impl<'tcx> ScopeTree {
 
     pub fn opt_encl_scope(&self, id: Scope) -> Option<Scope> {
         //! Returns the narrowest scope that encloses `id`, if any.
-        self.parent_map.get(&id).cloned()
+        self.parent_map.get(&id).cloned().map(|(p, _)| p)
     }
 
     #[allow(dead_code)] // used in cfg
@@ -590,7 +592,7 @@ impl<'tcx> ScopeTree {
         // returned.
         let mut id = Scope::Node(expr_id);
 
-        while let Some(&p) = self.parent_map.get(&id) {
+        while let Some(&(p, _)) = self.parent_map.get(&id) {
             match p.data() {
                 ScopeData::Destruction(..) => {
                     debug!("temporary_scope({:?}) = {:?} [enclosing]",
@@ -658,57 +660,61 @@ impl<'tcx> ScopeTree {
         }
     }
 
-    /// Finds the nearest common ancestor (if any) of two scopes.  That is, finds the smallest
-    /// scope which is greater than or equal to both `scope_a` and `scope_b`.
-    pub fn nearest_common_ancestor(&self,
-                                   scope_a: Scope,
-                                   scope_b: Scope)
-                                   -> Scope {
+    /// Finds the nearest common ancestor of two scopes.  That is, finds the
+    /// smallest scope which is greater than or equal to both `scope_a` and
+    /// `scope_b`.
+    pub fn nearest_common_ancestor(&self, scope_a: Scope, scope_b: Scope) -> Scope {
         if scope_a == scope_b { return scope_a; }
 
-        // Process the lists in tandem from the innermost scope, recording the
-        // scopes seen so far. The first scope that comes up for a second time
-        // is the nearest common ancestor.
-        //
-        // Note: another way to compute the nearest common ancestor is to get
-        // the full scope chain for both scopes and then compare the chains to
-        // find the first scope in a common tail. But getting a parent scope
-        // requires a hash table lookup, and we often have very long scope
-        // chains (10s or 100s of scopes) that only differ by a few elements at
-        // the start. So this algorithm is faster.
-
-        let mut ma = Some(&scope_a);
-        let mut mb = Some(&scope_b);
-
-        // A HashSet<Scope> is a more obvious choice for these, but SmallVec is
-        // faster because the set size is normally small so linear search is
-        // as good or better than a hash table lookup, plus the size is usually
-        // small enough to avoid a heap allocation.
-        let mut seen_a: SmallVec<[&Scope; 32]> = SmallVec::new();
-        let mut seen_b: SmallVec<[&Scope; 32]> = SmallVec::new();
+        let mut a = scope_a;
+        let mut b = scope_b;
 
-        loop {
-            if let Some(a) = ma {
-                if seen_b.iter().any(|s| *s == a) {
-                    return *a;
-                }
-                seen_a.push(a);
-                ma = self.parent_map.get(&a);
-            }
+        // Get the depth of each scope's parent. If either scope has no parent,
+        // it must be the root, which means we can stop immediately because the
+        // root must be the nearest common ancestor. (In practice, this is
+        // moderately common.)
+        let (parent_a, parent_a_depth) = match self.parent_map.get(&a) {
+            Some(pd) => *pd,
+            None => return a,
+        };
+        let (parent_b, parent_b_depth) = match self.parent_map.get(&b) {
+            Some(pd) => *pd,
+            None => return b,
+        };
 
-            if let Some(b) = mb {
-                if seen_a.iter().any(|s| *s == b) {
-                    return *b;
-                }
-                seen_b.push(b);
-                mb = self.parent_map.get(&b);
+        if parent_a_depth > parent_b_depth {
+            // `a` is lower than `b`. Move `a` up until it's at the same depth
+            // as `b`. The first move up is trivial because we already found
+            // `parent_a` above; the loop does the remaining N-1 moves.
+            a = parent_a;
+            for _ in 0..(parent_a_depth - parent_b_depth - 1) {
+                a = self.parent_map.get(&a).unwrap().0;
             }
-
-            if ma.is_none() && mb.is_none() {
-                // No nearest common ancestor found.
-                bug!();
+        } else if parent_b_depth > parent_a_depth {
+            // `b` is lower than `a`.
+            b = parent_b;
+            for _ in 0..(parent_b_depth - parent_a_depth - 1) {
+                b = self.parent_map.get(&b).unwrap().0;
             }
+        } else {
+            // Both scopes are at the same depth, and we know they're not equal
+            // because that case was tested for at the top of this function. So
+            // we can trivially move them both up one level now.
+            assert!(parent_a_depth != 0);
+            a = parent_a;
+            b = parent_b;
         }
+
+        // Now both scopes are at the same level. We move upwards in lockstep
+        // until they match. In practice, this loop is almost always executed
+        // zero times because `a` is almost always a direct ancestor of `b` or
+        // vice versa.
+        while a != b {
+            a = self.parent_map.get(&a).unwrap().0;
+            b = self.parent_map.get(&b).unwrap().0;
+        };
+
+        a
     }
 
     /// Assuming that the provided region was defined within this `ScopeTree`,
@@ -807,7 +813,7 @@ fn record_var_lifetime(visitor: &mut RegionResolutionVisitor,
             //
             // extern fn isalnum(c: c_int) -> c_int
         }
-        Some(parent_scope) =>
+        Some((parent_scope, _)) =>
             visitor.scope_tree.record_var_scope(var_id, parent_scope),
     }
 }
@@ -1019,7 +1025,7 @@ fn resolve_expr<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, expr:
             // Keep traversing up while we can.
             match visitor.scope_tree.parent_map.get(&scope) {
                 // Don't cross from closure bodies to their parent.
-                Some(&superscope) => match superscope.data() {
+                Some(&(superscope, _)) => match superscope.data() {
                     ScopeData::CallSite(_) => break,
                     _ => scope = superscope
                 },
@@ -1036,7 +1042,7 @@ fn resolve_local<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
                            init: Option<&'tcx hir::Expr>) {
     debug!("resolve_local(pat={:?}, init={:?})", pat, init);
 
-    let blk_scope = visitor.cx.var_parent;
+    let blk_scope = visitor.cx.var_parent.map(|(p, _)| p);
 
     // As an exception to the normal rules governing temporary
     // lifetimes, initializers in a let have a temporary lifetime
@@ -1261,16 +1267,20 @@ fn resolve_local<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
 
 impl<'a, 'tcx> RegionResolutionVisitor<'a, 'tcx> {
     /// Records the current parent (if any) as the parent of `child_scope`.
-    fn record_child_scope(&mut self, child_scope: Scope) {
+    /// Returns the depth of `child_scope`.
+    fn record_child_scope(&mut self, child_scope: Scope) -> ScopeDepth {
         let parent = self.cx.parent;
         self.scope_tree.record_scope_parent(child_scope, parent);
+        // If `child_scope` has no parent, it must be the root node, and so has
+        // a depth of 1. Otherwise, its depth is one more than its parent's.
+        parent.map_or(1, |(_p, d)| d + 1)
     }
 
     /// Records the current parent (if any) as the parent of `child_scope`,
     /// and sets `child_scope` as the new current parent.
     fn enter_scope(&mut self, child_scope: Scope) {
-        self.record_child_scope(child_scope);
-        self.cx.parent = Some(child_scope);
+        let child_depth = self.record_child_scope(child_scope);
+        self.cx.parent = Some((child_scope, child_depth));
     }
 
     fn enter_node_scope_with_dtor(&mut self, id: hir::ItemLocalId) {

src/librustc_driver/test.rs

@@ -191,11 +191,12 @@ impl<'a, 'gcx, 'tcx> Env<'a, 'gcx, 'tcx> {
         self.infcx.tcx
     }
 
-    pub fn create_region_hierarchy(&mut self, rh: &RH, parent: region::Scope) {
+    pub fn create_region_hierarchy(&mut self, rh: &RH,
+                                   parent: (region::Scope, region::ScopeDepth)) {
         let me = region::Scope::Node(rh.id);
         self.region_scope_tree.record_scope_parent(me, Some(parent));
         for child_rh in rh.sub {
-            self.create_region_hierarchy(child_rh, me);
+            self.create_region_hierarchy(child_rh, (me, parent.1 + 1));
         }
     }
 
@@ -215,7 +216,7 @@ impl<'a, 'gcx, 'tcx> Env<'a, 'gcx, 'tcx> {
                 id: hir::ItemLocalId(11),
                 sub: &[],
             }],
-        }, dscope);
+        }, (dscope, 1));
     }
 
     #[allow(dead_code)] // this seems like it could be useful, even if we don't use it now