feat(vm): Improve tracer trait #121

Deniallugo · 2023-09-29T14:55:07Z

What ❔

Changing the way, how tracers are working in VM. Try to make less calls and add more static dispatching

Tracer Api changes:

Method before decoding now always empty (zk_evm don't call it)
Method before cycle doesn't exist anymore
Method after_cycle renamed to finish_cycle and unified with should stop execution
Method save_results was removed
All methods from ExecutionProcessing trait moved to VmTracer

Vm Changes:
Refund tracer now dispatching statically and not dynamically

Why ❔

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.

…deb6bd627ec9dc901a78aca374f16fd14e

codecov · 2023-09-29T16:11:19Z

Codecov Report

Attention: 36 lines in your changes are missing coverage. Please review.

Comparison is base (7e23188) 32.73% compared to head (b266161) 32.75%.
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #121      +/-   ##
==========================================
+ Coverage   32.73%   32.75%   +0.02%     
==========================================
  Files         536      536              
  Lines       28122    28126       +4     
==========================================
+ Hits         9207     9214       +7     
+ Misses      18915    18912       -3

Files	Coverage Δ
core/bin/system-constants-generator/src/utils.rs	`0.00% <ø> (ø)`
core/lib/vm/src/tracers/call.rs	`50.00% <100.00%> (ø)`
core/lib/vm/src/tracers/result_tracer.rs	`50.00% <ø> (ø)`
core/lib/vm/src/tracers/traits.rs	`73.33% <85.71%> (-8.49%)`	⬇️
core/lib/vm/src/tracers/validation/mod.rs	`0.00% <0.00%> (ø)`
core/lib/vm/src/tracers/refunds.rs	`55.93% <50.00%> (+2.15%)`	⬆️
core/lib/vm/src/tracers/storage_invocations.rs	`0.00% <0.00%> (ø)`
core/lib/vm/src/implementation/execution.rs	`48.27% <46.15%> (-3.58%)`	⬇️
core/lib/vm/src/tracers/default_tracers.rs	`50.96% <45.45%> (+4.42%)`	⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…6deb6bd627ec9dc901a78aca374f16fd14e

Signed-off-by: Danil <deniallugo@gmail.com>

mm-zk · 2023-10-11T15:03:06Z

@Deniallugo - can you add more info in the PR description?
which methods you are removing, what are you changing in the API

RomanBrodetski · 2023-10-11T15:03:45Z

core/lib/vm/src/tracers/default_tracers.rs

+                Halt::ValidationOutOfGas,
+            ));
+        }
+        TracerExecutionStatus::Continue


this is a little hard to follow - we do match, don't return only in the Batch variabt, and then do an if. I think it would be cleaner with

match self.execution_mode { VmExecutionMode::OneTx if self.tx_has_been_processed() => TracerExecutionStatus::Stop(TracerExecutionStopReason::Finish) } VmExecutionMode::Bootloader if self.ret_from_the_bootloader == Some(RetOpcode::Ok) => TracerExecutionStatus::Stop(TracerExecutionStopReason::Finish); } VmExecutionMode::Batch if self.validation_run_out_of_gas() => TracerExecutionStatus::Stop(TracerExecutionStopReason::Abort( Halt::ValidationOutOfGas, _ => TracerExecutionStatus::Continue };

The two variants can be squashed into one i think with |

We have to always check self.validation_run_out_of_gas. Independent from execution_mode.
Thanks for the other suggestions :)

The two variants can be squashed into one i think with |

Only without if..

Why? We dobn't check it now since we return

RomanBrodetski · 2023-10-11T15:08:22Z

@Deniallugo can you please explain why the RefundTracer needs to be treated specially?

Deniallugo · 2023-10-11T15:10:13Z

@Deniallugo can you please explain why the RefundTracer needs to be treated specially?

It's an internal tracer for VM and it used almost with all calls and the results of the execution are presented in VmExecutionResultAndLogs

From performance perspective it's better to make it statically dispatched

core/lib/vm/src/tracers/traits.rs

core/lib/vm/src/tracers/refunds.rs

core/lib/vm/src/tracers/default_tracers.rs

core/lib/vm/src/implementation/execution.rs

montekki · 2023-10-11T15:24:12Z

core/lib/vm/src/tracers/default_tracers.rs

+                Halt::ValidationOutOfGas,
+            ));
+        }
+        TracerExecutionStatus::Continue


The two variants can be squashed into one i think with |

Signed-off-by: Danil <deniallugo@gmail.com>

core/lib/vm/src/tracers/traits.rs

perekopskiy

Looks really nice! Left a couple of nits

core/lib/vm/src/implementation/execution.rs

core/lib/vm/src/tracers/traits.rs

Signed-off-by: Danil <deniallugo@gmail.com>

