From 6159f7531a0340a69c4926c4e0325811ed7cabb8 Mon Sep 17 00:00:00 2001 From: Joonatan Saarhelo Date: Tue, 15 Oct 2024 13:23:25 +0100 Subject: [PATCH] fix: restore instruction count functionality (#3081) Brings back instruction counts that show that bytecode has changed rather than VM performance. They were previously disabled because vm2 didn't support tracers. --- Cargo.lock | 1 + core/tests/vm-benchmark/Cargo.toml | 1 + core/tests/vm-benchmark/benches/iai.rs | 1 + .../src/bin/compare_iai_results.rs | 31 +++++----- .../src/bin/instruction_counts.rs | 9 ++- .../vm-benchmark/src/instruction_counter.rs | 1 - core/tests/vm-benchmark/src/vm.rs | 58 ++++++++++++++----- 7 files changed, 71 insertions(+), 31 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 11c37bda57f1..774471d3d6c3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8743,6 +8743,7 @@ dependencies = [ "zksync_types", "zksync_utils", "zksync_vlog", + "zksync_vm2", ] [[package]] diff --git a/core/tests/vm-benchmark/Cargo.toml b/core/tests/vm-benchmark/Cargo.toml index 4586c637e128..59c1e21493b4 100644 --- a/core/tests/vm-benchmark/Cargo.toml +++ b/core/tests/vm-benchmark/Cargo.toml @@ -11,6 +11,7 @@ zksync_multivm.workspace = true zksync_types.workspace = true zksync_utils.workspace = true zksync_vlog.workspace = true +zksync_vm2.workspace = true criterion.workspace = true once_cell.workspace = true diff --git a/core/tests/vm-benchmark/benches/iai.rs b/core/tests/vm-benchmark/benches/iai.rs index 6b8965afa4f1..8cbb9f10dd83 100644 --- a/core/tests/vm-benchmark/benches/iai.rs +++ b/core/tests/vm-benchmark/benches/iai.rs @@ -31,4 +31,5 @@ make_functions_and_main!( write_and_decode => write_and_decode_legacy, event_spam => event_spam_legacy, slot_hash_collision => slot_hash_collision_legacy, + heap_read_write => heap_read_write_legacy, ); diff --git a/core/tests/vm-benchmark/src/bin/compare_iai_results.rs b/core/tests/vm-benchmark/src/bin/compare_iai_results.rs index faf72a18f451..c274b039c9bd 100644 --- a/core/tests/vm-benchmark/src/bin/compare_iai_results.rs +++ b/core/tests/vm-benchmark/src/bin/compare_iai_results.rs @@ -25,14 +25,7 @@ fn main() { .keys() .collect::>() .intersection(&iai_after.keys().collect()) - .filter_map(|&name| { - let diff = percent_difference(iai_before[name], iai_after[name]); - if diff.abs() > 2. { - Some((name, format!("{:+.1}%", diff))) - } else { - None - } - }) + .map(|&name| (name, percent_difference(iai_before[name], iai_after[name]))) .collect::>(); let duration_changes = opcodes_before @@ -47,12 +40,17 @@ fn main() { let mut nonzero_diff = false; - for name in perf_changes.keys().collect::>().union( - &duration_changes - .iter() - .filter_map(|(key, value)| (*value != 0).then_some(key)) - .collect(), - ) { + for name in perf_changes + .iter() + .filter_map(|(key, value)| (value.abs() > 2.).then_some(key)) + .collect::>() + .union( + &duration_changes + .iter() + .filter_map(|(key, value)| (*value != 0).then_some(key)) + .collect(), + ) + { // write the header before writing the first line of diff if !nonzero_diff { println!("Benchmark name | change in estimated runtime | change in number of opcodes executed \n--- | --- | ---"); @@ -63,7 +61,10 @@ fn main() { println!( "{} | {} | {}", name, - perf_changes.get(**name).unwrap_or(&n_a.clone()), + perf_changes + .get(**name) + .map(|percent| format!("{:+.1}%", percent)) + .unwrap_or(n_a.clone()), duration_changes .get(**name) .map(|abs_diff| format!( diff --git a/core/tests/vm-benchmark/src/bin/instruction_counts.rs b/core/tests/vm-benchmark/src/bin/instruction_counts.rs index f9bb04c01bff..96208007fd97 100644 --- a/core/tests/vm-benchmark/src/bin/instruction_counts.rs +++ b/core/tests/vm-benchmark/src/bin/instruction_counts.rs @@ -1,11 +1,16 @@ //! Runs all benchmarks and prints out the number of zkEVM opcodes each one executed. -use vm_benchmark::{BenchmarkingVm, BYTECODES}; +use vm_benchmark::{BenchmarkingVmFactory, Fast, Legacy, BYTECODES}; fn main() { for bytecode in BYTECODES { let tx = bytecode.deploy_tx(); let name = bytecode.name; - println!("{name} {}", BenchmarkingVm::new().instruction_count(&tx)); + println!("{name} {}", Fast::<()>::count_instructions(&tx)); + println!( + "{} {}", + name.to_string() + "_legacy", + Legacy::count_instructions(&tx) + ); } } diff --git a/core/tests/vm-benchmark/src/instruction_counter.rs b/core/tests/vm-benchmark/src/instruction_counter.rs index 48b1e3527ade..0899c4c91719 100644 --- a/core/tests/vm-benchmark/src/instruction_counter.rs +++ b/core/tests/vm-benchmark/src/instruction_counter.rs @@ -13,7 +13,6 @@ pub struct InstructionCounter { /// A tracer that counts the number of instructions executed by the VM. impl InstructionCounter { - #[allow(dead_code)] // FIXME: re-enable instruction counting once new tracers are merged pub fn new(output: Rc>) -> Self { Self { count: 0, output } } diff --git a/core/tests/vm-benchmark/src/vm.rs b/core/tests/vm-benchmark/src/vm.rs index f4a0010f29eb..30e2321298fe 100644 --- a/core/tests/vm-benchmark/src/vm.rs +++ b/core/tests/vm-benchmark/src/vm.rs @@ -9,8 +9,8 @@ use zksync_multivm::{ VmExecutionResultAndLogs, VmFactory, VmInterface, VmInterfaceExt, VmInterfaceHistoryEnabled, }, - vm_fast, vm_latest, - vm_latest::{constants::BATCH_COMPUTATIONAL_GAS_LIMIT, HistoryEnabled}, + vm_fast, + vm_latest::{self, constants::BATCH_COMPUTATIONAL_GAS_LIMIT, HistoryEnabled, ToTracerPointer}, zk_evm_latest::ethereum_types::{Address, U256}, }; use zksync_types::{ @@ -20,7 +20,7 @@ use zksync_types::{ }; use zksync_utils::bytecode::hash_bytecode; -use crate::transaction::PRIVATE_KEY; +use crate::{instruction_counter::InstructionCounter, transaction::PRIVATE_KEY}; static SYSTEM_CONTRACTS: Lazy = Lazy::new(BaseSystemContracts::load_from_disk); @@ -72,16 +72,19 @@ pub trait BenchmarkingVmFactory { system_env: SystemEnv, storage: &'static InMemoryStorage, ) -> Self::Instance; + + /// Counts instructions executed by the VM while processing the transaction. + fn count_instructions(tx: &Transaction) -> usize; } /// Factory for the new / fast VM. #[derive(Debug)] -pub struct Fast(()); +pub struct Fast(Tr); -impl BenchmarkingVmFactory for Fast { +impl BenchmarkingVmFactory for Fast { const LABEL: VmLabel = VmLabel::Fast; - type Instance = vm_fast::Vm<&'static InMemoryStorage>; + type Instance = vm_fast::Vm<&'static InMemoryStorage, Tr>; fn create( batch_env: L1BatchEnv, @@ -90,6 +93,29 @@ impl BenchmarkingVmFactory for Fast { ) -> Self::Instance { vm_fast::Vm::custom(batch_env, system_env, storage) } + + fn count_instructions(tx: &Transaction) -> usize { + let mut vm = BenchmarkingVm::>::default(); + vm.0.push_transaction(tx.clone()); + + #[derive(Default)] + struct InstructionCount(usize); + impl vm_fast::Tracer for InstructionCount { + fn before_instruction< + OP: zksync_vm2::interface::OpcodeType, + S: zksync_vm2::interface::StateInterface, + >( + &mut self, + _: &mut S, + ) { + self.0 += 1; + } + } + let mut tracer = InstructionCount(0); + + vm.0.inspect(&mut tracer, VmExecutionMode::OneTx); + tracer.0 + } } /// Factory for the legacy VM (latest version). @@ -109,6 +135,19 @@ impl BenchmarkingVmFactory for Legacy { let storage = StorageView::new(storage).to_rc_ptr(); vm_latest::Vm::new(batch_env, system_env, storage) } + + fn count_instructions(tx: &Transaction) -> usize { + let mut vm = BenchmarkingVm::::default(); + vm.0.push_transaction(tx.clone()); + let count = Rc::new(RefCell::new(0)); + vm.0.inspect( + &mut InstructionCounter::new(count.clone()) + .into_tracer_pointer() + .into(), + VmExecutionMode::OneTx, + ); + count.take() + } } #[derive(Debug)] @@ -169,13 +208,6 @@ impl BenchmarkingVm { } tx_result } - - pub fn instruction_count(&mut self, tx: &Transaction) -> usize { - self.0.push_transaction(tx.clone()); - let count = Rc::new(RefCell::new(0)); - self.0.execute(VmExecutionMode::OneTx); // FIXME: re-enable instruction counting once new tracers are merged - count.take() - } } impl BenchmarkingVm {