perf(vm): Improve snapshot management in batch executor #2513

slowli · 2024-07-26T13:55:05Z

What ❔

Does not embed snapshots into each other, and instead use a linear workflow.
Discards an old snapshot, if any, when processing a new transaction (currently, old snapshots are seemingly never discarded unless the transaction is rolled back).

Why ❔

Linear snapshow workflow is easier to grasp and it could allow optimizations in the future.
Accumulating snapshots introduces an overhead, esp. in the new VM.

Checklist

PR title corresponds to the body of PR (we generate changelog entries from PRs).
Tests for the changes have been added / updated.
Documentation comments have been added / updated.
Code has been formatted via zk fmt and zk lint.

core/node/state_keeper/src/batch_executor/traits.rs

core/node/state_keeper/src/batch_executor/tests/batch_vm_factory.rs

core/node/state_keeper/src/batch_executor/traits.rs

joonazan · 2024-07-30T11:36:59Z

core/lib/multivm/src/interface/traits/vm.rs

+/// 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.


What does this mean? Seems to contradict the line above.

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.

it's not required to call either of these methods by the end of VM execution.

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.

core/lib/multivm/src/interface/traits/vm.rs

joonazan · 2024-07-30T11:39:32Z

core/lib/multivm/src/interface/traits/vm.rs

+/// - `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),
+/// which may allow additional VM optimizations.


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.

core/lib/multivm/src/versions/shadow.rs

core/lib/state/src/rocksdb/mod.rs

core/node/state_keeper/src/batch_executor/traits.rs

slowli · 2024-07-30T18:06:09Z

I've decided to remove batch VM traits from the PR. They can be added separately, after the new VM is merged into main.

slowli · 2024-07-31T08:44:40Z

There are 2 somewhat suspicious failing CI jobs on this PR, but I don't see how they could be related to this PR:

First: Missing revert data for estimateGas during the upgrade test
Second: Missing revert data for estimateGas during server integration tests (specifically "ERC20 contract checks › Can perform a deposit with precalculated max value") + reuse of a nonce in another server integration test ("ERC20 contract checks › Should claim failed deposit").

Since the new VM isn't integrated into the API server, I don't see how the estimateGas error could be related to this PR or the new VM integration in general, but I may be missing something.

joonazan

I'm in favor of merging if you are sure that the disabled tests can be fixed.

joonazan · 2024-07-31T14:13:24Z

core/lib/multivm/src/versions/vm_refunds_enhancement/vm.rs

-        self.snapshots
-            .pop()
-            .expect("Snapshot should be created before rolling it back");
+        self.snapshots.pop();


When is it necessary to pop a nonexistent snapshot?

It happens here on the first transaction, or after the previous transaction was rolled back.

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.

slowli added 6 commits July 26, 2024 14:03

Rework VmInterfaceHistoryEnabled

b5f8c23

Do not embed / accumulate snapshots in batch executor

bf01e82

Sketch batch VM-related traits

671057d

Brush up batch executor VM traits

decad61

Test snapshot count

09e0c45

Update from upstream

d6b881a

slowli commented Jul 29, 2024

View reviewed changes

core/node/state_keeper/src/batch_executor/traits.rs Outdated Show resolved Hide resolved

core/node/state_keeper/src/batch_executor/traits.rs Outdated Show resolved Hide resolved

core/node/state_keeper/src/batch_executor/traits.rs Outdated Show resolved Hide resolved

Fix bytecode compression logic

7c2524d

slowli marked this pull request as ready for review July 29, 2024 14:12

slowli requested review from popzxc, montekki, joonazan and Artemka374 July 29, 2024 14:12

popzxc reviewed Jul 30, 2024

View reviewed changes

slowli added 3 commits July 30, 2024 12:49

Remove BatchExecutor trait

cd1a2a4

Brush up batch executor code

6636f81

Rename assertion method

c119e9a

joonazan reviewed Jul 30, 2024

View reviewed changes

slowli added 5 commits July 30, 2024 18:17

Assert <=1 snapshot for new VM

babd716

Remove custom VM factory implementation

92e6ee4

Update from upstream

c1876f9

Ignore failing unit tests

5073f0e

Revert batch VM traits

b51e26d

slowli force-pushed the aov-pla-1000-improve-snapshot-management-in-batch-executor branch from bcec129 to b51e26d Compare July 30, 2024 18:05

slowli requested review from joonazan and popzxc July 31, 2024 08:44

joonazan reviewed Jul 31, 2024

View reviewed changes

slowli merged commit 69be78c into jms-vm2 Jul 31, 2024
47 checks passed

slowli deleted the aov-pla-1000-improve-snapshot-management-in-batch-executor branch July 31, 2024 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(vm): Improve snapshot management in batch executor #2513

perf(vm): Improve snapshot management in batch executor #2513

slowli commented Jul 26, 2024 •

edited

Loading

joonazan Jul 30, 2024

slowli Jul 30, 2024

joonazan Jul 31, 2024

joonazan Jul 30, 2024

slowli commented Jul 30, 2024

slowli commented Jul 31, 2024

joonazan left a comment

joonazan Jul 31, 2024

slowli Jul 31, 2024

joonazan Jul 31, 2024

perf(vm): Improve snapshot management in batch executor #2513

perf(vm): Improve snapshot management in batch executor #2513

Conversation

slowli commented Jul 26, 2024 • edited Loading

What ❔

Why ❔

Checklist

joonazan Jul 30, 2024

Choose a reason for hiding this comment

slowli Jul 30, 2024

Choose a reason for hiding this comment

joonazan Jul 31, 2024

Choose a reason for hiding this comment

joonazan Jul 30, 2024

Choose a reason for hiding this comment

slowli commented Jul 30, 2024

slowli commented Jul 31, 2024

joonazan left a comment

Choose a reason for hiding this comment

joonazan Jul 31, 2024

Choose a reason for hiding this comment

slowli Jul 31, 2024

Choose a reason for hiding this comment

joonazan Jul 31, 2024

Choose a reason for hiding this comment

slowli commented Jul 26, 2024 •

edited

Loading