github-actions · 2023-10-12T08:49:11Z

Detected VM performance changes

Benchmark name	Difference in runtime
decode_shl_sub	-3.3%
event_spam	-2.5%
slot_hash_collision	-3.2%
access_memory	-3.0%
write_and_decode	-3.1%

core/lib/vm/src/tracers/traits.rs

🤖 I have created a release *beep* *boop* --- ## [16.1.0](core-v16.0.2...core-v16.1.0) (2023-10-24) ### Features * Add new commitments ([#219](#219)) ([a19256e](a19256e)) * arm64 zk-environment rust Docker images and other ([#296](#296)) ([33174aa](33174aa)) * **config:** Extract everything not related to the env config from zksync_config crate ([#245](#245)) ([42c64e9](42c64e9)) * **eth-watch:** process governor upgrades ([#247](#247)) ([d250294](d250294)) * **merkle tree:** Expose Merkle tree API ([#209](#209)) ([4010c7e](4010c7e)) * **merkle tree:** Snapshot recovery for Merkle tree ([#163](#163)) ([9e20703](9e20703)) * **multivm:** Remove lifetime from multivm ([#218](#218)) ([7eda27c](7eda27c)) * Remove fee_ticker and token_trading_volume fetcher modules ([#262](#262)) ([44f7179](44f7179)) * **reorg_detector:** compare miniblock hashes for reorg detection ([#236](#236)) ([2c930b2](2c930b2)) * Rewrite server binary to use `vise` metrics ([#120](#120)) ([26ee1fb](26ee1fb)) * **types:** introduce state diff record type and compression ([#194](#194)) ([ccf753c](ccf753c)) * **vm:** Improve tracer trait ([#121](#121)) ([ff60138](ff60138)) * **vm:** Move all vm versions to the one crate ([#249](#249)) ([e3fb489](e3fb489)) ### Bug Fixes * **crypto:** update snark-vk to be used in server and update args for proof wrapping ([#240](#240)) ([4a5c54c](4a5c54c)) * **db:** Fix write stalls in RocksDB ([#250](#250)) ([650124c](650124c)) * **db:** Fix write stalls in RocksDB (again) ([#265](#265)) ([7b23ab0](7b23ab0)) * **db:** Fix write stalls in RocksDB (for real this time) ([#292](#292)) ([0f15919](0f15919)) * Fix `TxStage` string representation ([#255](#255)) ([246b5a0](246b5a0)) * fix typos ([#226](#226)) ([feb8a6c](feb8a6c)) * **witness-generator:** Witness generator oracle with cached storage refunds ([#274](#274)) ([8928a41](8928a41)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

