From 4c8450eb245c93f8b2589cd0e751cd301648f42e Mon Sep 17 00:00:00 2001 From: Timothy Zakian Date: Fri, 31 Jan 2025 15:12:45 -0800 Subject: [PATCH] [9/n][vm-rewrite][Move] Update depth calculation calculation/caching to be threadsafe Switching the depth calculation of types to be lazily evaluated when we first encountered the value, coupled with shared package cache, and the types living in this shared package cache meant that the `depth` for a type needed to be made threadsafe since we may need to write to it in a parallel context. This does this by `RwLock`ing the `depth` field and then making the changes from there. --- Cargo.lock | 1 + .../src/execution/dispatch_tables.rs | 46 ++++++++----------- .../src/jit/execution/translate.rs | 5 +- 3 files changed, 22 insertions(+), 30 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d5577a28f76dd4..0dfe6a6cb74666 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8231,6 +8231,7 @@ dependencies = [ "fail", "hex", "lasso", + "move-abstract-interpreter", "move-binary-format", "move-bytecode-verifier", "move-core-types", diff --git a/external-crates/move/crates/move-vm-runtime/src/execution/dispatch_tables.rs b/external-crates/move/crates/move-vm-runtime/src/execution/dispatch_tables.rs index a2f7106d6ed549..392701b2d4640b 100644 --- a/external-crates/move/crates/move-vm-runtime/src/execution/dispatch_tables.rs +++ b/external-crates/move/crates/move-vm-runtime/src/execution/dispatch_tables.rs @@ -92,7 +92,7 @@ pub struct IntraPackageKey { pub member_name: IdentifierKey, } -#[derive(Debug, Clone, Eq, Hash, Ord, PartialEq, PartialOrd)] +#[derive(Debug)] pub struct CachedDatatype { pub abilities: AbilitySet, pub type_parameters: Vec, @@ -101,7 +101,9 @@ pub struct CachedDatatype { pub runtime_id: ModuleId, pub module_key: IdentifierKey, pub member_key: IdentifierKey, - pub depth: Option, + // Type depth is calculated lazily and cached here. Note that this is a read-write lock because + // this is calculated lazily and could be shared across multiple threads. + pub depth: RwLock>, pub datatype_info: Datatype, } @@ -234,32 +236,20 @@ impl VMDispatchTables { })? .vtable .types; - match Arc::get_mut(&mut tys.type_at(&cache_idx.inner_pkg_key)) { - Some(datatype) => { - // This can happen if we race for filling in the depth of the datatype which is - // fine as only one will win. However, if we race for filling in the depth of - // the then the two depths must be the same. - if let Some(prev_depth) = datatype.depth.as_ref() { - if prev_depth != &depth { - return Err(PartialVMError::new( - StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, - ) - .with_message(format!( - "Depth calculation mismatch for {cache_idx:?}" - ))); - } - } - datatype.depth = Some(depth); - } - None => { - // we have pending references to the `Arc` which is impossible, - // given the code that adds the `Arc` is above and no reference to - // it should exist. - // So in the spirit of not crashing we log the issue and move on leaving the - // datatypes depth as `None` -- if we try to access it later we will treat this - // as too deep. - error!("Arc cannot have any live reference while publishing"); + let datatype_depth = &tys.type_at(&cache_idx.inner_pkg_key).depth; + let mut depth_lock = datatype_depth.write(); + if let Some(prev_depth) = depth_lock.as_ref() { + // This can happen if we race for filling in the depth of the datatype which is + // fine as only one will win. However, if we race for filling in the depth of + // the then the two depths must be the same. + if prev_depth != &depth { + return Err( + PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) + .with_message(format!("Depth calculation mismatch for {cache_idx:?}")), + ); } + } else { + *depth_lock = Some(depth); } } Ok(depth_formula) @@ -272,7 +262,7 @@ impl VMDispatchTables { ) -> PartialVMResult { let datatype = self.resolve_type(&def_idx.clone())?; // If we've already computed this datatypes depth, no more work remains to be done. - if let Some(form) = &datatype.depth { + if let Some(form) = datatype.depth.read().as_ref() { return Ok(form.clone()); } if let Some(form) = depth_cache.get(def_idx) { diff --git a/external-crates/move/crates/move-vm-runtime/src/jit/execution/translate.rs b/external-crates/move/crates/move-vm-runtime/src/jit/execution/translate.rs index 3d9076fc7f48f6..97be2e5f3a5a3a 100644 --- a/external-crates/move/crates/move-vm-runtime/src/jit/execution/translate.rs +++ b/external-crates/move/crates/move-vm-runtime/src/jit/execution/translate.rs @@ -30,6 +30,7 @@ use move_binary_format::{ internals::ModuleIndex, }; use move_core_types::{identifier::Identifier, language_storage::ModuleId, vm_status::StatusCode}; +use parking_lot::RwLock; use std::collections::{BTreeMap, BTreeSet, HashMap}; struct PackageContext<'natives> { @@ -1105,7 +1106,7 @@ fn load_module_types( name: name.to_owned(), defining_id, runtime_id: module_id.clone(), - depth: None, + depth: RwLock::new(None), datatype_info: Datatype::Struct(StructType { fields, field_names, @@ -1181,7 +1182,7 @@ fn load_module_types( name: name.to_owned(), defining_id, runtime_id: module_id.clone(), - depth: None, + depth: RwLock::new(None), datatype_info: Datatype::Enum(EnumType { variants, enum_def: EnumDefinitionIndex(idx as u16),