Auto merge of #55906 - nnethercote:rm-OpenSnapshot-CommittedSnapshot,…

… r=nikomatsakis Clean up and streamline snapshot data structures These commits clean up the snapshot structures a bit, so they are more consistent with each other and with the `ena` crate. They also remove the `OpenSnapshot` and `CommittedSnapshot` entries in the undo log, just like I did for the `ena` crate in rust-lang/ena#14. This PR in combination with that `ena` PR reduces instruction counts by up to 6% on benchmarks. r? @nikomatsakis. Note that this isn't quite ready for landing, because the `ena` dependency in the first commit needs to be updated once rust-lang/ena#14 lands. But otherwise it should be good.
rust-lang · Nov 25, 2018 · e9bca7a · e9bca7a
2 parents abe19a7 + 94967ae
commit e9bca7a
Show file tree

Hide file tree

Showing 9 changed files with 96 additions and 118 deletions.
diff --git a/Cargo.lock b/Cargo.lock
@@ -661,7 +661,7 @@ dependencies = [
 
 [[package]]
 name = "ena"
-version = "0.10.1"
+version = "0.11.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "log 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -2230,7 +2230,7 @@ name = "rustc_data_structures"
 version = "0.0.0"
 dependencies = [
  "cfg-if 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)",
- "ena 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)",
+ "ena 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "graphviz 0.0.0",
  "log 0.4.5 (registry+https://github.com/rust-lang/crates.io-index)",
  "parking_lot 0.6.4 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -3295,7 +3295,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 "checksum difference 2.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "524cbf6897b527295dff137cec09ecf3a05f4fddffd7dfcd1585403449e74198"
 "checksum either 1.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3be565ca5c557d7f59e7cfcf1844f9e3033650c929c6566f511e8005f205c1d0"
 "checksum elasticlunr-rs 2.3.3 (registry+https://github.com/rust-lang/crates.io-index)" = "4837d77a1e157489a3933b743fd774ae75074e0e390b2b7f071530048a0d87ee"
-"checksum ena 0.10.1 (registry+https://github.com/rust-lang/crates.io-index)" = "25b4e5febb25f08c49f1b07dc33a182729a6b21edfb562b5aef95f78e0dbe5bb"
+"checksum ena 0.11.0 (registry+https://github.com/rust-lang/crates.io-index)" = "f56c93cc076508c549d9bb747f79aa9b4eb098be7b8cad8830c3137ef52d1e00"
 "checksum ena 0.9.3 (registry+https://github.com/rust-lang/crates.io-index)" = "88dc8393b3c7352f94092497f6b52019643e493b6b890eb417cdb7c46117e621"
 "checksum env_logger 0.5.12 (registry+https://github.com/rust-lang/crates.io-index)" = "f4d7e69c283751083d53d01eac767407343b8b69c4bd70058e08adc2637cb257"
 "checksum env_logger 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)" = "afb070faf94c85d17d50ca44f6ad076bce18ae92f0037d350947240a36e9d42e"

diff --git a/src/librustc/infer/higher_ranked/mod.rs b/src/librustc/infer/higher_ranked/mod.rs
@@ -554,11 +554,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
     ) {
         debug!("pop_placeholders({:?})", placeholder_map);
         let placeholder_regions: FxHashSet<_> = placeholder_map.values().cloned().collect();
-        self.borrow_region_constraints()
-            .pop_placeholders(
-                &placeholder_regions,
-                &snapshot.region_constraints_snapshot,
-            );
+        self.borrow_region_constraints().pop_placeholders(&placeholder_regions);
         self.universe.set(snapshot.universe);
         if !placeholder_map.is_empty() {
             self.projection_cache.borrow_mut().rollback_placeholder(

diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs
@@ -790,7 +790,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
 
         self.projection_cache
             .borrow_mut()
-            .commit(&projection_cache_snapshot);
+            .commit(projection_cache_snapshot);
         self.type_variables.borrow_mut().commit(type_snapshot);
         self.int_unification_table.borrow_mut().commit(int_snapshot);
         self.float_unification_table

diff --git a/src/librustc/infer/region_constraints/mod.rs b/src/librustc/infer/region_constraints/mod.rs
@@ -11,7 +11,7 @@
 //! See README.md
 
 use self::CombineMapType::*;
-use self::UndoLogEntry::*;
+use self::UndoLog::*;
 
 use super::unify_key;
 use super::{MiscVariable, RegionVariableOrigin, SubregionOrigin};
@@ -52,14 +52,17 @@ pub struct RegionConstraintCollector<'tcx> {
 
     /// The undo log records actions that might later be undone.
     ///
-    /// Note: when the undo_log is empty, we are not actively
+    /// Note: `num_open_snapshots` is used to track if we are actively
     /// snapshotting. When the `start_snapshot()` method is called, we
-    /// push an OpenSnapshot entry onto the list to indicate that we
-    /// are now actively snapshotting. The reason for this is that
-    /// otherwise we end up adding entries for things like the lower
-    /// bound on a variable and so forth, which can never be rolled
-    /// back.
-    undo_log: Vec<UndoLogEntry<'tcx>>,
+    /// increment `num_open_snapshots` to indicate that we are now actively
+    /// snapshotting. The reason for this is that otherwise we end up adding
+    /// entries for things like the lower bound on a variable and so forth,
+    /// which can never be rolled back.
+    undo_log: Vec<UndoLog<'tcx>>,
+
+    /// The number of open snapshots, i.e. those that haven't been committed or
+    /// rolled back.
+    num_open_snapshots: usize,
 
     /// When we add a R1 == R2 constriant, we currently add (a) edges
     /// R1 <= R2 and R2 <= R1 and (b) we unify the two regions in this
@@ -254,15 +257,7 @@ struct TwoRegions<'tcx> {
 }
 
 #[derive(Copy, Clone, PartialEq)]
-enum UndoLogEntry<'tcx> {
-    /// Pushed when we start a snapshot.
-    OpenSnapshot,
-
-    /// Replaces an `OpenSnapshot` when a snapshot is committed, but
-    /// that snapshot is not the root. If the root snapshot is
-    /// unrolled, all nested snapshots must be committed.
-    CommitedSnapshot,
-
+enum UndoLog<'tcx> {
     /// We added `RegionVid`
     AddVar(RegionVid),
 
@@ -387,6 +382,7 @@ impl<'tcx> RegionConstraintCollector<'tcx> {
             glbs,
             bound_count: _,
             undo_log: _,
+            num_open_snapshots: _,
             unification_table,
             any_unifications,
         } = self;
@@ -415,53 +411,60 @@ impl<'tcx> RegionConstraintCollector<'tcx> {
     }
 
     fn in_snapshot(&self) -> bool {
-        !self.undo_log.is_empty()
+        self.num_open_snapshots > 0
     }
 
     pub fn start_snapshot(&mut self) -> RegionSnapshot {
         let length = self.undo_log.len();
         debug!("RegionConstraintCollector: start_snapshot({})", length);
-        self.undo_log.push(OpenSnapshot);
+        self.num_open_snapshots += 1;
         RegionSnapshot {
             length,
             region_snapshot: self.unification_table.snapshot(),
             any_unifications: self.any_unifications,
         }
     }
 
+    fn assert_open_snapshot(&self, snapshot: &RegionSnapshot) {
+        assert!(self.undo_log.len() >= snapshot.length);
+        assert!(self.num_open_snapshots > 0);
+    }
+
     pub fn commit(&mut self, snapshot: RegionSnapshot) {
         debug!("RegionConstraintCollector: commit({})", snapshot.length);
-        assert!(self.undo_log.len() > snapshot.length);
-        assert!(self.undo_log[snapshot.length] == OpenSnapshot);
+        self.assert_open_snapshot(&snapshot);
 
-        if snapshot.length == 0 {
+        if self.num_open_snapshots == 1 {
+            // The root snapshot. It's safe to clear the undo log because
+            // there's no snapshot further out that we might need to roll back
+            // to.
+            assert!(snapshot.length == 0);
             self.undo_log.clear();
-        } else {
-            (*self.undo_log)[snapshot.length] = CommitedSnapshot;
         }
+
+        self.num_open_snapshots -= 1;
+
         self.unification_table.commit(snapshot.region_snapshot);
     }
 
     pub fn rollback_to(&mut self, snapshot: RegionSnapshot) {
         debug!("RegionConstraintCollector: rollback_to({:?})", snapshot);
-        assert!(self.undo_log.len() > snapshot.length);
-        assert!(self.undo_log[snapshot.length] == OpenSnapshot);
-        while self.undo_log.len() > snapshot.length + 1 {
+        self.assert_open_snapshot(&snapshot);
+
+        while self.undo_log.len() > snapshot.length {
             let undo_entry = self.undo_log.pop().unwrap();
             self.rollback_undo_entry(undo_entry);
         }
-        let c = self.undo_log.pop().unwrap();
-        assert!(c == OpenSnapshot);
+
+        self.num_open_snapshots -= 1;
+
         self.unification_table.rollback_to(snapshot.region_snapshot);
         self.any_unifications = snapshot.any_unifications;
     }
 
-    fn rollback_undo_entry(&mut self, undo_entry: UndoLogEntry<'tcx>) {
+    fn rollback_undo_entry(&mut self, undo_entry: UndoLog<'tcx>) {
         match undo_entry {
-            OpenSnapshot => {
-                panic!("Failure to observe stack discipline");
-            }
-            Purged | CommitedSnapshot => {
+            Purged => {
                 // nothing to do here
             }
             AddVar(vid) => {
@@ -521,15 +524,10 @@ impl<'tcx> RegionConstraintCollector<'tcx> {
     /// in `skols`. This is used after a higher-ranked operation
     /// completes to remove all trace of the placeholder regions
     /// created in that time.
-    pub fn pop_placeholders(
-        &mut self,
-        placeholders: &FxHashSet<ty::Region<'tcx>>,
-        snapshot: &RegionSnapshot,
-    ) {
+    pub fn pop_placeholders(&mut self, placeholders: &FxHashSet<ty::Region<'tcx>>) {
         debug!("pop_placeholders(placeholders={:?})", placeholders);
 
         assert!(self.in_snapshot());
-        assert!(self.undo_log[snapshot.length] == OpenSnapshot);
 
         let constraints_to_kill: Vec<usize> = self.undo_log
             .iter()
@@ -548,7 +546,7 @@ impl<'tcx> RegionConstraintCollector<'tcx> {
 
         fn kill_constraint<'tcx>(
             placeholders: &FxHashSet<ty::Region<'tcx>>,
-            undo_entry: &UndoLogEntry<'tcx>,
+            undo_entry: &UndoLog<'tcx>,
         ) -> bool {
             match undo_entry {
                 &AddConstraint(Constraint::VarSubVar(..)) => false,
@@ -562,7 +560,7 @@ impl<'tcx> RegionConstraintCollector<'tcx> {
                 &AddCombination(_, ref two_regions) => {
                     placeholders.contains(&two_regions.a) || placeholders.contains(&two_regions.b)
                 }
-                &AddVar(..) | &OpenSnapshot | &Purged | &CommitedSnapshot => false,
+                &AddVar(..) | &Purged => false,
             }
         }
     }

diff --git a/src/librustc/infer/region_constraints/taint.rs b/src/librustc/infer/region_constraints/taint.rs
@@ -29,7 +29,7 @@ impl<'tcx> TaintSet<'tcx> {
     pub(super) fn fixed_point(
         &mut self,
         tcx: TyCtxt<'_, '_, 'tcx>,
-        undo_log: &[UndoLogEntry<'tcx>],
+        undo_log: &[UndoLog<'tcx>],
         verifys: &[Verify<'tcx>],
     ) {
         let mut prev_len = 0;
@@ -65,8 +65,7 @@ impl<'tcx> TaintSet<'tcx> {
                             "we never add verifications while doing higher-ranked things",
                         )
                     }
-                    &Purged | &AddCombination(..) | &AddVar(..) | &OpenSnapshot
-                    | &CommitedSnapshot => {}
+                    &Purged | &AddCombination(..) | &AddVar(..) => {}
                 }
             }
         }

diff --git a/src/librustc/traits/project.rs b/src/librustc/traits/project.rs
@@ -1652,15 +1652,15 @@ impl<'tcx> ProjectionCache<'tcx> {
     }
 
     pub fn rollback_to(&mut self, snapshot: ProjectionCacheSnapshot) {
-        self.map.rollback_to(&snapshot.snapshot);
+        self.map.rollback_to(snapshot.snapshot);
     }
 
     pub fn rollback_placeholder(&mut self, snapshot: &ProjectionCacheSnapshot) {
         self.map.partial_rollback(&snapshot.snapshot, &|k| k.ty.has_re_placeholders());
     }
 
-    pub fn commit(&mut self, snapshot: &ProjectionCacheSnapshot) {
-        self.map.commit(&snapshot.snapshot);
+    pub fn commit(&mut self, snapshot: ProjectionCacheSnapshot) {
+        self.map.commit(snapshot.snapshot);
     }
 
     /// Try to start normalize `key`; returns an error if
@@ -1714,12 +1714,8 @@ impl<'tcx> ProjectionCache<'tcx> {
     /// to be a NormalizedTy.
     pub fn complete_normalized(&mut self, key: ProjectionCacheKey<'tcx>, ty: &NormalizedTy<'tcx>) {
         // We want to insert `ty` with no obligations. If the existing value
-        // already has no obligations (as is common) we can use `insert_noop`
-        // to do a minimal amount of work -- the HashMap insertion is skipped,
-        // and minimal changes are made to the undo log.
-        if ty.obligations.is_empty() {
-            self.map.insert_noop();
-        } else {
+        // already has no obligations (as is common) we don't insert anything.
+        if !ty.obligations.is_empty() {
             self.map.insert(key, ProjectionCacheEntry::NormalizedTy(Normalized {
                 value: ty.value,
                 obligations: vec![]

diff --git a/src/librustc_data_structures/Cargo.toml b/src/librustc_data_structures/Cargo.toml
@@ -9,7 +9,7 @@ path = "lib.rs"
 crate-type = ["dylib"]
 
 [dependencies]
-ena = "0.10.1"
+ena = "0.11"
 log = "0.4"
 rustc_cratesio_shim = { path = "../librustc_cratesio_shim" }
 serialize = { path = "../libserialize" }