Auto merge of rust-lang#99702 - SparrowLii:transtive_relation, r=oli-obk

get rid of `RefCell` in `TransitiveRelation`

This is one of the jobs in `Pending refactorings` in rust-lang#48685. The parallel-compiler's work has been suspended for quite some time, but I think I can pick it up gradually. I think this PR should be a start.

Regarding the refactoring of `TransitiveRelation`, `@nikomatsakis`  has proposed [two(three?) schemes](rust-lang#48587 (comment)). In order to satisfy both compilation efficiency and robustness, I think adding the `freeze` method may be the best solution, although it requires relatively more code changes.

Author: bors

Diff for: compiler/rustc_borrowck/src/type_check/free_region_relations.rs

@@ -1,5 +1,5 @@
 use rustc_data_structures::frozen::Frozen;
-use rustc_data_structures::transitive_relation::TransitiveRelation;
+use rustc_data_structures::transitive_relation::{TransitiveRelation, TransitiveRelationBuilder};
 use rustc_infer::infer::canonical::QueryRegionConstraints;
 use rustc_infer::infer::outlives;
 use rustc_infer::infer::outlives::env::RegionBoundPairs;
@@ -61,25 +61,13 @@ pub(crate) fn create<'tcx>(
         constraints,
         universal_regions: universal_regions.clone(),
         region_bound_pairs: Default::default(),
-        relations: UniversalRegionRelations {
-            universal_regions: universal_regions.clone(),
-            outlives: Default::default(),
-            inverse_outlives: Default::default(),
-        },
+        outlives: Default::default(),
+        inverse_outlives: Default::default(),
     }
     .create()
 }
 
 impl UniversalRegionRelations<'_> {
-    /// Records in the `outlives_relation` (and
-    /// `inverse_outlives_relation`) that `fr_a: fr_b`. Invoked by the
-    /// builder below.
-    fn relate_universal_regions(&mut self, fr_a: RegionVid, fr_b: RegionVid) {
-        debug!("relate_universal_regions: fr_a={:?} outlives fr_b={:?}", fr_a, fr_b);
-        self.outlives.add(fr_a, fr_b);
-        self.inverse_outlives.add(fr_b, fr_a);
-    }
-
     /// Given two universal regions, returns the postdominating
     /// upper-bound (effectively the least upper bound).
     ///
@@ -216,11 +204,20 @@ struct UniversalRegionRelationsBuilder<'this, 'tcx> {
     constraints: &'this mut MirTypeckRegionConstraints<'tcx>,
 
     // outputs:
-    relations: UniversalRegionRelations<'tcx>,
+    outlives: TransitiveRelationBuilder<RegionVid>,
+    inverse_outlives: TransitiveRelationBuilder<RegionVid>,
     region_bound_pairs: RegionBoundPairs<'tcx>,
 }
 
 impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
+    /// Records in the `outlives_relation` (and
+    /// `inverse_outlives_relation`) that `fr_a: fr_b`.
+    fn relate_universal_regions(&mut self, fr_a: RegionVid, fr_b: RegionVid) {
+        debug!("relate_universal_regions: fr_a={:?} outlives fr_b={:?}", fr_a, fr_b);
+        self.outlives.add(fr_a, fr_b);
+        self.inverse_outlives.add(fr_b, fr_a);
+    }
+
     pub(crate) fn create(mut self) -> CreateResult<'tcx> {
         let unnormalized_input_output_tys = self
             .universal_regions
@@ -292,9 +289,9 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         let fr_fn_body = self.universal_regions.fr_fn_body;
         for fr in self.universal_regions.universal_regions() {
             debug!("build: relating free region {:?} to itself and to 'static", fr);
-            self.relations.relate_universal_regions(fr, fr);
-            self.relations.relate_universal_regions(fr_static, fr);
-            self.relations.relate_universal_regions(fr, fr_fn_body);
+            self.relate_universal_regions(fr, fr);
+            self.relate_universal_regions(fr_static, fr);
+            self.relate_universal_regions(fr, fr_fn_body);
         }
 
         for data in &constraint_sets {
@@ -313,7 +310,11 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         }
 
         CreateResult {
-            universal_region_relations: Frozen::freeze(self.relations),
+            universal_region_relations: Frozen::freeze(UniversalRegionRelations {
+                universal_regions: self.universal_regions,
+                outlives: self.outlives.freeze(),
+                inverse_outlives: self.inverse_outlives.freeze(),
+            }),
             region_bound_pairs: self.region_bound_pairs,
             normalized_inputs_and_output,
         }
