From 715f3a9acd7de9a14fc16e31822d0846b157a25b Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Wed, 13 Nov 2024 00:34:59 +0000 Subject: [PATCH] [comments] Addressing Igor's comments --- aptos-move/aptos-vm/src/lib.rs | 6 +++- .../block-executor/src/code_cache_global.rs | 1 + .../src/code_cache_global_manager.rs | 17 +++++++++-- aptos-move/block-executor/src/counters.rs | 28 +++++++++++++++++-- .../src/workflow/do_get_execution_output.rs | 1 + types/src/block_executor/config.rs | 7 +++-- 6 files changed, 52 insertions(+), 8 deletions(-) diff --git a/aptos-move/aptos-vm/src/lib.rs b/aptos-move/aptos-vm/src/lib.rs index 25f807a499f95..3f21e37f8e2b1 100644 --- a/aptos-move/aptos-vm/src/lib.rs +++ b/aptos-move/aptos-vm/src/lib.rs @@ -153,7 +153,11 @@ pub trait VMValidator { ) -> VMValidatorResult; } -/// This trait describes the block executor interface. +/// This trait describes the block executor interface which is responsible for executing a block of +/// transactions. In general, block execution returns a vector of transaction outputs. This vector +/// has the same length as the input vector of transactions. In case transactions are skipped or +/// discarded, they are still included - but their output is empty. The outputs are not applied to +/// the state directly. It is the responsibility of the caller to update the state accordingly. pub trait VMBlockExecutor: Send + Sync { /// Be careful if any state (such as caches) is kept in [VMBlockExecutor]. It is the /// responsibility of the implementation to ensure the state is valid across multiple diff --git a/aptos-move/block-executor/src/code_cache_global.rs b/aptos-move/block-executor/src/code_cache_global.rs index fef08f5f5c940..076dca358be7f 100644 --- a/aptos-move/block-executor/src/code_cache_global.rs +++ b/aptos-move/block-executor/src/code_cache_global.rs @@ -133,6 +133,7 @@ where /// 1. Only verified modules are inserted. /// 2. Valid modules should not be removed, and new modules should have unique ownership. If /// these constraints are violated, a panic error is returned. + // TODO(loader_v2): Use a trait for sync methods, and a concrete implementation for unsync. pub fn insert_verified_unsync( &self, modules: impl Iterator>)>, diff --git a/aptos-move/block-executor/src/code_cache_global_manager.rs b/aptos-move/block-executor/src/code_cache_global_manager.rs index e37187d36f0e7..f616a85703e76 100644 --- a/aptos-move/block-executor/src/code_cache_global_manager.rs +++ b/aptos-move/block-executor/src/code_cache_global_manager.rs @@ -1,7 +1,14 @@ // Copyright © Aptos Foundation // SPDX-License-Identifier: Apache-2.0 -use crate::{code_cache_global::GlobalModuleCache, explicit_sync_wrapper::ExplicitSyncWrapper}; +use crate::{ + code_cache_global::GlobalModuleCache, + counters::{ + GLOBAL_MODULE_CACHE_NUM_MODULES, GLOBAL_MODULE_CACHE_SIZE_IN_BYTES, + STRUCT_NAME_INDEX_MAP_NUM_ENTRIES, + }, + explicit_sync_wrapper::ExplicitSyncWrapper, +}; use aptos_types::block_executor::config::BlockExecutorModuleCacheLocalConfig; use aptos_vm_environment::environment::AptosEnvironment; use move_binary_format::errors::Location; @@ -150,13 +157,19 @@ where let struct_name_index_map_size = runtime_environment .struct_name_index_map_size() .map_err(|err| err.finish(Location::Undefined).into_vm_status())?; + STRUCT_NAME_INDEX_MAP_NUM_ENTRIES.set(struct_name_index_map_size as i64); + if struct_name_index_map_size > config.max_struct_name_index_map_num_entries { module_cache.flush_unsync(); runtime_environment.flush_struct_name_and_info_caches(); } // Check 2: If the module cache is too big, flush it. - if module_cache.size_in_bytes() > config.max_module_cache_size_in_bytes { + let module_cache_size_in_bytes = module_cache.size_in_bytes(); + GLOBAL_MODULE_CACHE_SIZE_IN_BYTES.set(module_cache_size_in_bytes as i64); + GLOBAL_MODULE_CACHE_NUM_MODULES.set(module_cache.num_modules() as i64); + + if module_cache_size_in_bytes > config.max_module_cache_size_in_bytes { module_cache.flush_unsync(); } diff --git a/aptos-move/block-executor/src/counters.rs b/aptos-move/block-executor/src/counters.rs index c3f1dc61f8fd3..e75d1b3354805 100644 --- a/aptos-move/block-executor/src/counters.rs +++ b/aptos-move/block-executor/src/counters.rs @@ -3,8 +3,8 @@ use aptos_metrics_core::{ exponential_buckets, register_avg_counter_vec, register_histogram, register_histogram_vec, - register_int_counter, register_int_counter_vec, Histogram, HistogramVec, IntCounter, - IntCounterVec, + register_int_counter, register_int_counter_vec, register_int_gauge, Histogram, HistogramVec, + IntCounter, IntCounterVec, IntGauge, }; use aptos_mvhashmap::BlockStateStats; use aptos_types::fee_statement::FeeStatement; @@ -333,3 +333,27 @@ pub(crate) fn update_state_counters(block_state_stats: BlockStateStats, is_paral .with_label_values(&[mode_str, "delayed_field"]) .observe(block_state_stats.base_delayed_fields_size as f64); } + +pub static GLOBAL_MODULE_CACHE_SIZE_IN_BYTES: Lazy = Lazy::new(|| { + register_int_gauge!( + "global_module_cache_size_in_bytes", + "Sum of sizes of all serialized modules stored in global module cache" + ) + .unwrap() +}); + +pub static GLOBAL_MODULE_CACHE_NUM_MODULES: Lazy = Lazy::new(|| { + register_int_gauge!( + "global_module_cache_num_modules", + "Number of modules cached in global module cache" + ) + .unwrap() +}); + +pub static STRUCT_NAME_INDEX_MAP_NUM_ENTRIES: Lazy = Lazy::new(|| { + register_int_gauge!( + "struct_name_index_map_num_entries", + "Number of struct names interned and cached in execution environment" + ) + .unwrap() +}); diff --git a/execution/executor/src/workflow/do_get_execution_output.rs b/execution/executor/src/workflow/do_get_execution_output.rs index 5e7987b95098f..5d2e962dff989 100644 --- a/execution/executor/src/workflow/do_get_execution_output.rs +++ b/execution/executor/src/workflow/do_get_execution_output.rs @@ -44,6 +44,7 @@ use std::{iter, sync::Arc}; pub struct DoGetExecutionOutput; impl DoGetExecutionOutput { + // Note: state checkpoint will be appended in when the current block is Some(..). pub fn by_transaction_execution( executor: &V, transactions: ExecutableTransactions, diff --git a/types/src/block_executor/config.rs b/types/src/block_executor/config.rs index 1342eba0de53f..ca578142e7295 100644 --- a/types/src/block_executor/config.rs +++ b/types/src/block_executor/config.rs @@ -22,9 +22,10 @@ impl Default for BlockExecutorModuleCacheLocalConfig { fn default() -> Self { Self { prefetch_framework_code: true, - // Use 50 Mb for now, should be large enough to cache many modules. - max_module_cache_size_in_bytes: 50 * 1024 * 1024, - max_struct_name_index_map_num_entries: 100_000, + // Use 512 Mb for now, should be large enough to cache all mainnet modules (at the time + // of writing this comment, 13.11.24). + max_module_cache_size_in_bytes: 512 * 1024 * 1024, + max_struct_name_index_map_num_entries: 1_000_000, } } }