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 be1df2dfe7ce9f..38d37b90cec066 100644 --- a/aptos-move/block-executor/src/code_cache_global_manager.rs +++ b/aptos-move/block-executor/src/code_cache_global_manager.rs @@ -122,7 +122,7 @@ where // If the environment caches too many struct names, flush type caches. Also flush module // caches because they contain indices for struct names. if struct_name_index_map_size > config.max_struct_name_index_map_num_entries { - runtime_environment.flush_struct_name_and_info_caches(); + runtime_environment.flush_struct_name_and_tag_caches(); self.module_cache.flush(); } diff --git a/aptos-move/block-executor/src/executor.rs b/aptos-move/block-executor/src/executor.rs index dbe8fc03e6fd8f..2bacee3ae3ff83 100644 --- a/aptos-move/block-executor/src/executor.rs +++ b/aptos-move/block-executor/src/executor.rs @@ -1734,7 +1734,7 @@ where module_cache_manager_guard .environment() .runtime_environment() - .flush_struct_name_and_info_caches(); + .flush_struct_name_and_tag_caches(); module_cache_manager_guard.module_cache_mut().flush(); info!("parallel execution requiring fallback"); diff --git a/third_party/move/move-vm/runtime/src/interpreter.rs b/third_party/move/move-vm/runtime/src/interpreter.rs index 43918cf9d797f3..2af79dc9288852 100644 --- a/third_party/move/move-vm/runtime/src/interpreter.rs +++ b/third_party/move/move-vm/runtime/src/interpreter.rs @@ -28,6 +28,7 @@ use move_core_types::{ language_storage::{ModuleId, TypeTag}, vm_status::{StatusCode, StatusType}, }; +use move_vm_metrics::{Timer, VM_TIMER}; use move_vm_types::{ debug_write, debug_writeln, gas::{GasMeter, SimpleInstruction}, @@ -45,7 +46,7 @@ use move_vm_types::{ use std::{ cell::RefCell, cmp::min, - collections::{btree_map, BTreeSet, VecDeque}, + collections::{btree_map, BTreeSet, HashMap, VecDeque}, fmt::Write, rc::Rc, }; @@ -1541,6 +1542,8 @@ impl CallStack { } fn check_depth_of_type(resolver: &Resolver, ty: &Type) -> PartialVMResult<()> { + let _timer = VM_TIMER.timer_with_label("Interpreter::check_depth_of_type"); + // Start at 1 since we always call this right before we add a new node to the value's depth. let max_depth = match resolver.vm_config().max_value_nest_depth { Some(max_depth) => max_depth, @@ -1589,6 +1592,7 @@ fn check_depth_of_type_impl( *idx, resolver.module_store(), resolver.module_storage(), + &mut HashMap::new(), )?; check_depth!(formula.solve(&[])) }, @@ -1606,6 +1610,7 @@ fn check_depth_of_type_impl( *idx, resolver.module_store(), resolver.module_storage(), + &mut HashMap::new(), )?; check_depth!(formula.solve(&ty_arg_depths)) }, diff --git a/third_party/move/move-vm/runtime/src/loader/mod.rs b/third_party/move/move-vm/runtime/src/loader/mod.rs index fe5ec818db599e..8efc90f3640592 100644 --- a/third_party/move/move-vm/runtime/src/loader/mod.rs +++ b/third_party/move/move-vm/runtime/src/loader/mod.rs @@ -40,7 +40,7 @@ use move_vm_types::{ }; use parking_lot::{Mutex, RwLock}; use std::{ - collections::{btree_map, BTreeMap, BTreeSet}, + collections::{btree_map, BTreeMap, BTreeSet, HashMap}, hash::Hash, sync::Arc, }; @@ -57,8 +57,10 @@ use crate::{ loader::modules::{StructVariantInfo, VariantFieldInfo}, native_functions::NativeFunctions, storage::{ - loader::LoaderV2, module_storage::FunctionValueExtensionAdapter, ty_cache::StructInfoCache, - ty_layout_converter::LoaderLayoutConverter, ty_tag_converter::TypeTagConverter, + loader::LoaderV2, + module_storage::FunctionValueExtensionAdapter, + ty_layout_converter::LoaderLayoutConverter, + ty_tag_converter::{PricedStructTag, TypeTagCache, TypeTagConverter}, }, }; pub use function::{Function, LoadedFunction}; @@ -153,16 +155,6 @@ impl Loader { versioned_loader_getter!(ty_builder, TypeBuilder); - pub(crate) fn ty_cache<'a>( - &'a self, - module_storage: &'a dyn ModuleStorage, - ) -> &StructInfoCache { - match self { - Self::V1(loader) => &loader.type_cache, - Self::V2(_) => module_storage.runtime_environment().ty_cache(), - } - } - pub(crate) fn struct_name_index_map<'a>( &'a self, module_storage: &'a dyn ModuleStorage, @@ -176,7 +168,7 @@ impl Loader { pub(crate) fn v1(natives: NativeFunctions, vm_config: VMConfig) -> Self { Self::V1(LoaderV1 { scripts: RwLock::new(ScriptCache::new()), - type_cache: StructInfoCache::empty(), + type_cache: TypeTagCache::empty(), name_cache: StructNameIndexMap::empty(), natives, invalidated: RwLock::new(false), @@ -378,7 +370,7 @@ impl Loader { // The `pub(crate)` API is what a Loader offers to the runtime. pub(crate) struct LoaderV1 { scripts: RwLock, - type_cache: StructInfoCache, + type_cache: TypeTagCache, natives: NativeFunctions, pub(crate) name_cache: StructNameIndexMap, @@ -1727,9 +1719,9 @@ impl LoaderV1 { ty_args: &[Type], gas_context: &mut PseudoGasContext, ) -> PartialVMResult { - if let Some((struct_tag, gas)) = self.type_cache.get_struct_tag(&struct_name_idx, ty_args) { - gas_context.charge(gas)?; - return Ok(struct_tag.clone()); + if let Some(priced_tag) = self.type_cache.get_struct_tag(&struct_name_idx, ty_args) { + gas_context.charge(priced_tag.pseudo_gas_cost)?; + return Ok(priced_tag.struct_tag); } let cur_cost = gas_context.current_cost(); @@ -1743,13 +1735,14 @@ impl LoaderV1 { .idx_to_struct_tag(struct_name_idx, type_args)?; gas_context.charge_struct_tag(&struct_tag)?; - self.type_cache.store_struct_tag( - struct_name_idx, - ty_args.to_vec(), - struct_tag.clone(), - gas_context.current_cost() - cur_cost, - ); - Ok(struct_tag) + + let priced_tag = PricedStructTag { + struct_tag, + pseudo_gas_cost: gas_context.current_cost() - cur_cost, + }; + self.type_cache + .insert_struct_tag(&struct_name_idx, ty_args, &priced_tag); + Ok(priced_tag.struct_tag) } pub(crate) fn type_to_type_tag_impl( @@ -1813,10 +1806,10 @@ impl Loader { struct_name_idx: StructNameIndex, module_store: &LegacyModuleStorageAdapter, module_storage: &dyn ModuleStorage, + visited_cache: &mut HashMap, ) -> PartialVMResult { - let ty_cache = self.ty_cache(module_storage); - if let Some(depth_formula) = ty_cache.get_depth_formula(&struct_name_idx) { - return Ok(depth_formula); + if let Some(depth_formula) = visited_cache.get(&struct_name_idx) { + return Ok(depth_formula.clone()); } let struct_type = @@ -1825,22 +1818,46 @@ impl Loader { StructLayout::Single(fields) => fields .iter() .map(|(_, field_ty)| { - self.calculate_depth_of_type(field_ty, module_store, module_storage) + self.calculate_depth_of_type( + field_ty, + module_store, + module_storage, + visited_cache, + ) }) .collect::>>()?, StructLayout::Variants(variants) => variants .iter() .flat_map(|variant| variant.1.iter().map(|(_, ty)| ty)) .map(|field_ty| { - self.calculate_depth_of_type(field_ty, module_store, module_storage) + self.calculate_depth_of_type( + field_ty, + module_store, + module_storage, + visited_cache, + ) }) .collect::>>()?, }; let formula = DepthFormula::normalize(formulas); - - let struct_name_index_map = self.struct_name_index_map(module_storage); - ty_cache.store_depth_formula(struct_name_idx, struct_name_index_map, &formula)?; + if visited_cache + .insert(struct_name_idx, formula.clone()) + .is_some() + { + // Same thread has put this entry previously, which means there is a recursion. + let struct_name = self + .struct_name_index_map(module_storage) + .idx_to_struct_name_ref(struct_name_idx)?; + return Err( + PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR).with_message( + format!( + "Depth formula for struct '{}' is already cached by the same thread", + struct_name.as_ref(), + ), + ), + ); + } Ok(formula) } @@ -1849,6 +1866,7 @@ impl Loader { ty: &Type, module_store: &LegacyModuleStorageAdapter, module_storage: &dyn ModuleStorage, + visited_cache: &mut HashMap, ) -> PartialVMResult { Ok(match ty { Type::Bool @@ -1861,19 +1879,25 @@ impl Loader { | Type::U32 | Type::U256 => DepthFormula::constant(1), Type::Vector(ty) => { - let mut inner = self.calculate_depth_of_type(ty, module_store, module_storage)?; + let mut inner = + self.calculate_depth_of_type(ty, module_store, module_storage, visited_cache)?; inner.scale(1); inner }, Type::Reference(ty) | Type::MutableReference(ty) => { - let mut inner = self.calculate_depth_of_type(ty, module_store, module_storage)?; + let mut inner = + self.calculate_depth_of_type(ty, module_store, module_storage, visited_cache)?; inner.scale(1); inner }, Type::TyParam(ty_idx) => DepthFormula::type_parameter(*ty_idx), Type::Struct { idx, .. } => { - let mut struct_formula = - self.calculate_depth_of_struct(*idx, module_store, module_storage)?; + let mut struct_formula = self.calculate_depth_of_struct( + *idx, + module_store, + module_storage, + visited_cache, + )?; debug_assert!(struct_formula.terms.is_empty()); struct_formula.scale(1); struct_formula @@ -1886,12 +1910,21 @@ impl Loader { let var = idx as TypeParameterIndex; Ok(( var, - self.calculate_depth_of_type(ty, module_store, module_storage)?, + self.calculate_depth_of_type( + ty, + module_store, + module_storage, + visited_cache, + )?, )) }) .collect::>>()?; - let struct_formula = - self.calculate_depth_of_struct(*idx, module_store, module_storage)?; + let struct_formula = self.calculate_depth_of_struct( + *idx, + module_store, + module_storage, + visited_cache, + )?; let mut subst_struct_formula = struct_formula.subst(ty_arg_map)?; subst_struct_formula.scale(1); subst_struct_formula @@ -1900,45 +1933,6 @@ impl Loader { } } -// Public APIs for external uses. -impl Loader { - pub(crate) fn get_type_layout( - &self, - type_tag: &TypeTag, - move_storage: &mut TransactionDataCache, - module_storage_adapter: &LegacyModuleStorageAdapter, - module_storage: &impl ModuleStorage, - ) -> VMResult { - let ty = self.load_type( - type_tag, - move_storage, - module_storage_adapter, - module_storage, - )?; - LoaderLayoutConverter::new(self, module_storage_adapter, module_storage) - .type_to_type_layout(&ty) - .map_err(|e| e.finish(Location::Undefined)) - } - - pub(crate) fn get_fully_annotated_type_layout( - &self, - type_tag: &TypeTag, - move_storage: &mut TransactionDataCache, - module_storage_adapter: &LegacyModuleStorageAdapter, - module_storage: &impl ModuleStorage, - ) -> VMResult { - let ty = self.load_type( - type_tag, - move_storage, - module_storage_adapter, - module_storage, - )?; - LoaderLayoutConverter::new(self, module_storage_adapter, module_storage) - .type_to_fully_annotated_layout(&ty) - .map_err(|e| e.finish(Location::Undefined)) - } -} - // Matches the actual returned type to the expected type, binding any type args to the // necessary type as stored in the map. The expected type must be a concrete type (no TyParam). // Returns true if a successful match is made. diff --git a/third_party/move/move-vm/runtime/src/session.rs b/third_party/move/move-vm/runtime/src/session.rs index 831c1e93e696d4..5da9e3c3662f0e 100644 --- a/third_party/move/move-vm/runtime/src/session.rs +++ b/third_party/move/move-vm/runtime/src/session.rs @@ -441,35 +441,6 @@ impl<'r, 'l> Session<'r, 'l> { ) } - pub fn get_type_layout( - &mut self, - type_tag: &TypeTag, - module_storage: &impl ModuleStorage, - ) -> VMResult { - self.move_vm.runtime.loader().get_type_layout( - type_tag, - &mut self.data_cache, - &self.module_store, - module_storage, - ) - } - - pub fn get_fully_annotated_type_layout( - &mut self, - type_tag: &TypeTag, - module_storage: &impl ModuleStorage, - ) -> VMResult { - self.move_vm - .runtime - .loader() - .get_fully_annotated_type_layout( - type_tag, - &mut self.data_cache, - &self.module_store, - module_storage, - ) - } - pub fn get_type_tag( &self, ty: &Type, @@ -482,6 +453,8 @@ impl<'r, 'l> Session<'r, 'l> { .map_err(|e| e.finish(Location::Undefined)) } + /// Returns [MoveTypeLayout] of a given runtime type. For [TypeTag] to layout conversions, one + /// needs to first convert the tag to runtime type. pub fn get_type_layout_from_ty( &self, ty: &Type, diff --git a/third_party/move/move-vm/runtime/src/storage/environment.rs b/third_party/move/move-vm/runtime/src/storage/environment.rs index 057b1be157fb91..9a72e0d482221c 100644 --- a/third_party/move/move-vm/runtime/src/storage/environment.rs +++ b/third_party/move/move-vm/runtime/src/storage/environment.rs @@ -5,10 +5,7 @@ use crate::{ config::VMConfig, loader::check_natives, native_functions::{NativeFunction, NativeFunctions}, - storage::{ - ty_cache::StructInfoCache, ty_tag_converter::TypeTagCache, - verified_module_cache::VERIFIED_MODULES_V2, - }, + storage::{ty_tag_converter::TypeTagCache, verified_module_cache::VERIFIED_MODULES_V2}, Module, Script, }; use ambassador::delegatable_trait; @@ -58,27 +55,6 @@ pub struct RuntimeEnvironment { /// Caches struct tags for instantiated types. This cache can be used concurrently and /// speculatively because type tag information does not change with module publishes. ty_tag_cache: Arc, - - /// Type cache for struct layouts, tags and depths, shared across multiple threads. - /// - /// SAFETY: - /// Here we informally show that it is safe to share type cache across multiple threads. - /// - /// 1) Struct has been already published. - /// In this case, it is fine to have multiple transactions concurrently accessing and - /// caching struct tags, layouts and depth formulas. Even if transaction failed due to - /// speculation, and is re-executed later, the speculative aborted execution cached a non- - /// speculative existing struct information. It is safe for other threads to access it. - /// - /// 2) Struct is being published with a module. - /// The design of V2 loader ensures that when modules are published, i.e., staged on top of - /// the existing module storage, the runtime environment is cloned. Hence, it is not even - /// possible to mutate this global cache speculatively. - /// Importantly, this SHOULD NOT be mutated by speculative module publish. - // TODO(loader_v2): - // Provide a generic (trait) implementation for clients to implement their own type caching - // logic. - ty_cache: StructInfoCache, } impl RuntimeEnvironment { @@ -108,7 +84,6 @@ impl RuntimeEnvironment { natives, struct_name_index_map: Arc::new(StructNameIndexMap::empty()), ty_tag_cache: Arc::new(TypeTagCache::empty()), - ty_cache: StructInfoCache::empty(), } } @@ -275,30 +250,16 @@ impl RuntimeEnvironment { &self.ty_tag_cache } - /// Returns the type cache owned by this runtime environment which stores information about - /// struct layouts, tags and depth formulae. - pub(crate) fn ty_cache(&self) -> &StructInfoCache { - &self.ty_cache - } - /// Returns the size of the struct name re-indexing cache. Can be used to bound the size of the /// cache at block boundaries. pub fn struct_name_index_map_size(&self) -> PartialVMResult { self.struct_name_index_map.checked_len() } - /// Flushes the struct information (type and tag) caches. Flushing this cache does not - /// invalidate struct name index map or module cache. - pub fn flush_struct_info_cache(&self) { + /// Flushes the global caches with struct name indices and struct tags. Note that when calling + /// this function, modules that still store indices into struct name cache must also be flushed. + pub fn flush_struct_name_and_tag_caches(&self) { self.ty_tag_cache.flush(); - self.ty_cache.flush(); - } - - /// Flushes the global caches with struct name indices and the struct information. Note that - /// when calling this function, modules that still store indices into struct name cache must - /// also be invalidated. - pub fn flush_struct_name_and_info_caches(&self) { - self.flush_struct_info_cache(); self.struct_name_index_map.flush(); } @@ -326,7 +287,6 @@ impl Clone for RuntimeEnvironment { Self { vm_config: self.vm_config.clone(), natives: self.natives.clone(), - ty_cache: self.ty_cache.clone(), struct_name_index_map: Arc::clone(&self.struct_name_index_map), ty_tag_cache: Arc::clone(&self.ty_tag_cache), } diff --git a/third_party/move/move-vm/runtime/src/storage/mod.rs b/third_party/move/move-vm/runtime/src/storage/mod.rs index 72e42aa2f7ff48..6e8a73e058ac77 100644 --- a/third_party/move/move-vm/runtime/src/storage/mod.rs +++ b/third_party/move/move-vm/runtime/src/storage/mod.rs @@ -2,7 +2,6 @@ // SPDX-License-Identifier: Apache-2.0 pub(crate) mod loader; -pub(crate) mod ty_cache; pub(crate) mod ty_tag_converter; mod verified_module_cache; diff --git a/third_party/move/move-vm/runtime/src/storage/ty_cache.rs b/third_party/move/move-vm/runtime/src/storage/ty_cache.rs deleted file mode 100644 index 53091a720cef9a..00000000000000 --- a/third_party/move/move-vm/runtime/src/storage/ty_cache.rs +++ /dev/null @@ -1,252 +0,0 @@ -// Copyright (c) The Move Contributors -// SPDX-License-Identifier: Apache-2.0 - -use move_binary_format::errors::{PartialVMError, PartialVMResult}; -use move_core_types::{language_storage::StructTag, value::MoveTypeLayout, vm_status::StatusCode}; -use move_vm_types::loaded_data::{ - runtime_types::{DepthFormula, Type}, - struct_name_indexing::{StructNameIndex, StructNameIndexMap}, -}; -use parking_lot::RwLock; - -/// Layout information of a single struct instantiation. -#[derive(Clone)] -struct StructLayoutInfo { - /// Layout of this struct instantiation. - struct_layout: MoveTypeLayout, - /// Number of nodes in the type layout. - node_count: u64, - /// True if this struct contains delayed fields, e.g., aggregators. - has_identifier_mappings: bool, -} - -impl StructLayoutInfo { - fn unpack(self) -> (MoveTypeLayout, u64, bool) { - ( - self.struct_layout, - self.node_count, - self.has_identifier_mappings, - ) - } -} - -/// Struct instantiation information included in [StructInfo]. -#[derive(Clone)] -struct StructInstantiationInfo { - /// Struct tag of this struct instantiation, and its pseudo-gas cost. - struct_tag: Option<(StructTag, u64)>, - /// Runtime struct layout information. - struct_layout_info: Option, - /// Annotated struct layout information: layout with the node count. - annotated_struct_layout_info: Option<(MoveTypeLayout, u64)>, -} - -impl StructInstantiationInfo { - fn none() -> Self { - Self { - struct_tag: None, - struct_layout_info: None, - annotated_struct_layout_info: None, - } - } -} - -/// Cached information for any struct type. Caches information about its instantiations as well if -/// the struct is generic. -#[derive(Clone)] -struct StructInfo { - /// Depth formula of a possibly generic struct, together with a thread id that cached it. If - /// the formula is not yet cached, [None] is stored. - depth_formula: Option<(DepthFormula, std::thread::ThreadId)>, - /// Cached information for different struct instantiations. - instantiation_info: hashbrown::HashMap, StructInstantiationInfo>, -} - -impl StructInfo { - /// Returns an empty struct information. - pub(crate) fn none() -> Self { - Self { - depth_formula: None, - instantiation_info: hashbrown::HashMap::new(), - } - } -} - -/// A thread-safe struct information cache that can be used by the VM to store information about -/// structs, such as their depth formulae, tags, layouts. -pub(crate) struct StructInfoCache(RwLock>); - -impl StructInfoCache { - /// Returns an empty struct information cache. - pub(crate) fn empty() -> Self { - Self(RwLock::new(hashbrown::HashMap::new())) - } - - /// Flushes the cached struct information. - pub(crate) fn flush(&self) { - self.0.write().clear() - } - - /// Returns the depth formula associated with a struct, or [None] if it has not been cached. - pub(crate) fn get_depth_formula(&self, idx: &StructNameIndex) -> Option { - Some(self.0.read().get(idx)?.depth_formula.as_ref()?.0.clone()) - } - - /// Caches the depth formula, returning an error if the same thread has cached it before (i.e., - /// a recursive type has been found). - pub(crate) fn store_depth_formula( - &self, - idx: StructNameIndex, - struct_name_index_map: &StructNameIndexMap, - depth_formula: &DepthFormula, - ) -> PartialVMResult<()> { - let mut cache = self.0.write(); - let struct_info = cache.entry(idx).or_insert_with(StructInfo::none); - - // Cache the formula if it has not been cached, and otherwise return the thread id of the - // previously cached one. Release the write lock. - let id = std::thread::current().id(); - let prev_id = match &struct_info.depth_formula { - None => { - struct_info.depth_formula = Some((depth_formula.clone(), id)); - return Ok(()); - }, - Some((_, prev_id)) => *prev_id, - }; - drop(cache); - - if id == prev_id { - // Same thread has put this entry previously, which means there is a recursion. - let struct_name = struct_name_index_map.idx_to_struct_name_ref(idx)?; - return Err( - PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR).with_message( - format!( - "Depth formula for struct '{}' is already cached by the same thread", - struct_name.as_ref(), - ), - ), - ); - } - - Ok(()) - } - - /// Returns cached struct tag and its pseudo-gas cost if it exists, and [None] otherwise. - pub(crate) fn get_struct_tag( - &self, - idx: &StructNameIndex, - ty_args: &[Type], - ) -> Option<(StructTag, u64)> { - self.0 - .read() - .get(idx)? - .instantiation_info - .get(ty_args)? - .struct_tag - .as_ref() - .cloned() - } - - /// Caches annotated struct tag and its pseudo-gas cost. - pub(crate) fn store_struct_tag( - &self, - idx: StructNameIndex, - ty_args: Vec, - struct_tag: StructTag, - cost: u64, - ) { - self.0 - .write() - .entry(idx) - .or_insert_with(StructInfo::none) - .instantiation_info - .entry(ty_args) - .or_insert_with(StructInstantiationInfo::none) - .struct_tag = Some((struct_tag, cost)); - } - - /// Returns struct layout information if it has been cached, and [None] otherwise. - pub(crate) fn get_struct_layout_info( - &self, - idx: &StructNameIndex, - ty_args: &[Type], - ) -> Option<(MoveTypeLayout, u64, bool)> { - self.0 - .read() - .get(idx)? - .instantiation_info - .get(ty_args)? - .struct_layout_info - .as_ref() - .map(|info| info.clone().unpack()) - } - - /// Caches struct layout information. - pub(crate) fn store_struct_layout_info( - &self, - idx: StructNameIndex, - ty_args: Vec, - struct_layout: MoveTypeLayout, - node_count: u64, - has_identifier_mappings: bool, - ) { - let mut cache = self.0.write(); - let info = cache - .entry(idx) - .or_insert_with(StructInfo::none) - .instantiation_info - .entry(ty_args) - .or_insert_with(StructInstantiationInfo::none); - info.struct_layout_info = Some(StructLayoutInfo { - struct_layout, - node_count, - has_identifier_mappings, - }); - } - - /// Returns annotated struct layout information if it has been cached, and [None] otherwise. - pub(crate) fn get_annotated_struct_layout_info( - &self, - idx: &StructNameIndex, - ty_args: &[Type], - ) -> Option<(MoveTypeLayout, u64)> { - self.0 - .read() - .get(idx)? - .instantiation_info - .get(ty_args)? - .annotated_struct_layout_info - .as_ref() - .cloned() - } - - /// Caches annotated struct layout information. - pub(crate) fn store_annotated_struct_layout_info( - &self, - idx: StructNameIndex, - ty_args: Vec, - struct_layout: MoveTypeLayout, - node_count: u64, - ) { - let mut cache = self.0.write(); - let info = cache - .entry(idx) - .or_insert_with(StructInfo::none) - .instantiation_info - .entry(ty_args) - .or_insert_with(StructInstantiationInfo::none); - info.annotated_struct_layout_info = Some((struct_layout, node_count)); - } -} - -impl Clone for StructInfoCache { - fn clone(&self) -> Self { - Self(RwLock::new(self.0.read().clone())) - } -} - -#[cfg(test)] -mod test { - // TODO(loader_v2): - // This has never been tested before, so we definitely should be adding tests. -} diff --git a/third_party/move/move-vm/runtime/src/storage/ty_layout_converter.rs b/third_party/move/move-vm/runtime/src/storage/ty_layout_converter.rs index b65abc96e70abf..863be0060b5b32 100644 --- a/third_party/move/move-vm/runtime/src/storage/ty_layout_converter.rs +++ b/third_party/move/move-vm/runtime/src/storage/ty_layout_converter.rs @@ -4,7 +4,7 @@ use crate::{ config::VMConfig, loader::{LegacyModuleStorageAdapter, Loader, PseudoGasContext, VALUE_DEPTH_MAX}, - storage::{ty_cache::StructInfoCache, ty_tag_converter::TypeTagConverter}, + storage::ty_tag_converter::TypeTagConverter, ModuleStorage, }; use move_binary_format::errors::{PartialVMError, PartialVMResult}; @@ -13,6 +13,7 @@ use move_core_types::{ value::{IdentifierMappingKind, MoveFieldLayout, MoveStructLayout, MoveTypeLayout}, vm_status::StatusCode, }; +use move_vm_metrics::{Timer, VM_TIMER}; use move_vm_types::loaded_data::{ runtime_types::{StructLayout, StructType, Type}, struct_name_indexing::{StructNameIndex, StructNameIndexMap}, @@ -28,6 +29,8 @@ const MAX_TYPE_TO_LAYOUT_NODES: u64 = 256; pub trait LayoutConverter: LayoutConverterBase { /// Converts a runtime type to a type layout. fn type_to_type_layout(&self, ty: &Type) -> PartialVMResult { + let _timer = VM_TIMER.timer_with_label("Loader::type_to_type_layout"); + let mut count = 0; self.type_to_type_layout_impl(ty, &mut count, 1) .map(|(l, _)| l) @@ -38,6 +41,8 @@ pub trait LayoutConverter: LayoutConverterBase { &self, ty: &Type, ) -> PartialVMResult<(MoveTypeLayout, bool)> { + let _timer = VM_TIMER.timer_with_label("Loader::type_to_type_layout"); + let mut count = 0; self.type_to_type_layout_impl(ty, &mut count, 1) } @@ -45,6 +50,8 @@ pub trait LayoutConverter: LayoutConverterBase { /// Converts a runtime type to a fully annotated type layout, containing information about /// field names. fn type_to_fully_annotated_layout(&self, ty: &Type) -> PartialVMResult { + let _timer = VM_TIMER.timer_with_label("Loader::type_to_type_layout"); + let mut count = 0; self.type_to_fully_annotated_layout_impl(ty, &mut count, 1) } @@ -54,7 +61,6 @@ pub trait LayoutConverter: LayoutConverterBase { // into this crate trait. pub(crate) trait LayoutConverterBase { fn vm_config(&self) -> &VMConfig; - fn struct_info_cache(&self) -> &StructInfoCache; fn fetch_struct_ty_by_idx(&self, idx: StructNameIndex) -> PartialVMResult>; fn struct_name_index_map(&self) -> &StructNameIndexMap; @@ -167,15 +173,6 @@ pub(crate) trait LayoutConverterBase { count: &mut u64, depth: u64, ) -> PartialVMResult<(MoveTypeLayout, bool)> { - let struct_cache = self.struct_info_cache(); - if let Some((struct_layout, node_count, has_identifier_mappings)) = - struct_cache.get_struct_layout_info(&struct_name_idx, ty_args) - { - *count += node_count; - return Ok((struct_layout, has_identifier_mappings)); - } - - let count_before = *count; let struct_type = self.fetch_struct_ty_by_idx(struct_name_idx)?; let mut has_identifier_mappings = false; @@ -249,14 +246,6 @@ pub(crate) trait LayoutConverterBase { }, }; - let field_node_count = *count - count_before; - struct_cache.store_struct_layout_info( - struct_name_idx, - ty_args.to_vec(), - layout.clone(), - field_node_count, - has_identifier_mappings, - ); Ok((layout, has_identifier_mappings)) } @@ -334,14 +323,6 @@ pub(crate) trait LayoutConverterBase { count: &mut u64, depth: u64, ) -> PartialVMResult { - let struct_info_cache = self.struct_info_cache(); - if let Some((layout, annotated_node_count)) = - struct_info_cache.get_annotated_struct_layout_info(&struct_name_idx, ty_args) - { - *count += annotated_node_count; - return Ok(layout); - } - let struct_type = self.fetch_struct_ty_by_idx(struct_name_idx)?; // TODO(#13806): have annotated layouts for variants. Currently, we just return the raw @@ -352,7 +333,6 @@ pub(crate) trait LayoutConverterBase { .map(|(l, _)| l); } - let count_before = *count; let struct_tag = self.struct_name_idx_to_struct_tag(struct_name_idx, ty_args)?; let fields = struct_type.fields(None)?; @@ -369,14 +349,7 @@ pub(crate) trait LayoutConverterBase { .collect::>>()?; let struct_layout = MoveTypeLayout::Struct(MoveStructLayout::with_types(struct_tag, field_layouts)); - let field_node_count = *count - count_before; - - struct_info_cache.store_annotated_struct_layout_info( - struct_name_idx, - ty_args.to_vec(), - struct_layout.clone(), - field_node_count, - ); + Ok(struct_layout) } } @@ -399,10 +372,6 @@ impl<'a> LayoutConverterBase for StorageLayoutConverter<'a> { self.storage.runtime_environment().vm_config() } - fn struct_info_cache(&self) -> &StructInfoCache { - self.storage.runtime_environment().ty_cache() - } - fn fetch_struct_ty_by_idx(&self, idx: StructNameIndex) -> PartialVMResult> { let struct_name = self.struct_name_index_map().idx_to_struct_name_ref(idx)?; self.storage.fetch_struct_ty( @@ -458,10 +427,6 @@ impl<'a> LayoutConverterBase for LoaderLayoutConverter<'a> { self.loader.vm_config() } - fn struct_info_cache(&self) -> &StructInfoCache { - self.loader.ty_cache(self.module_storage) - } - fn fetch_struct_ty_by_idx(&self, idx: StructNameIndex) -> PartialVMResult> { match self.loader { Loader::V1(..) => { diff --git a/third_party/move/move-vm/runtime/src/storage/ty_tag_converter.rs b/third_party/move/move-vm/runtime/src/storage/ty_tag_converter.rs index 6b045d511102db..11e0bfc42e329b 100644 --- a/third_party/move/move-vm/runtime/src/storage/ty_tag_converter.rs +++ b/third_party/move/move-vm/runtime/src/storage/ty_tag_converter.rs @@ -161,6 +161,14 @@ impl TypeTagCache { } } +impl Clone for TypeTagCache { + fn clone(&self) -> Self { + Self { + cache: RwLock::new(self.cache.read().clone()), + } + } +} + /// Responsible for building type tags, while also doing the metering in order to bound space and /// time complexity. pub(crate) struct TypeTagConverter<'a> { @@ -288,7 +296,9 @@ mod tests { use hashbrown::Equivalent; use move_binary_format::file_format::StructTypeParameter; use move_core_types::{ - ability::{Ability, AbilitySet}, account_address::AccountAddress, identifier::Identifier, + ability::{Ability, AbilitySet}, + account_address::AccountAddress, + identifier::Identifier, language_storage::ModuleId, }; use move_vm_types::loaded_data::runtime_types::{