-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(vm): Improve snapshot management in batch executor #2513
Changes from 10 commits
b5f8c23
bf01e82
671057d
decad61
09e0c45
d6b881a
7c2524d
cd1a2a4
6636f81
c119e9a
babd716
92e6ee4
c1876f9
5073f0e
b51e26d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,13 +147,25 @@ pub trait VmFactory<S>: VmInterface { | |
} | ||
|
||
/// Methods of VM requiring history manipulations. | ||
/// | ||
/// # Snapshot workflow | ||
/// | ||
/// External callers must follow the following snapshot workflow: | ||
/// | ||
/// - Each new snapshot created using `make_snapshot()` must be either popped or rolled back before creating the following snapshot. | ||
/// OTOH, it's not required to call either of these methods by the end of VM execution. | ||
/// - `pop_snapshot_no_rollback()` may be called spuriously, when no snapshot was created. It is a no-op in this case. | ||
/// | ||
/// These rules guarantee that at each given moment, a VM instance has <=1 snapshot (unless the VM makes snapshots internally), | ||
slowli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// which may allow additional VM optimizations. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that a low amount of snapshots just avoids allocating a Vec for them. Zero snapshots allow forgetting history, which is important for memory use. |
||
pub trait VmInterfaceHistoryEnabled: VmInterface { | ||
/// Create a snapshot of the current VM state and push it into memory. | ||
fn make_snapshot(&mut self); | ||
|
||
/// Roll back VM state to the latest snapshot and destroy the snapshot. | ||
fn rollback_to_the_latest_snapshot(&mut self); | ||
|
||
/// Pop the latest snapshot from memory and destroy it. | ||
/// Pop the latest snapshot from memory and destroy it. If there are no snapshots, this should be a no-op | ||
/// (i.e., the VM must not panic in this case). | ||
fn pop_snapshot_no_rollback(&mut self); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,14 +146,12 @@ impl<S: WriteStorage, H: HistoryMode> VmFactory<S> for Vm<S, H> { | |
} | ||
} | ||
|
||
/// Methods of vm, which required some history manipulations | ||
impl<S: WriteStorage> VmInterfaceHistoryEnabled for Vm<S, HistoryEnabled> { | ||
/// Create snapshot of current vm state and push it into the memory | ||
fn make_snapshot(&mut self) { | ||
self.make_snapshot_inner() | ||
self.make_snapshot_inner(); | ||
} | ||
|
||
/// Rollback vm state to the latest snapshot and destroy the snapshot | ||
fn rollback_to_the_latest_snapshot(&mut self) { | ||
let snapshot = self | ||
.snapshots | ||
|
@@ -162,10 +160,7 @@ impl<S: WriteStorage> VmInterfaceHistoryEnabled for Vm<S, HistoryEnabled> { | |
self.rollback_to_snapshot(snapshot); | ||
} | ||
|
||
/// Pop the latest snapshot from the memory and destroy it | ||
fn pop_snapshot_no_rollback(&mut self) { | ||
self.snapshots | ||
.pop() | ||
.expect("Snapshot should be created before rolling it back"); | ||
self.snapshots.pop(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When is it necessary to pop a nonexistent snapshot? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It happens here on the first transaction, or after the previous transaction was rolled back. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you could just always pop the snapshot if it isn't rolled back in the same place where you may roll it back. |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean? Seems to contradict the line above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the contradiction? We don't create a snapshot at the end of the VM execution, so it may have either 0 or 1 snapshot at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This confused me (and still does) but I can understand now that it means that you can have multiple TX after your snapshot and then rollback/forget.