[comments] Addressing Igor's comments

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

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<Item = (K, Arc<ModuleCode<D, V, E>>)>,

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();
         }
 

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<IntGauge> = 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<IntGauge> = 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<IntGauge> = Lazy::new(|| {
+    register_int_gauge!(
+        "struct_name_index_map_num_entries",
+        "Number of struct names interned and cached in execution environment"
+    )
+    .unwrap()
+});

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<V: VMBlockExecutor>(
         executor: &V,
         transactions: ExecutableTransactions,

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,
         }
     }
 }