From 4cf8141a52af76368ca2a1a3410a0bb93b59d2a7 Mon Sep 17 00:00:00 2001 From: Jakob Meier Date: Fri, 14 Jul 2023 17:20:03 +0200 Subject: [PATCH] chore(estimator): remove TTN read estimation (#9307) Since we have flat storage for reads, we no longer charge for touched trie nodes (TTN) on reads. Remove the gas estimation for it. More specifically, we used to estimate TTN cost as `max(read_ttn, write_ttn)` and therefore had 3 numbers reported. (read, write, combined). Now we only need a single number reported. The removed code (read TTN estimation) also didn't work anymore, as it didn't actually touch any trie nodes, and hence an assertion was triggered. ``` thread 'main' panicked at 'assertion failed: nodes_touched_delta as usize >= 2 * final_key_len - 10', runtime/runtime-params-estimator/src/trie.rs:118:5 stack backtrace: 0: rust_begin_unwind at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/std/src/panicking.rs:575:5 1: core::panicking::panic_fmt at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:64:14 2: core::panicking::panic at /rustc/9eb3afe9ebe9c7d2b84b71002d44f4a0edac95e0/library/core/src/panicking.rs:114:5 3: runtime_params_estimator::touching_trie_node_read 4: runtime_params_estimator::touching_trie_node 5: runtime_params_estimator::run_estimation 6: runtime_params_estimator::main ``` We "fix" it by removing the code. --- runtime/runtime-params-estimator/src/cost.rs | 20 ++----- .../src/estimator_context.rs | 1 - runtime/runtime-params-estimator/src/lib.rs | 23 ++------ .../src/transaction_builder.rs | 8 --- runtime/runtime-params-estimator/src/trie.rs | 54 ------------------- 5 files changed, 7 insertions(+), 99 deletions(-) diff --git a/runtime/runtime-params-estimator/src/cost.rs b/runtime/runtime-params-estimator/src/cost.rs index 4ac5bfed272..4913e96e749 100644 --- a/runtime/runtime-params-estimator/src/cost.rs +++ b/runtime/runtime-params-estimator/src/cost.rs @@ -546,8 +546,10 @@ pub enum Cost { /// `storage_write` or `storage_remove`. The fee is paid once for each /// unique trie node accessed. /// - /// Estimation: Take the maximum of estimations for `TouchingTrieNodeRead` - /// and `TouchingTrieNodeWrite` + /// Estimation: Prepare an account that has many keys stored that are + /// prefixes from each other. Then measure write cost for the shortest and + /// the longest key. The gas estimation difference is divided by the + /// difference of actually touched nodes. TouchingTrieNode, /// It is similar to `TouchingTrieNode`, but it is charged instead of this /// cost when we can guarantee that trie node is cached in memory, which @@ -562,20 +564,6 @@ pub enum Cost { /// for this are a bit involved but roughly speaking, it just forces values /// out of CPU caches so that they are always read from memory. ReadCachedTrieNode, - /// Helper estimation for `TouchingTrieNode` - /// - /// Estimation: Prepare an account that has many keys stored that are - /// prefixes from each other. Then measure access cost for the shortest and - /// the longest key. The gas estimation difference is divided by the - /// difference of actually touched nodes. - TouchingTrieNodeRead, - /// Helper estimation for `TouchingTrieNode` - /// - /// Estimation: Prepare an account that has many keys stored that are - /// prefixes from each other. Then measure write cost for the shortest and - /// the longest key. The gas estimation difference is divided by the - /// difference of actually touched nodes. - TouchingTrieNodeWrite, /// Estimates `promise_and_base` which is charged for every call to /// `promise_and`. This should cover the base cost for creating receipt /// dependencies. diff --git a/runtime/runtime-params-estimator/src/estimator_context.rs b/runtime/runtime-params-estimator/src/estimator_context.rs index 700fb311476..1885726e74a 100644 --- a/runtime/runtime-params-estimator/src/estimator_context.rs +++ b/runtime/runtime-params-estimator/src/estimator_context.rs @@ -43,7 +43,6 @@ pub(crate) struct CachedCosts { pub(crate) compile_cost_base_per_byte_v2: Option<(GasCost, GasCost)>, pub(crate) gas_metering_cost_base_per_op: Option<(GasCost, GasCost)>, pub(crate) apply_block: Option, - pub(crate) touching_trie_node_read: Option, pub(crate) touching_trie_node_write: Option, pub(crate) ed25519_verify_base: Option, } diff --git a/runtime/runtime-params-estimator/src/lib.rs b/runtime/runtime-params-estimator/src/lib.rs index d5986d5ad95..35e37370f33 100644 --- a/runtime/runtime-params-estimator/src/lib.rs +++ b/runtime/runtime-params-estimator/src/lib.rs @@ -244,8 +244,6 @@ static ALL_COSTS: &[(Cost, fn(&mut EstimatorContext) -> GasCost)] = &[ (Cost::StorageRemoveRetValueByte, storage_remove_ret_value_byte), (Cost::TouchingTrieNode, touching_trie_node), (Cost::ReadCachedTrieNode, read_cached_trie_node), - (Cost::TouchingTrieNodeRead, touching_trie_node_read), - (Cost::TouchingTrieNodeWrite, touching_trie_node_write), (Cost::ApplyBlock, apply_block_cost), (Cost::ContractCompileBase, contract_compile_base), (Cost::ContractCompileBytes, contract_compile_bytes), @@ -1168,24 +1166,9 @@ fn storage_remove_ret_value_byte(ctx: &mut EstimatorContext) -> GasCost { } fn touching_trie_node(ctx: &mut EstimatorContext) -> GasCost { - let read = touching_trie_node_read(ctx); - let write = touching_trie_node_write(ctx); - return std::cmp::max(read, write); -} - -fn touching_trie_node_read(ctx: &mut EstimatorContext) -> GasCost { - if let Some(cost) = ctx.cached.touching_trie_node_read.clone() { - return cost; - } - let warmup_iters = ctx.config.warmup_iters_per_block; - let measured_iters = ctx.config.iter_per_block; - // Number of bytes in the final key. Will create 2x that many nodes. - // Picked somewhat arbitrarily, balancing estimation time vs accuracy. - let final_key_len = 1000; - let cost = trie::read_node_from_db(ctx, warmup_iters, measured_iters, final_key_len); - - ctx.cached.touching_trie_node_read = Some(cost.clone()); - cost + // TTN write cost = TTN cost because we no longer charge it on reads since + // flat storage for reads was introduced + touching_trie_node_write(ctx) } fn touching_trie_node_write(ctx: &mut EstimatorContext) -> GasCost { diff --git a/runtime/runtime-params-estimator/src/transaction_builder.rs b/runtime/runtime-params-estimator/src/transaction_builder.rs index 7f680c83454..8c18d5e2f7a 100644 --- a/runtime/runtime-params-estimator/src/transaction_builder.rs +++ b/runtime/runtime-params-estimator/src/transaction_builder.rs @@ -100,14 +100,6 @@ impl TransactionBuilder { self.transaction_from_function_call(account, "account_storage_insert_key", arg) } - /// Transaction that checks existence of a given key under an account. - /// The account must have the test contract deployed. - pub(crate) fn account_has_key(&mut self, account: AccountId, key: &str) -> SignedTransaction { - let arg = (key.len() as u64).to_le_bytes().into_iter().chain(key.bytes()).collect(); - - self.transaction_from_function_call(account, "account_storage_has_key", arg) - } - pub(crate) fn rng(&mut self) -> ThreadRng { rand::thread_rng() } diff --git a/runtime/runtime-params-estimator/src/trie.rs b/runtime/runtime-params-estimator/src/trie.rs index 5b21233a52b..ca606bd96ff 100644 --- a/runtime/runtime-params-estimator/src/trie.rs +++ b/runtime/runtime-params-estimator/src/trie.rs @@ -5,7 +5,6 @@ use near_primitives::hash::hash; use near_primitives::types::TrieCacheMode; use near_store::{TrieCachingStorage, TrieStorage}; use near_vm_runner::logic::ExtCosts; -use std::iter; use std::sync::atomic::{AtomicUsize, Ordering}; static SINK: AtomicUsize = AtomicUsize::new(0); @@ -69,59 +68,6 @@ pub(crate) fn write_node( cost } -pub(crate) fn read_node_from_db( - ctx: &mut EstimatorContext, - warmup_iters: usize, - measured_iters: usize, - final_key_len: usize, -) -> GasCost { - let block_latency = 0; - let overhead = overhead_per_measured_block(ctx, block_latency); - let mut testbed = ctx.testbed(); - let tb = testbed.transaction_builder(); - // Prepare a long chain in the trie - let signer = tb.random_account(); - let key = "j".repeat(final_key_len); - let mut setup_block = Vec::new(); - for key_len in 0..final_key_len { - let key = &key.as_bytes()[..key_len]; - let value = b"0"; - setup_block.push(tb.account_insert_key(signer.clone(), key, value)); - } - let mut blocks = Vec::with_capacity(1 + 2 * warmup_iters + 2 * measured_iters); - blocks.push(setup_block); - blocks.extend( - iter::repeat_with(|| vec![tb.account_has_key(signer.clone(), &key[0..1])]) - .take(measured_iters + warmup_iters), - ); - blocks.extend( - iter::repeat_with(|| vec![tb.account_has_key(signer.clone(), &key)]) - .take(measured_iters + warmup_iters), - ); - let results = &testbed.measure_blocks(blocks, block_latency)[1..]; - let (short_key_results, long_key_results) = results.split_at(measured_iters + warmup_iters); - let (cost_short_key, ext_cost_short_key) = aggregate_per_block_measurements( - 1, - short_key_results[warmup_iters..].to_vec(), - Some(overhead.clone()), - ); - let (cost_long_key, ext_cost_long_key) = aggregate_per_block_measurements( - 1, - long_key_results[warmup_iters..].to_vec(), - Some(overhead), - ); - let nodes_touched_delta = ext_cost_long_key[&ExtCosts::touching_trie_node] - - ext_cost_short_key[&ExtCosts::touching_trie_node]; - // The exact number of touched nodes is a implementation that we don't want - // to test here but it should be close to 2*final_key_len - assert!(nodes_touched_delta as usize <= 2 * final_key_len + 10); - assert!(nodes_touched_delta as usize >= 2 * final_key_len - 10); - let cost_delta = - cost_long_key.saturating_sub(&cost_short_key, &NonNegativeTolerance::PER_MILLE); - let cost = cost_delta / nodes_touched_delta; - cost -} - pub(crate) fn read_node_from_chunk_cache(testbed: &mut Testbed) -> GasCost { let debug = testbed.config.debug; let iters = 200;