From 2c162ceeb7b29729aa1178260eb9f894b669c8f1 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Fri, 2 Nov 2018 14:46:43 +1100 Subject: [PATCH] Remove `OpenSnapshot` and `CommittedSnapshot` markers. 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`. This commit removes them, and instead uses a counter to track how many snapshots are open. One consequence of this is that violations of stack discipline in API usage may be detected later -- e.g. if we rollback to an earlier snapshot and then to a later snapshot, the code will panic on the second rollback instead of the first. I think this is an acceptable trade-off for the improved performance. This commit also adds some basic unit tests. --- src/snapshot_vec.rs | 128 ++++++++++++++++++++++++++++++-------------- 1 file changed, 88 insertions(+), 40 deletions(-) diff --git a/src/snapshot_vec.rs b/src/snapshot_vec.rs index d828b02..4c1c261 100644 --- a/src/snapshot_vec.rs +++ b/src/snapshot_vec.rs @@ -27,12 +27,6 @@ use std::ops; #[derive(Debug)] pub enum UndoLog { - /// Indicates where a snapshot started. - OpenSnapshot, - - /// Indicates a snapshot that has been committed. - CommittedSnapshot, - /// New variable with given index was created. NewElem(usize), @@ -46,6 +40,7 @@ pub enum UndoLog { pub struct SnapshotVec { values: Vec, undo_log: Vec>, + num_open_snapshots: usize, } impl fmt::Debug for SnapshotVec @@ -58,6 +53,7 @@ impl fmt::Debug for SnapshotVec fmt.debug_struct("SnapshotVec") .field("values", &self.values) .field("undo_log", &self.undo_log) + .field("num_open_snapshots", &self.num_open_snapshots) .finish() } } @@ -81,6 +77,7 @@ impl Default for SnapshotVec { SnapshotVec { values: Vec::new(), undo_log: Vec::new(), + num_open_snapshots: 0, } } } @@ -94,11 +91,12 @@ impl SnapshotVec { SnapshotVec { values: Vec::with_capacity(c), undo_log: Vec::new(), + num_open_snapshots: 0, } } fn in_snapshot(&self) -> bool { - !self.undo_log.is_empty() + self.num_open_snapshots > 0 } pub fn record(&mut self, action: D::Undo) { @@ -176,7 +174,7 @@ impl SnapshotVec { pub fn start_snapshot(&mut self) -> Snapshot { let length = self.undo_log.len(); - self.undo_log.push(OpenSnapshot); + self.num_open_snapshots += 1; Snapshot { length: length } } @@ -185,14 +183,9 @@ impl SnapshotVec { } fn assert_open_snapshot(&self, snapshot: &Snapshot) { - // Or else there was a failure to follow a stack discipline: - assert!(self.undo_log.len() > snapshot.length); - - // Invariant established by start_snapshot(): - assert!(match self.undo_log[snapshot.length] { - OpenSnapshot => true, - _ => false, - }); + // Failures here may indicate a failure to follow a stack discipline. + assert!(self.undo_log.len() >= snapshot.length); + assert!(self.num_open_snapshots > 0); } pub fn rollback_to(&mut self, snapshot: Snapshot) { @@ -200,18 +193,8 @@ impl SnapshotVec { self.assert_open_snapshot(&snapshot); - while self.undo_log.len() > snapshot.length + 1 { + while self.undo_log.len() > snapshot.length { match self.undo_log.pop().unwrap() { - OpenSnapshot => { - // This indicates a failure to obey the stack discipline. - panic!("Cannot rollback an uncommitted snapshot"); - } - - CommittedSnapshot => { - // This occurs when there are nested snapshots and - // the inner is committed but outer is rolled back. - } - NewElem(i) => { self.values.pop(); assert!(self.values.len() == i); @@ -227,12 +210,7 @@ impl SnapshotVec { } } - let v = self.undo_log.pop().unwrap(); - assert!(match v { - OpenSnapshot => true, - _ => false, - }); - assert!(self.undo_log.len() == snapshot.length); + self.num_open_snapshots -= 1; } /// Commits all changes since the last snapshot. Of course, they @@ -242,12 +220,15 @@ impl SnapshotVec { self.assert_open_snapshot(&snapshot); - if snapshot.length == 0 { - // The root snapshot. - self.undo_log.truncate(0); - } else { - self.undo_log[snapshot.length] = CommittedSnapshot; + 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(); } + + self.num_open_snapshots -= 1; } } @@ -301,6 +282,7 @@ where SnapshotVec { values: self.values.clone(), undo_log: self.undo_log.clone(), + num_open_snapshots: self.num_open_snapshots, } } } @@ -312,11 +294,77 @@ where { fn clone(&self) -> Self { match *self { - OpenSnapshot => OpenSnapshot, - CommittedSnapshot => CommittedSnapshot, NewElem(i) => NewElem(i), SetElem(i, ref v) => SetElem(i, v.clone()), Other(ref u) => Other(u.clone()), } } } + +impl SnapshotVecDelegate for i32 { + type Value = i32; + type Undo = (); + + fn reverse(_: &mut Vec, _: ()) {} +} + +#[test] +fn basic() { + let mut vec: SnapshotVec = SnapshotVec::default(); + assert!(!vec.in_snapshot()); + assert_eq!(vec.len(), 0); + vec.push(22); + vec.push(33); + assert_eq!(vec.len(), 2); + assert_eq!(*vec.get(0), 22); + assert_eq!(*vec.get(1), 33); + vec.set(1, 34); + assert_eq!(vec.len(), 2); + assert_eq!(*vec.get(0), 22); + assert_eq!(*vec.get(1), 34); + + let snapshot = vec.start_snapshot(); + assert!(vec.in_snapshot()); + + vec.push(44); + vec.push(55); + vec.set(1, 35); + assert_eq!(vec.len(), 4); + assert_eq!(*vec.get(0), 22); + assert_eq!(*vec.get(1), 35); + assert_eq!(*vec.get(2), 44); + assert_eq!(*vec.get(3), 55); + + vec.rollback_to(snapshot); + assert!(!vec.in_snapshot()); + + assert_eq!(vec.len(), 2); + assert_eq!(*vec.get(0), 22); + assert_eq!(*vec.get(1), 34); +} + +#[test] +#[should_panic] +fn out_of_order() { + let mut vec: SnapshotVec = SnapshotVec::default(); + vec.push(22); + let snapshot1 = vec.start_snapshot(); + vec.push(33); + let snapshot2 = vec.start_snapshot(); + vec.push(44); + vec.rollback_to(snapshot1); // bogus, but accepted + vec.rollback_to(snapshot2); // asserts +} + +#[test] +fn nested_commit_then_rollback() { + let mut vec: SnapshotVec = SnapshotVec::default(); + vec.push(22); + let snapshot1 = vec.start_snapshot(); + let snapshot2 = vec.start_snapshot(); + vec.set(0, 23); + vec.commit(snapshot2); + assert_eq!(*vec.get(0), 23); + vec.rollback_to(snapshot1); + assert_eq!(*vec.get(0), 22); +}