@@ -356,7 +357,7 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
                     // The bound says that `r1 <= r2`; we store `r2: r1`.
                     let r1 = self.universal_regions.to_region_vid(r1);
                     let r2 = self.universal_regions.to_region_vid(r2);
-                    self.relations.relate_universal_regions(r2, r1);
+                    self.relate_universal_regions(r2, r1);
                 }
 
                 OutlivesBound::RegionSubParam(r_a, param_b) => {

Diff for: compiler/rustc_data_structures/src/transitive_relation.rs

@@ -1,45 +1,57 @@
+use crate::frozen::Frozen;
 use crate::fx::FxIndexSet;
-use crate::sync::Lock;
 use rustc_index::bit_set::BitMatrix;
 use std::fmt::Debug;
 use std::hash::Hash;
 use std::mem;
+use std::ops::Deref;
 
 #[cfg(test)]
 mod tests;
 
 #[derive(Clone, Debug)]
-pub struct TransitiveRelation<T> {
+pub struct TransitiveRelationBuilder<T> {
     // List of elements. This is used to map from a T to a usize.
     elements: FxIndexSet<T>,
 
     // List of base edges in the graph. Require to compute transitive
     // closure.
     edges: Vec<Edge>,
+}
+
+#[derive(Debug)]
+pub struct TransitiveRelation<T> {
+    // Frozen transitive relation elements and edges.
+    builder: Frozen<TransitiveRelationBuilder<T>>,
 
-    // This is a cached transitive closure derived from the edges.
-    // Currently, we build it lazily and just throw out any existing
-    // copy whenever a new edge is added. (The Lock is to permit
-    // the lazy computation.) This is kind of silly, except for the
-    // fact its size is tied to `self.elements.len()`, so I wanted to
-    // wait before building it up to avoid reallocating as new edges
-    // are added with new elements. Perhaps better would be to ask the
-    // user for a batch of edges to minimize this effect, but I
-    // already wrote the code this way. :P -nmatsakis
-    closure: Lock<Option<BitMatrix<usize, usize>>>,
+    // Cached transitive closure derived from the edges.
+    closure: Frozen<BitMatrix<usize, usize>>,
 }
 
-// HACK(eddyb) manual impl avoids `Default` bound on `T`.
-impl<T: Eq + Hash> Default for TransitiveRelation<T> {
-    fn default() -> Self {
+impl<T> Deref for TransitiveRelation<T> {
+    type Target = Frozen<TransitiveRelationBuilder<T>>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.builder
+    }
+}
+
+impl<T: Clone> Clone for TransitiveRelation<T> {
+    fn clone(&self) -> Self {
         TransitiveRelation {
-            elements: Default::default(),
-            edges: Default::default(),
-            closure: Default::default(),
+            builder: Frozen::freeze(self.builder.deref().clone()),
+            closure: Frozen::freeze(self.closure.deref().clone()),
         }
     }
 }
 
+// HACK(eddyb) manual impl avoids `Default` bound on `T`.
+impl<T: Eq + Hash> Default for TransitiveRelationBuilder<T> {
+    fn default() -> Self {
+        TransitiveRelationBuilder { elements: Default::default(), edges: Default::default() }
+    }
+}
+
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Debug)]
 struct Index(usize);
 
@@ -49,7 +61,7 @@ struct Edge {
     target: Index,
 }
 
-impl<T: Eq + Hash + Copy> TransitiveRelation<T> {
+impl<T: Eq + Hash + Copy> TransitiveRelationBuilder<T> {
     pub fn is_empty(&self) -> bool {
         self.edges.is_empty()
     }
@@ -63,23 +75,19 @@ impl<T: Eq + Hash + Copy> TransitiveRelation<T> {
     }
 
     fn add_index(&mut self, a: T) -> Index {
-        let (index, added) = self.elements.insert_full(a);
-        if added {
-            // if we changed the dimensions, clear the cache
-            *self.closure.get_mut() = None;
-        }
+        let (index, _added) = self.elements.insert_full(a);
         Index(index)
     }
 
     /// Applies the (partial) function to each edge and returns a new
-    /// relation. If `f` returns `None` for any end-point, returns
-    /// `None`.
-    pub fn maybe_map<F, U>(&self, mut f: F) -> Option<TransitiveRelation<U>>
+    /// relation builder. If `f` returns `None` for any end-point,
+    /// returns `None`.
+    pub fn maybe_map<F, U>(&self, mut f: F) -> Option<TransitiveRelationBuilder<U>>
     where
         F: FnMut(T) -> Option<U>,
         U: Clone + Debug + Eq + Hash + Copy,
     {
-        let mut result = TransitiveRelation::default();
+        let mut result = TransitiveRelationBuilder::default();
         for edge in &self.edges {
             result.add(f(self.elements[edge.source.0])?, f(self.elements[edge.target.0])?);
         }
@@ -93,10 +101,38 @@ impl<T: Eq + Hash + Copy> TransitiveRelation<T> {
         let edge = Edge { source: a, target: b };
         if !self.edges.contains(&edge) {
             self.edges.push(edge);
+        }
+    }
+
+    /// Compute the transitive closure derived from the edges, and converted to
+    /// the final result. After this, all elements will be immutable to maintain
+    /// the correctness of the result.
+    pub fn freeze(self) -> TransitiveRelation<T> {
+        let mut matrix = BitMatrix::new(self.elements.len(), self.elements.len());
+        let mut changed = true;
+        while changed {
+            changed = false;
+            for edge in &self.edges {
+                // add an edge from S -> T
+                changed |= matrix.insert(edge.source.0, edge.target.0);
 
-            // added an edge, clear the cache
-            *self.closure.get_mut() = None;
+                // add all outgoing edges from T into S
+                changed |= matrix.union_rows(edge.target.0, edge.source.0);
+            }
         }
+        TransitiveRelation { builder: Frozen::freeze(self), closure: Frozen::freeze(matrix) }
+    }
+}
+
+impl<T: Eq + Hash + Copy> TransitiveRelation<T> {
+    /// Applies the (partial) function to each edge and returns a new
+    /// relation including transitive closures.
+    pub fn maybe_map<F, U>(&self, f: F) -> Option<TransitiveRelation<U>>
+    where
+        F: FnMut(T) -> Option<U>,
+        U: Clone + Debug + Eq + Hash + Copy,
+    {
+        Some(self.builder.maybe_map(f)?.freeze())
     }
 
     /// Checks whether `a < target` (transitively)
@@ -322,30 +358,7 @@ impl<T: Eq + Hash + Copy> TransitiveRelation<T> {
     where
         OP: FnOnce(&BitMatrix<usize, usize>) -> R,
     {
-        let mut closure_cell = self.closure.borrow_mut();
-        let mut closure = closure_cell.take();
-        if closure.is_none() {
-            closure = Some(self.compute_closure());
-        }
-        let result = op(closure.as_ref().unwrap());
-        *closure_cell = closure;
-        result
-    }
-
-    fn compute_closure(&self) -> BitMatrix<usize, usize> {
-        let mut matrix = BitMatrix::new(self.elements.len(), self.elements.len());
-        let mut changed = true;
-        while changed {
-            changed = false;
-            for edge in &self.edges {
-                // add an edge from S -> T
-                changed |= matrix.insert(edge.source.0, edge.target.0);
-
-                // add all outgoing edges from T into S
-                changed |= matrix.union_rows(edge.target.0, edge.source.0);
-            }
-        }
-        matrix
+        op(&self.closure)
     }
 
     /// Lists all the base edges in the graph: the initial _non-transitive_ set of element