From 7aefb039d6b583d6f6f261815164d0d52aefeed1 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 3 Dec 2024 18:00:58 +0100 Subject: [PATCH 1/3] perf(coverage): cache current HitMap, reserve when merging --- crates/evm/coverage/src/inspector.rs | 76 +++++++++++++++++++++++----- crates/evm/coverage/src/lib.rs | 31 ++++++++++-- 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/crates/evm/coverage/src/inspector.rs b/crates/evm/coverage/src/inspector.rs index 07c24ae9e5ef..eeea3900c0d9 100644 --- a/crates/evm/coverage/src/inspector.rs +++ b/crates/evm/coverage/src/inspector.rs @@ -1,25 +1,40 @@ use crate::{HitMap, HitMaps}; use alloy_primitives::B256; use revm::{interpreter::Interpreter, Database, EvmContext, Inspector}; +use std::ptr::NonNull; /// Inspector implementation for collecting coverage information. -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug)] pub struct CoverageCollector { + current_map: NonNull, + current_hash: B256, maps: HitMaps, } +// SAFETY: `current_map` is always valid and points into an allocation managed by self. +unsafe impl Send for CoverageCollector {} +unsafe impl Sync for CoverageCollector {} + +impl Default for CoverageCollector { + fn default() -> Self { + Self { + current_map: NonNull::dangling(), + current_hash: B256::ZERO, + maps: Default::default(), + } + } +} + impl Inspector for CoverageCollector { fn initialize_interp(&mut self, interpreter: &mut Interpreter, _context: &mut EvmContext) { - self.maps - .entry(*get_contract_hash(interpreter)) - .or_insert_with(|| HitMap::new(interpreter.contract.bytecode.original_bytes())); + get_or_insert_contract_hash(interpreter); + self.insert_map(interpreter); } #[inline] fn step(&mut self, interpreter: &mut Interpreter, _context: &mut EvmContext) { - if let Some(map) = self.maps.get_mut(get_contract_hash(interpreter)) { - map.hit(interpreter.program_counter()); - } + let map = self.get_or_insert_map(interpreter); + map.hit(interpreter.program_counter()); } } @@ -28,15 +43,50 @@ impl CoverageCollector { pub fn finish(self) -> HitMaps { self.maps } + + #[inline] + fn get_or_insert_map(&mut self, interpreter: &mut Interpreter) -> &mut HitMap { + let hash = get_or_insert_contract_hash(interpreter); + if self.current_hash != *hash { + self.insert_map(interpreter); + } + unsafe { self.current_map.as_mut() } + } + + #[cold] + #[inline(never)] + fn insert_map(&mut self, interpreter: &Interpreter) { + let Some(hash) = interpreter.contract.hash else { eof_panic() }; + self.current_hash = hash; + self.current_map = self + .maps + .entry(hash) + .or_insert_with(|| HitMap::new(interpreter.contract.bytecode.original_bytes())) + .into(); + } } /// Helper function for extracting contract hash used to record coverage hit map. -/// If contract hash available in interpreter contract is zero (contract not yet created but going -/// to be created in current tx) then it hash is calculated from contract bytecode. -fn get_contract_hash(interpreter: &mut Interpreter) -> &B256 { - let hash = interpreter.contract.hash.as_mut().expect("coverage does not support EOF"); - if *hash == B256::ZERO { - *hash = interpreter.contract.bytecode.hash_slow(); +/// +/// If the contract hash is zero (contract not yet created but it's going to be created in current +/// tx) then the hash is calculated from the bytecode. +#[inline] +fn get_or_insert_contract_hash(interpreter: &mut Interpreter) -> &B256 { + let Some(hash) = interpreter.contract.hash.as_mut() else { eof_panic() }; + if hash.is_zero() { + set_contract_hash(hash, &interpreter.contract.bytecode); } hash } + +#[cold] +#[inline(never)] +fn set_contract_hash(hash: &mut B256, bytecode: &revm::primitives::Bytecode) { + *hash = bytecode.hash_slow(); +} + +#[cold] +#[inline(never)] +fn eof_panic() -> ! { + panic!("coverage does not support EOF"); +} diff --git a/crates/evm/coverage/src/lib.rs b/crates/evm/coverage/src/lib.rs index 345261ad5b2c..52ec329add14 100644 --- a/crates/evm/coverage/src/lib.rs +++ b/crates/evm/coverage/src/lib.rs @@ -174,6 +174,7 @@ impl HitMaps { /// Merges two `HitMaps`. pub fn merge(&mut self, other: Self) { + self.reserve(other.len()); for (code_hash, other) in other.0 { self.entry(code_hash).and_modify(|e| e.merge(&other)).or_insert(other); } @@ -211,37 +212,61 @@ pub struct HitMap { impl HitMap { /// Create a new hitmap with the given bytecode. + #[inline] pub fn new(bytecode: Bytes) -> Self { - Self { bytecode, hits: Default::default() } + Self { bytecode, hits: HashMap::with_capacity_and_hasher(1024, Default::default()) } } /// Returns the bytecode. + #[inline] pub fn bytecode(&self) -> &Bytes { &self.bytecode } /// Returns the number of hits for the given program counter. + #[inline] pub fn get(&self, pc: usize) -> Option { NonZeroU32::new(self.hits.get(&Self::cvt_pc(pc)).copied().unwrap_or(0)) } /// Increase the hit counter by 1 for the given program counter. + #[inline] pub fn hit(&mut self, pc: usize) { self.hits(pc, 1) } /// Increase the hit counter by `hits` for the given program counter. + #[inline] pub fn hits(&mut self, pc: usize, hits: u32) { *self.hits.entry(Self::cvt_pc(pc)).or_default() += hits; } /// Merge another hitmap into this, assuming the bytecode is consistent pub fn merge(&mut self, other: &Self) { - for (&pc, &hits) in &other.hits { - self.hits(pc as usize, hits); + self.hits.reserve(other.len()); + for (pc, hits) in other.iter() { + self.hits(pc, hits); } } + /// Returns an iterator over all the program counters and their hit counts. + #[inline] + pub fn iter(&self) -> impl Iterator + '_ { + self.hits.iter().map(|(&pc, &hits)| (pc as usize, hits)) + } + + /// Returns the number of program counters hit in the hitmap. + #[inline] + pub fn len(&self) -> usize { + self.hits.len() + } + + /// Returns `true` if the hitmap is empty. + #[inline] + pub fn is_empty(&self) -> bool { + self.hits.is_empty() + } + #[inline] fn cvt_pc(pc: usize) -> u32 { pc.try_into().expect("4GiB bytecode") From 9fd8cd138627d572e1747476094948ee170562fb Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 3 Dec 2024 18:14:03 +0100 Subject: [PATCH 2/3] test: shuffle RPC env --- crates/test-utils/src/rpc.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/test-utils/src/rpc.rs b/crates/test-utils/src/rpc.rs index 361bf56c2706..05cd84920f78 100644 --- a/crates/test-utils/src/rpc.rs +++ b/crates/test-utils/src/rpc.rs @@ -142,7 +142,8 @@ fn next_archive_endpoint(is_ws: bool) -> String { let rpc_env_vars = env::var(env_urls).unwrap_or_default(); if !rpc_env_vars.is_empty() { - let urls = rpc_env_vars.split(',').collect::>(); + let mut urls = rpc_env_vars.split(',').collect::>(); + urls.shuffle(&mut rand::thread_rng()); next(&urls).to_string() } else if is_ws { format!("wss://eth-mainnet.g.alchemy.com/v2/{}", next(&ALCHEMY_KEYS)) From f470ae500c11966e58e36cb4e366a5b8d763b82a Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Tue, 3 Dec 2024 20:43:59 +0100 Subject: [PATCH 3/3] Revert "test: shuffle RPC env" This reverts commit 9fd8cd138627d572e1747476094948ee170562fb. --- crates/test-utils/src/rpc.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/test-utils/src/rpc.rs b/crates/test-utils/src/rpc.rs index 05cd84920f78..361bf56c2706 100644 --- a/crates/test-utils/src/rpc.rs +++ b/crates/test-utils/src/rpc.rs @@ -142,8 +142,7 @@ fn next_archive_endpoint(is_ws: bool) -> String { let rpc_env_vars = env::var(env_urls).unwrap_or_default(); if !rpc_env_vars.is_empty() { - let mut urls = rpc_env_vars.split(',').collect::>(); - urls.shuffle(&mut rand::thread_rng()); + let urls = rpc_env_vars.split(',').collect::>(); next(&urls).to_string() } else if is_ws { format!("wss://eth-mainnet.g.alchemy.com/v2/{}", next(&ALCHEMY_KEYS))