…mode and Rollup mode (#121) * Add example: build_commit_tx_input_data_is_correct (fails) * Abstract normal_checker_function test * Abstract checker_processes_pre_boojum_batches * Abstract checker_functions_after_snapshot_recovery test * Abstract checker_functions_after_snapshot_recovery test * Remove unnecessary auxiliar function * Fix all the failing tests * Use test_helpers module * Remove ValidiumModeL1BatchCommitDataGenerator fix * Add bytes pubdata with 0 value for the encoding (#136)

Deniallugo requested review from RomanBrodetski, perekopskiy, popzxc and a team as code owners September 29, 2023 14:55

Deniallugo changed the title ~~Mirrors branch deniallugo-vm-perf-optimization from private at 008df6…~~ chore(perf): Remove unnecessary tracer calls Sep 29, 2023

Deniallugo marked this pull request as draft September 29, 2023 15:26

Mirrors branch deniallugo-vm-perf-optimization from private at 008df6…

aa3172c

…deb6bd627ec9dc901a78aca374f16fd14e

Deniallugo force-pushed the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch from 57259be to aa3172c Compare September 29, 2023 15:56

Merge branch 'main' into mirror-deniallugo-vm-perf-optimization-008df…

63dd153

…6deb6bd627ec9dc901a78aca374f16fd14e

Deniallugo force-pushed the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch from f6ab256 to 63dd153 Compare October 10, 2023 16:05

Deniallugo added 2 commits October 10, 2023 19:07

Remove ExecutionEndTracer trait

a9e5799

Signed-off-by: Danil <deniallugo@gmail.com>

Move save results to VmTracer and remove mutable reference

f3f53e6

Signed-off-by: Danil <deniallugo@gmail.com>

Deniallugo force-pushed the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch from 0eabe2a to b54f04a Compare October 10, 2023 17:31

Remove finish_global_frame optimization

d5aaf93

Signed-off-by: Danil <deniallugo@gmail.com>

Deniallugo force-pushed the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch 2 times, most recently from dbd8145 to d5aaf93 Compare October 10, 2023 18:07

Attemp to use references

e65dc72

Signed-off-by: Danil <deniallugo@gmail.com>

Deniallugo force-pushed the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch 3 times, most recently from 54aac4a to db319f3 Compare October 11, 2023 14:26

Deniallugo marked this pull request as ready for review October 11, 2023 14:27

Deniallugo requested a review from a team as a code owner October 11, 2023 14:27

Deniallugo requested a review from slowli October 11, 2023 14:39

RomanBrodetski reviewed Oct 11, 2023

View reviewed changes

mm-zk reviewed Oct 11, 2023

View reviewed changes

montekki reviewed Oct 11, 2023

View reviewed changes

Move continue as the most frequent result to the top

a3cab2c

Signed-off-by: Danil <deniallugo@gmail.com>

Deniallugo force-pushed the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch from db319f3 to a3cab2c Compare October 11, 2023 15:27

montekki previously approved these changes Oct 11, 2023

View reviewed changes

core/lib/vm/src/tracers/traits.rs Outdated Show resolved Hide resolved

Deniallugo dismissed montekki’s stale review via a8108be October 12, 2023 08:21

Deniallugo force-pushed the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch from 94c2986 to a8108be Compare October 12, 2023 08:21

Deniallugo requested a review from RomanBrodetski October 12, 2023 08:23

perekopskiy reviewed Oct 12, 2023

View reviewed changes

core/lib/vm/src/implementation/execution.rs Show resolved Hide resolved

core/lib/vm/src/tracers/traits.rs Outdated Show resolved Hide resolved

Fix some nits

b266161

Signed-off-by: Danil <deniallugo@gmail.com>

Deniallugo force-pushed the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch from a8108be to b266161 Compare October 12, 2023 08:41

Deniallugo requested review from perekopskiy and montekki October 12, 2023 08:42

Deniallugo changed the title ~~chore(perf): Remove unnecessary tracer calls~~ feat(vm): Improve tracer trait Oct 12, 2023

perekopskiy approved these changes Oct 12, 2023

View reviewed changes

montekki approved these changes Oct 12, 2023

View reviewed changes

slowli reviewed Oct 12, 2023

View reviewed changes

core/lib/vm/src/tracers/traits.rs Show resolved Hide resolved

core/lib/vm/src/tracers/traits.rs Show resolved Hide resolved

core/lib/vm/src/tracers/traits.rs Show resolved Hide resolved

Deniallugo added this pull request to the merge queue Oct 12, 2023

Deniallugo removed this pull request from the merge queue due to a manual request Oct 12, 2023

Deniallugo added this pull request to the merge queue Oct 12, 2023

Deniallugo removed this pull request from the merge queue due to a manual request Oct 12, 2023

Deniallugo added this pull request to the merge queue Oct 12, 2023

Merged via the queue into main with commit ff60138 Oct 12, 2023
24 checks passed

Deniallugo deleted the mirror-deniallugo-vm-perf-optimization-008df6deb6bd627ec9dc901a78aca374f16fd14e branch October 12, 2023 18:51

zksync-era-bot mentioned this pull request Oct 12, 2023

chore(main): release core 16.1.0 #217

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(vm): Improve tracer trait #121

feat(vm): Improve tracer trait #121

Deniallugo commented Sep 29, 2023 •

edited

Loading

codecov bot commented Sep 29, 2023 •

edited

Loading

mm-zk commented Oct 11, 2023

RomanBrodetski Oct 11, 2023

montekki Oct 11, 2023

Deniallugo Oct 11, 2023

RomanBrodetski Oct 13, 2023

RomanBrodetski commented Oct 11, 2023

Deniallugo commented Oct 11, 2023

montekki Oct 11, 2023

perekopskiy left a comment

github-actions bot commented Oct 12, 2023

feat(vm): Improve tracer trait #121

feat(vm): Improve tracer trait #121

Conversation

Deniallugo commented Sep 29, 2023 • edited Loading

What ❔

Why ❔

Checklist

codecov bot commented Sep 29, 2023 • edited Loading

Codecov Report

mm-zk commented Oct 11, 2023

RomanBrodetski Oct 11, 2023

Choose a reason for hiding this comment

montekki Oct 11, 2023

Choose a reason for hiding this comment

Deniallugo Oct 11, 2023

Choose a reason for hiding this comment

RomanBrodetski Oct 13, 2023

Choose a reason for hiding this comment

RomanBrodetski commented Oct 11, 2023

Deniallugo commented Oct 11, 2023

montekki Oct 11, 2023

Choose a reason for hiding this comment

perekopskiy left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 12, 2023

Detected VM performance changes

Deniallugo commented Sep 29, 2023 •

edited

Loading

codecov bot commented Sep 29, 2023 •

edited

Loading