From 1e34dfce6f49b5bef0da31a8abe34e753c3af513 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 2 Nov 2018 19:46:59 +1100 Subject: [PATCH 1/8] Update to `ena` 0.11.0. This version has some significant speed-ups relating to snapshotting. --- Cargo.lock | 6 +++--- src/librustc_data_structures/Cargo.toml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dc9296b81e223..a7b83f87b191b 100644 --- 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_data_structures/Cargo.toml b/src/librustc_data_structures/Cargo.toml index 5b3dd38adf23f..188919d063351 100644 --- 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" } From 9847b5cfcb16876630064ed766fbbb7545c67368 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Nov 2018 09:28:12 +1100 Subject: [PATCH 2/8] Remove `insert_noop`. Because it's as useless as its name suggests. This commit also renames `UndoLog::Noop` as `UndoLog::Purged`, because (a) that's a more descriptive name and (b) it matches the name used in similar code in `librustc/infer/region_constraints/mod.rs`. --- src/librustc/traits/project.rs | 8 ++------ src/librustc_data_structures/snapshot_map/mod.rs | 14 ++++---------- 2 files changed, 6 insertions(+), 16 deletions(-) diff --git a/src/librustc/traits/project.rs b/src/librustc/traits/project.rs index b59bd0e238873..e44bf2661c55c 100644 --- a/src/librustc/traits/project.rs +++ b/src/librustc/traits/project.rs @@ -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/snapshot_map/mod.rs b/src/librustc_data_structures/snapshot_map/mod.rs index 0b42cb1edddec..2c36a549baca1 100644 --- a/src/librustc_data_structures/snapshot_map/mod.rs +++ b/src/librustc_data_structures/snapshot_map/mod.rs @@ -44,7 +44,7 @@ enum UndoLog { CommittedSnapshot, Inserted(K), Overwrite(K, V), - Noop, + Purged, } impl SnapshotMap @@ -72,12 +72,6 @@ impl SnapshotMap } } - pub fn insert_noop(&mut self) { - if !self.undo_log.is_empty() { - self.undo_log.push(UndoLog::Noop); - } - } - pub fn remove(&mut self, key: K) -> bool { match self.map.remove(&key) { Some(old_value) => { @@ -128,13 +122,13 @@ impl SnapshotMap let reverse = match self.undo_log[i] { UndoLog::OpenSnapshot => false, UndoLog::CommittedSnapshot => false, - UndoLog::Noop => false, + UndoLog::Purged => false, UndoLog::Inserted(ref k) => should_revert_key(k), UndoLog::Overwrite(ref k, _) => should_revert_key(k), }; if reverse { - let entry = mem::replace(&mut self.undo_log[i], UndoLog::Noop); + let entry = mem::replace(&mut self.undo_log[i], UndoLog::Purged); self.reverse(entry); } } @@ -171,7 +165,7 @@ impl SnapshotMap self.map.insert(key, old_value); } - UndoLog::Noop => {} + UndoLog::Purged => {} } } } From c86bbd4830410f19812ddbe4286ebb83ae043a4b Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Nov 2018 09:34:37 +1100 Subject: [PATCH 3/8] Rename `UndoLogEntry` as `UndoLog`. So that it matches `librustc_data_structures/snapshot_map/mod.rs` and the `ena` crate. --- src/librustc/infer/region_constraints/mod.rs | 10 +++++----- src/librustc/infer/region_constraints/taint.rs | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/librustc/infer/region_constraints/mod.rs b/src/librustc/infer/region_constraints/mod.rs index 391bfc428c3bb..eb4c0d08f9fa1 100644 --- 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}; @@ -59,7 +59,7 @@ pub struct RegionConstraintCollector<'tcx> { /// 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>, + undo_log: Vec>, /// 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,7 +254,7 @@ struct TwoRegions<'tcx> { } #[derive(Copy, Clone, PartialEq)] -enum UndoLogEntry<'tcx> { +enum UndoLog<'tcx> { /// Pushed when we start a snapshot. OpenSnapshot, @@ -456,7 +456,7 @@ impl<'tcx> RegionConstraintCollector<'tcx> { 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"); @@ -548,7 +548,7 @@ impl<'tcx> RegionConstraintCollector<'tcx> { fn kill_constraint<'tcx>( placeholders: &FxHashSet>, - undo_entry: &UndoLogEntry<'tcx>, + undo_entry: &UndoLog<'tcx>, ) -> bool { match undo_entry { &AddConstraint(Constraint::VarSubVar(..)) => false, diff --git a/src/librustc/infer/region_constraints/taint.rs b/src/librustc/infer/region_constraints/taint.rs index ef7365276f6d2..9f08fdcad7eea 100644 --- 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; From 7fe09a6551b72133e68911f3125f28642ec243ac Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Nov 2018 09:35:28 +1100 Subject: [PATCH 4/8] Replace a `.truncate(0)` call with `.clear()`. --- src/librustc_data_structures/snapshot_map/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc_data_structures/snapshot_map/mod.rs b/src/librustc_data_structures/snapshot_map/mod.rs index 2c36a549baca1..6ad886625d8b5 100644 --- a/src/librustc_data_structures/snapshot_map/mod.rs +++ b/src/librustc_data_structures/snapshot_map/mod.rs @@ -106,7 +106,7 @@ impl SnapshotMap self.assert_open_snapshot(snapshot); if snapshot.len == 0 { // The root snapshot. - self.undo_log.truncate(0); + self.undo_log.clear(); } else { self.undo_log[snapshot.len] = UndoLog::CommittedSnapshot; } From f5624e41e819bb85408eba7b3abd550f61a2857a Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Nov 2018 10:37:23 +1100 Subject: [PATCH 5/8] Make `commit` and `rollback_to` methods take ownership of the snapshots. Because they shouldn't be reused. This provides consistency with the `ena` crate. --- src/librustc/infer/mod.rs | 2 +- src/librustc/traits/project.rs | 6 +++--- src/librustc_data_structures/snapshot_map/mod.rs | 8 ++++---- src/librustc_data_structures/snapshot_map/test.rs | 8 ++++---- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/librustc/infer/mod.rs b/src/librustc/infer/mod.rs index 87e32be1a1759..9e51a4c16e5cb 100644 --- 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/traits/project.rs b/src/librustc/traits/project.rs index e44bf2661c55c..8f165863b3bab 100644 --- 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_skol()); } - 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 diff --git a/src/librustc_data_structures/snapshot_map/mod.rs b/src/librustc_data_structures/snapshot_map/mod.rs index 6ad886625d8b5..8b761bdc8c622 100644 --- a/src/librustc_data_structures/snapshot_map/mod.rs +++ b/src/librustc_data_structures/snapshot_map/mod.rs @@ -102,8 +102,8 @@ impl SnapshotMap }); } - pub fn commit(&mut self, snapshot: &Snapshot) { - self.assert_open_snapshot(snapshot); + pub fn commit(&mut self, snapshot: Snapshot) { + self.assert_open_snapshot(&snapshot); if snapshot.len == 0 { // The root snapshot. self.undo_log.clear(); @@ -134,8 +134,8 @@ impl SnapshotMap } } - pub fn rollback_to(&mut self, snapshot: &Snapshot) { - self.assert_open_snapshot(snapshot); + pub fn rollback_to(&mut self, snapshot: Snapshot) { + self.assert_open_snapshot(&snapshot); while self.undo_log.len() > snapshot.len + 1 { let entry = self.undo_log.pop().unwrap(); self.reverse(entry); diff --git a/src/librustc_data_structures/snapshot_map/test.rs b/src/librustc_data_structures/snapshot_map/test.rs index 700f9c95e3b57..67be806261b74 100644 --- a/src/librustc_data_structures/snapshot_map/test.rs +++ b/src/librustc_data_structures/snapshot_map/test.rs @@ -20,7 +20,7 @@ fn basic() { map.insert(44, "fourty-four"); assert_eq!(map[&44], "fourty-four"); assert_eq!(map.get(&33), None); - map.rollback_to(&snapshot); + map.rollback_to(snapshot); assert_eq!(map[&22], "twenty-two"); assert_eq!(map.get(&33), None); assert_eq!(map.get(&44), None); @@ -33,7 +33,7 @@ fn out_of_order() { map.insert(22, "twenty-two"); let snapshot1 = map.snapshot(); let _snapshot2 = map.snapshot(); - map.rollback_to(&snapshot1); + map.rollback_to(snapshot1); } #[test] @@ -43,8 +43,8 @@ fn nested_commit_then_rollback() { let snapshot1 = map.snapshot(); let snapshot2 = map.snapshot(); map.insert(22, "thirty-three"); - map.commit(&snapshot2); + map.commit(snapshot2); assert_eq!(map[&22], "thirty-three"); - map.rollback_to(&snapshot1); + map.rollback_to(snapshot1); assert_eq!(map[&22], "twenty-two"); } From f23c969492fe5aeb2e33316032387175020d2672 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Nov 2018 10:44:52 +1100 Subject: [PATCH 6/8] Introduce `in_snapshot` and `assert_open_snapshot` methods. This makes the two snapshot implementations more consistent with each other and with crate `ena`. --- src/librustc/infer/region_constraints/mod.rs | 11 +++++++---- src/librustc_data_structures/snapshot_map/mod.rs | 10 +++++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/librustc/infer/region_constraints/mod.rs b/src/librustc/infer/region_constraints/mod.rs index eb4c0d08f9fa1..231eff9ff9fd7 100644 --- a/src/librustc/infer/region_constraints/mod.rs +++ b/src/librustc/infer/region_constraints/mod.rs @@ -429,10 +429,14 @@ impl<'tcx> RegionConstraintCollector<'tcx> { } } - pub fn commit(&mut self, snapshot: RegionSnapshot) { - debug!("RegionConstraintCollector: commit({})", snapshot.length); + fn assert_open_snapshot(&self, snapshot: &RegionSnapshot) { assert!(self.undo_log.len() > snapshot.length); assert!(self.undo_log[snapshot.length] == OpenSnapshot); + } + + pub fn commit(&mut self, snapshot: RegionSnapshot) { + debug!("RegionConstraintCollector: commit({})", snapshot.length); + self.assert_open_snapshot(&snapshot); if snapshot.length == 0 { self.undo_log.clear(); @@ -444,8 +448,7 @@ impl<'tcx> RegionConstraintCollector<'tcx> { 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); + self.assert_open_snapshot(&snapshot); while self.undo_log.len() > snapshot.length + 1 { let undo_entry = self.undo_log.pop().unwrap(); self.rollback_undo_entry(undo_entry); diff --git a/src/librustc_data_structures/snapshot_map/mod.rs b/src/librustc_data_structures/snapshot_map/mod.rs index 8b761bdc8c622..a480499a30c3c 100644 --- a/src/librustc_data_structures/snapshot_map/mod.rs +++ b/src/librustc_data_structures/snapshot_map/mod.rs @@ -55,16 +55,20 @@ impl SnapshotMap self.undo_log.clear(); } + fn in_snapshot(&self) -> bool { + !self.undo_log.is_empty() + } + pub fn insert(&mut self, key: K, value: V) -> bool { match self.map.insert(key.clone(), value) { None => { - if !self.undo_log.is_empty() { + if self.in_snapshot() { self.undo_log.push(UndoLog::Inserted(key)); } true } Some(old_value) => { - if !self.undo_log.is_empty() { + if self.in_snapshot() { self.undo_log.push(UndoLog::Overwrite(key, old_value)); } false @@ -75,7 +79,7 @@ impl SnapshotMap pub fn remove(&mut self, key: K) -> bool { match self.map.remove(&key) { Some(old_value) => { - if !self.undo_log.is_empty() { + if self.in_snapshot() { self.undo_log.push(UndoLog::Overwrite(key, old_value)); } true From 2d68fa07bff2b417094024597790d30acd501ab3 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Nov 2018 10:59:33 +1100 Subject: [PATCH 7/8] Remove `OpenSnapshot` and `CommittedSnapshot` markers from `SnapshotMap`. They're not strictly necessary, and they result in the `Vec` being allocated even for the trivial (and common) case where a `start_snapshot` is immediately followed by a `commit` or `rollback_to`. --- .../snapshot_map/mod.rs | 48 +++++++------------ .../snapshot_map/test.rs | 11 +++-- 2 files changed, 25 insertions(+), 34 deletions(-) diff --git a/src/librustc_data_structures/snapshot_map/mod.rs b/src/librustc_data_structures/snapshot_map/mod.rs index a480499a30c3c..c256506a19d42 100644 --- a/src/librustc_data_structures/snapshot_map/mod.rs +++ b/src/librustc_data_structures/snapshot_map/mod.rs @@ -21,6 +21,7 @@ pub struct SnapshotMap { map: FxHashMap, undo_log: Vec>, + num_open_snapshots: usize, } // HACK(eddyb) manual impl avoids `Default` bounds on `K` and `V`. @@ -31,6 +32,7 @@ impl Default for SnapshotMap SnapshotMap { map: Default::default(), undo_log: Default::default(), + num_open_snapshots: 0, } } } @@ -40,8 +42,6 @@ pub struct Snapshot { } enum UndoLog { - OpenSnapshot, - CommittedSnapshot, Inserted(K), Overwrite(K, V), Purged, @@ -53,10 +53,11 @@ impl SnapshotMap pub fn clear(&mut self) { self.map.clear(); self.undo_log.clear(); + self.num_open_snapshots = 0; } fn in_snapshot(&self) -> bool { - !self.undo_log.is_empty() + self.num_open_snapshots > 0 } pub fn insert(&mut self, key: K, value: V) -> bool { @@ -93,27 +94,27 @@ impl SnapshotMap } pub fn snapshot(&mut self) -> Snapshot { - self.undo_log.push(UndoLog::OpenSnapshot); - let len = self.undo_log.len() - 1; + let len = self.undo_log.len(); + self.num_open_snapshots += 1; Snapshot { len } } fn assert_open_snapshot(&self, snapshot: &Snapshot) { - assert!(snapshot.len < self.undo_log.len()); - assert!(match self.undo_log[snapshot.len] { - UndoLog::OpenSnapshot => true, - _ => false, - }); + assert!(self.undo_log.len() >= snapshot.len); + assert!(self.num_open_snapshots > 0); } pub fn commit(&mut self, snapshot: Snapshot) { self.assert_open_snapshot(&snapshot); - if snapshot.len == 0 { - // The root snapshot. + 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.len == 0); self.undo_log.clear(); - } else { - self.undo_log[snapshot.len] = UndoLog::CommittedSnapshot; } + + self.num_open_snapshots -= 1; } pub fn partial_rollback(&mut self, @@ -122,10 +123,8 @@ impl SnapshotMap where F: Fn(&K) -> bool { self.assert_open_snapshot(snapshot); - for i in (snapshot.len + 1..self.undo_log.len()).rev() { + for i in (snapshot.len .. self.undo_log.len()).rev() { let reverse = match self.undo_log[i] { - UndoLog::OpenSnapshot => false, - UndoLog::CommittedSnapshot => false, UndoLog::Purged => false, UndoLog::Inserted(ref k) => should_revert_key(k), UndoLog::Overwrite(ref k, _) => should_revert_key(k), @@ -140,27 +139,16 @@ impl SnapshotMap pub fn rollback_to(&mut self, snapshot: Snapshot) { self.assert_open_snapshot(&snapshot); - while self.undo_log.len() > snapshot.len + 1 { + while self.undo_log.len() > snapshot.len { let entry = self.undo_log.pop().unwrap(); self.reverse(entry); } - let v = self.undo_log.pop().unwrap(); - assert!(match v { - UndoLog::OpenSnapshot => true, - _ => false, - }); - assert!(self.undo_log.len() == snapshot.len); + self.num_open_snapshots -= 1; } fn reverse(&mut self, entry: UndoLog) { match entry { - UndoLog::OpenSnapshot => { - panic!("cannot rollback an uncommitted snapshot"); - } - - UndoLog::CommittedSnapshot => {} - UndoLog::Inserted(key) => { self.map.remove(&key); } diff --git a/src/librustc_data_structures/snapshot_map/test.rs b/src/librustc_data_structures/snapshot_map/test.rs index 67be806261b74..b4ecb85fc4302 100644 --- a/src/librustc_data_structures/snapshot_map/test.rs +++ b/src/librustc_data_structures/snapshot_map/test.rs @@ -17,8 +17,8 @@ fn basic() { let snapshot = map.snapshot(); map.insert(22, "thirty-three"); assert_eq!(map[&22], "thirty-three"); - map.insert(44, "fourty-four"); - assert_eq!(map[&44], "fourty-four"); + map.insert(44, "forty-four"); + assert_eq!(map[&44], "forty-four"); assert_eq!(map.get(&33), None); map.rollback_to(snapshot); assert_eq!(map[&22], "twenty-two"); @@ -32,8 +32,11 @@ fn out_of_order() { let mut map = SnapshotMap::default(); map.insert(22, "twenty-two"); let snapshot1 = map.snapshot(); - let _snapshot2 = map.snapshot(); - map.rollback_to(snapshot1); + map.insert(33, "thirty-three"); + let snapshot2 = map.snapshot(); + map.insert(44, "forty-four"); + map.rollback_to(snapshot1); // bogus, but accepted + map.rollback_to(snapshot2); // asserts } #[test] From 94967ae8c1129d63df4446240df4fc45a57c8164 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Mon, 5 Nov 2018 12:21:54 +1100 Subject: [PATCH 8/8] Remove `OpenSnapshot` and `CommittedSnapshot` markers from `RegionConstraintCollector`. They're not strictly necessary, and they result in the `Vec` being allocated even for the trivial (and common) case where a `start_snapshot` is immediately followed by a `commit` or `rollback_to`. The commit also removes a now-unnecessary argument of `pop_placeholders()`. --- src/librustc/infer/higher_ranked/mod.rs | 6 +- src/librustc/infer/region_constraints/mod.rs | 65 +++++++++---------- .../infer/region_constraints/taint.rs | 3 +- 3 files changed, 32 insertions(+), 42 deletions(-) diff --git a/src/librustc/infer/higher_ranked/mod.rs b/src/librustc/infer/higher_ranked/mod.rs index 8172f620c3646..4e203b986dfb0 100644 --- a/src/librustc/infer/higher_ranked/mod.rs +++ b/src/librustc/infer/higher_ranked/mod.rs @@ -543,11 +543,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/region_constraints/mod.rs b/src/librustc/infer/region_constraints/mod.rs index 231eff9ff9fd7..af1b6964b8189 100644 --- a/src/librustc/infer/region_constraints/mod.rs +++ b/src/librustc/infer/region_constraints/mod.rs @@ -52,15 +52,18 @@ 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. + /// 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>, + /// 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 /// table. You can then call `opportunistic_resolve_var` early @@ -255,14 +258,6 @@ struct TwoRegions<'tcx> { #[derive(Copy, Clone, PartialEq)] enum UndoLog<'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, - /// 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,13 +411,13 @@ 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(), @@ -430,41 +426,45 @@ impl<'tcx> RegionConstraintCollector<'tcx> { } fn assert_open_snapshot(&self, snapshot: &RegionSnapshot) { - assert!(self.undo_log.len() > snapshot.length); - assert!(self.undo_log[snapshot.length] == OpenSnapshot); + 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); 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); self.assert_open_snapshot(&snapshot); - while self.undo_log.len() > snapshot.length + 1 { + + 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: UndoLog<'tcx>) { match undo_entry { - OpenSnapshot => { - panic!("Failure to observe stack discipline"); - } - Purged | CommitedSnapshot => { + Purged => { // nothing to do here } AddVar(vid) => { @@ -524,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>, - snapshot: &RegionSnapshot, - ) { + pub fn pop_placeholders(&mut self, placeholders: &FxHashSet>) { debug!("pop_placeholders(placeholders={:?})", placeholders); assert!(self.in_snapshot()); - assert!(self.undo_log[snapshot.length] == OpenSnapshot); let constraints_to_kill: Vec = self.undo_log .iter() @@ -565,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 index 9f08fdcad7eea..27ce7f106030a 100644 --- a/src/librustc/infer/region_constraints/taint.rs +++ b/src/librustc/infer/region_constraints/taint.rs @@ -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(..) => {} } } }