From e274217aecbe4a77560d6861522a02c93f12cb53 Mon Sep 17 00:00:00 2001 From: Fraser Hutchison Date: Tue, 26 Nov 2024 12:47:31 +0000 Subject: [PATCH 1/3] fix to allow benchmarks to run on macOS --- crates/astria-sequencer/benches/benchmark.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/astria-sequencer/benches/benchmark.rs b/crates/astria-sequencer/benches/benchmark.rs index 57c6df628a..e08f2e9347 100644 --- a/crates/astria-sequencer/benches/benchmark.rs +++ b/crates/astria-sequencer/benches/benchmark.rs @@ -1,7 +1,14 @@ -// Required to force the benchmark target to actually register the divan benchmark cases. -use astria_sequencer as _; +//! To run all sequencer benchmarks, from the root of the monorepo, run: +//! ```sh +//! cargo bench --features=benchmark -qp astria-sequencer +//! ``` fn main() { + // Required to force the benchmark target to actually register the divan benchmark cases. + // See https://github.com/nvzqz/divan/issues/61#issuecomment-2500002168. + use config::Config as _; + let _ = astria_sequencer::Config::get(); + // Handle `nextest` querying the benchmark binary for tests. Currently `divan` is incompatible // with `nextest`, so just report no tests available. // See https://github.com/nvzqz/divan/issues/43 for further details. From a865a6d3c326e339893846a6e658462960732365 Mon Sep 17 00:00:00 2001 From: Fraser Hutchison Date: Fri, 30 Aug 2024 22:13:11 +0100 Subject: [PATCH 2/3] improve mempool benchmark initialization --- crates/astria-sequencer/src/app/benchmarks.rs | 10 +-- .../src/mempool/benchmarks.rs | 89 +++++++++++++------ crates/astria-sequencer/src/mempool/mod.rs | 13 +++ .../src/mempool/transactions_container.rs | 31 ++----- 4 files changed, 85 insertions(+), 58 deletions(-) diff --git a/crates/astria-sequencer/src/app/benchmarks.rs b/crates/astria-sequencer/src/app/benchmarks.rs index 0b6aa3ef2b..fa0ac4d4c3 100644 --- a/crates/astria-sequencer/src/app/benchmarks.rs +++ b/crates/astria-sequencer/src/app/benchmarks.rs @@ -17,8 +17,10 @@ use cnidarium::Storage; use crate::{ app::{ benchmark_and_test_utils::{ + initialize_app_with_storage, mock_balances, mock_tx_cost, + proto_genesis_state, }, App, }, @@ -65,16 +67,12 @@ impl Fixture { accounts, authority_sudo_address: first_address.clone(), ibc_sudo_address: first_address.clone(), - ..crate::app::benchmark_and_test_utils::proto_genesis_state() + ..proto_genesis_state() }, ) .unwrap(); - let (app, storage) = crate::app::benchmark_and_test_utils::initialize_app_with_storage( - Some(genesis_state), - vec![], - ) - .await; + let (app, storage) = initialize_app_with_storage(Some(genesis_state), vec![]).await; let mock_balances = mock_balances(0, 0); let mock_tx_cost = mock_tx_cost(0, 0, 0); diff --git a/crates/astria-sequencer/src/mempool/benchmarks.rs b/crates/astria-sequencer/src/mempool/benchmarks.rs index 3caaa920d7..abada300d3 100644 --- a/crates/astria-sequencer/src/mempool/benchmarks.rs +++ b/crates/astria-sequencer/src/mempool/benchmarks.rs @@ -100,36 +100,50 @@ fn transactions() -> &'static Vec> { /// Returns a new `Mempool` initialized with the number of transactions specified by `T::size()` /// taken from the static `transactions()`, and with a full `comet_bft_removal_cache`. fn init_mempool() -> Mempool { + static CELL_100: std::sync::OnceLock = std::sync::OnceLock::new(); + static CELL_1_000: std::sync::OnceLock = std::sync::OnceLock::new(); + static CELL_10_000: std::sync::OnceLock = std::sync::OnceLock::new(); + static CELL_100_000: std::sync::OnceLock = std::sync::OnceLock::new(); let runtime = tokio::runtime::Builder::new_current_thread() .enable_all() .build() .unwrap(); - let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap())); - let mempool = Mempool::new(metrics, T::size()); - let account_mock_balance = mock_balances(0, 0); - let tx_mock_cost = mock_tx_cost(0, 0, 0); - runtime.block_on(async { - for tx in transactions().iter().take(T::checked_size()) { - mempool - .insert( - tx.clone(), - 0, - account_mock_balance.clone(), - tx_mock_cost.clone(), - ) - .await - .unwrap(); - } - for i in 0..super::REMOVAL_CACHE_SIZE { - let hash = Sha256::digest(i.to_le_bytes()).into(); - mempool - .comet_bft_removal_cache - .write() - .await - .add(hash, RemovalReason::Expired); - } - }); - mempool + let init = || { + let metrics = Box::leak(Box::new(Metrics::noop_metrics(&()).unwrap())); + let mempool = Mempool::new(metrics, T::size()); + runtime.block_on(async { + let account_mock_balance = mock_balances(0, 0); + let tx_mock_cost = mock_tx_cost(0, 0, 0); + for tx in transactions().iter().take(T::checked_size()) { + mempool + .insert( + tx.clone(), + 0, + account_mock_balance.clone(), + tx_mock_cost.clone(), + ) + .await + .unwrap(); + } + for i in 0..super::REMOVAL_CACHE_SIZE { + let hash = Sha256::digest(i.to_le_bytes()).into(); + mempool + .comet_bft_removal_cache + .write() + .await + .add(hash, RemovalReason::Expired); + } + }); + mempool + }; + let mempool = match T::checked_size() { + 100 => CELL_100.get_or_init(init), + 1_000 => CELL_1_000.get_or_init(init), + 10_000 => CELL_10_000.get_or_init(init), + 100_000 => CELL_100_000.get_or_init(init), + _ => unreachable!(), + }; + runtime.block_on(async { mempool.deep_clone().await }) } /// Returns the first transaction from the static `transactions()` not included in the initialized @@ -138,6 +152,29 @@ fn get_unused_tx() -> Arc { transactions().get(T::checked_size()).unwrap().clone() } +/// This is not really a benchmark test, rather a means to memoize the data used in the "real" +/// benchmark tests below. +/// +/// It should always be named so that it is alphabetically first in the suite of tests, since the +/// tests are run in that order. +/// +/// This means that all the real tests are able to have a meaningful number of iterations, making +/// the results more accurate, rather than one test only having a single iteration due to the time +/// taken to memoize the data. +#[divan::bench( + max_time = MAX_TIME, + types = [ + mempool_with_100_txs, + mempool_with_1000_txs, + mempool_with_10000_txs, + mempool_with_100000_txs + ] +)] +fn a_warmup(bencher: divan::Bencher) { + init_mempool::(); + bencher.bench(|| ()); +} + /// Benchmarks `Mempool::insert` for a single new transaction on a mempool with the given number of /// existing entries. #[divan::bench( diff --git a/crates/astria-sequencer/src/mempool/mod.rs b/crates/astria-sequencer/src/mempool/mod.rs index 68579620f4..3822bab8e6 100644 --- a/crates/astria-sequencer/src/mempool/mod.rs +++ b/crates/astria-sequencer/src/mempool/mod.rs @@ -497,6 +497,19 @@ impl Mempool { let parked = self.parked.write().await; (pending, parked) } + + #[cfg(feature = "benchmark")] + pub(crate) async fn deep_clone(&self) -> Self { + Mempool { + pending: Arc::new(RwLock::new(self.pending.read().await.clone())), + parked: Arc::new(RwLock::new(self.parked.read().await.clone())), + comet_bft_removal_cache: Arc::new(RwLock::new( + self.comet_bft_removal_cache.read().await.clone(), + )), + contained_txs: Arc::new(RwLock::new(self.contained_txs.read().await.clone())), + metrics: self.metrics, + } + } } #[cfg(test)] diff --git a/crates/astria-sequencer/src/mempool/transactions_container.rs b/crates/astria-sequencer/src/mempool/transactions_container.rs index 95d17b00c1..18d5bd5696 100644 --- a/crates/astria-sequencer/src/mempool/transactions_container.rs +++ b/crates/astria-sequencer/src/mempool/transactions_container.rs @@ -106,10 +106,6 @@ impl TimemarkedTransaction { &self.address } - pub(super) fn cost(&self) -> &HashMap { - &self.cost - } - pub(super) fn id(&self) -> [u8; 32] { self.tx_hash } @@ -217,28 +213,11 @@ impl PendingTransactionsForAccount { ) -> Vec { let mut split_at = 0; - 'outer: for (nonce, tx) in &self.txs { + for (nonce, tx) in &self.txs { // ensure we have enough balance to cover inclusion - for (denom, cost) in tx.cost() { - if *cost == 0 { - continue; - } - match available_balances.entry(*denom) { - hash_map::Entry::Occupied(mut entry) => { - // try to subtract cost, if not enough balance, do not include - let current_balance = entry.get_mut(); - *current_balance = match current_balance.checked_sub(*cost) { - None => break 'outer, - Some(new_value) => new_value, - }; - } - hash_map::Entry::Vacant(_) => { - // not enough balance, do not include - break 'outer; - } - } + if tx.deduct_costs(&mut available_balances).is_err() { + break; } - split_at = nonce.saturating_add(1); } @@ -1712,7 +1691,7 @@ mod tests { .txs .get(&0) .unwrap() - .cost() + .cost .get(&denom_0().to_ibc_prefixed()) .unwrap(), &0, @@ -1737,7 +1716,7 @@ mod tests { .txs .get(&0) .unwrap() - .cost() + .cost .get(&denom_0().to_ibc_prefixed()) .unwrap(), &MOCK_SEQUENCE_FEE, From 755f160a1cbaedfc0aa66e897b1879909f3a02ba Mon Sep 17 00:00:00 2001 From: Fraser Hutchison Date: Mon, 2 Dec 2024 17:49:26 +0000 Subject: [PATCH 3/3] fix mempool benchmarks --- crates/astria-sequencer/src/benchmark_utils.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/astria-sequencer/src/benchmark_utils.rs b/crates/astria-sequencer/src/benchmark_utils.rs index 3d0769eccf..ef7ad699a6 100644 --- a/crates/astria-sequencer/src/benchmark_utils.rs +++ b/crates/astria-sequencer/src/benchmark_utils.rs @@ -97,6 +97,7 @@ fn rollup_data_submission_actions() -> Vec> { .try_build() .expect("failed to build transaction from actions") .sign(signing_key); + *nonce = (*nonce).wrapping_add(1); Arc::new(tx) }) .take(SEQUENCE_ACTION_TX_COUNT)