From 5240633681ce0946025c2b29659fad25647c5869 Mon Sep 17 00:00:00 2001 From: George Mitenkov Date: Wed, 15 Jan 2025 14:39:19 +0000 Subject: [PATCH] [move] Avoid double hashing in favour of equivalent keys for type tag cache --- Cargo.lock | 1 + third_party/move/move-vm/runtime/Cargo.toml | 2 + .../runtime/src/storage/ty_tag_cache.rs | 136 ++++++++++++++++-- 3 files changed, 128 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 616fbcb7d3300..80265096a6f3c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11919,6 +11919,7 @@ dependencies = [ "proptest", "serde", "sha3 0.9.1", + "smallbitvec", "triomphe", "typed-arena", ] diff --git a/third_party/move/move-vm/runtime/Cargo.toml b/third_party/move/move-vm/runtime/Cargo.toml index 4b20b934b638c..49bcf05143f36 100644 --- a/third_party/move/move-vm/runtime/Cargo.toml +++ b/third_party/move/move-vm/runtime/Cargo.toml @@ -38,7 +38,9 @@ move-binary-format = { workspace = true, features = ["fuzzing"] } move-compiler = { workspace = true } move-ir-compiler = { workspace = true } move-vm-test-utils ={ workspace = true } +move-vm-types ={ workspace = true, features = ["testing"] } proptest = { workspace = true } +smallbitvec = { workspace = true } [features] default = [] diff --git a/third_party/move/move-vm/runtime/src/storage/ty_tag_cache.rs b/third_party/move/move-vm/runtime/src/storage/ty_tag_cache.rs index b9c46defd2774..5cad728e0701e 100644 --- a/third_party/move/move-vm/runtime/src/storage/ty_tag_cache.rs +++ b/third_party/move/move-vm/runtime/src/storage/ty_tag_cache.rs @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 use crate::{loader::PseudoGasContext, RuntimeEnvironment}; +use hashbrown::{hash_map::Entry, HashMap}; use move_binary_format::errors::{PartialVMError, PartialVMResult}; use move_core_types::{ language_storage::{StructTag, TypeTag}, @@ -9,7 +10,58 @@ use move_core_types::{ }; use move_vm_types::loaded_data::{runtime_types::Type, struct_name_indexing::StructNameIndex}; use parking_lot::RwLock; -use std::collections::{hash_map::Entry, HashMap}; +use std::hash::{Hash, Hasher}; + +/// Key type for [TypeTagCache] that corresponds to a fully-instantiated struct. +#[derive(Clone, Eq, PartialEq)] +struct StructKey { + idx: StructNameIndex, + ty_args: Vec, +} + +#[derive(Eq, PartialEq)] +struct StructKeyRef<'a> { + idx: &'a StructNameIndex, + ty_args: &'a [Type], +} + +impl StructKey { + #[cfg(test)] + fn as_ref(&self) -> StructKeyRef { + StructKeyRef { + idx: &self.idx, + ty_args: self.ty_args.as_slice(), + } + } +} + +impl<'a> hashbrown::Equivalent> for StructKey { + fn equivalent(&self, other: &StructKeyRef<'a>) -> bool { + &self.idx == other.idx && self.ty_args.as_slice() == other.ty_args + } +} + +impl<'a> hashbrown::Equivalent for StructKeyRef<'a> { + fn equivalent(&self, other: &StructKey) -> bool { + self.idx == &other.idx && self.ty_args == other.ty_args.as_slice() + } +} + +// Ensure hash is the same as for StructKeyRef. +impl Hash for StructKey { + fn hash(&self, state: &mut H) { + self.idx.hash(state); + self.ty_args.hash(state); + } +} + +// Ensure hash is the same as for StructKey. +impl<'a> Hash for StructKeyRef<'a> { + fn hash(&self, state: &mut H) { + self.idx.hash(state); + self.ty_args.hash(state); + } +} /// An entry in [TypeTagCache] that also stores a "cost" of the tag. The cost is proportional to /// the size of the tag, which includes the number of inner nodes and the sum of the sizes in bytes @@ -46,7 +98,7 @@ pub(crate) struct PricedStructTag { /// If thread 3 reads the tag of this enum, the read result is always **deterministic** for the /// fixed type parameters used by thread 3. pub(crate) struct TypeTagCache { - cache: RwLock, PricedStructTag>>>, + cache: RwLock>, } impl TypeTagCache { @@ -68,7 +120,10 @@ impl TypeTagCache { idx: &StructNameIndex, ty_args: &[Type], ) -> Option { - self.cache.read().get(idx)?.get(ty_args).cloned() + self.cache + .read() + .get(&StructKeyRef { idx, ty_args }) + .cloned() } /// Inserts the struct tag and its pseudo-gas cost ([PricedStructTag]) into the cache. Returns @@ -80,19 +135,24 @@ impl TypeTagCache { priced_struct_tag: &PricedStructTag, ) -> bool { // Check if already cached. - if let Some(inner_cache) = self.cache.read().get(idx) { - if inner_cache.contains_key(ty_args) { - return false; - } + if self + .cache + .read() + .contains_key(&StructKeyRef { idx, ty_args }) + { + return false; } + let key = StructKey { + idx: *idx, + ty_args: ty_args.to_vec(), + }; let priced_struct_tag = priced_struct_tag.clone(); - let ty_args = ty_args.to_vec(); // Otherwise, we need to insert. We did the clones outside the lock, and also avoid the // double insertion. let mut cache = self.cache.write(); - if let Entry::Vacant(entry) = cache.entry(*idx).or_default().entry(ty_args) { + if let Entry::Vacant(entry) = cache.entry(key) { entry.insert(priced_struct_tag); true } else { @@ -225,14 +285,68 @@ mod tests { use super::*; use crate::config::VMConfig; use claims::{assert_err, assert_none, assert_ok, assert_ok_eq, assert_some}; - use move_binary_format::file_format::{AbilitySet, StructTypeParameter}; + use hashbrown::Equivalent; + use move_binary_format::file_format::{Ability, AbilitySet, StructTypeParameter}; use move_core_types::{ account_address::AccountAddress, identifier::Identifier, language_storage::ModuleId, }; use move_vm_types::loaded_data::runtime_types::{ AbilityInfo, StructIdentifier, StructLayout, StructType, TypeBuilder, }; - use std::str::FromStr; + use smallbitvec::SmallBitVec; + use std::{collections::hash_map::DefaultHasher, str::FromStr}; + use triomphe::Arc; + + fn calculate_hash(t: &T) -> u64 { + let mut s = DefaultHasher::new(); + t.hash(&mut s); + s.finish() + } + + #[test] + fn test_struct_key_equivalence_and_hash() { + let struct_keys = [ + StructKey { + idx: StructNameIndex::new(0), + ty_args: vec![], + }, + StructKey { + idx: StructNameIndex::new(1), + ty_args: vec![Type::U8], + }, + StructKey { + idx: StructNameIndex::new(2), + ty_args: vec![Type::Bool, Type::Vector(Arc::new(Type::Bool))], + }, + StructKey { + idx: StructNameIndex::new(3), + ty_args: vec![ + Type::Struct { + idx: StructNameIndex::new(0), + ability: AbilityInfo::struct_(AbilitySet::singleton(Ability::Key)), + }, + Type::StructInstantiation { + idx: StructNameIndex::new(1), + ty_args: Arc::new(vec![Type::Address, Type::Struct { + idx: StructNameIndex::new(2), + ability: AbilityInfo::struct_(AbilitySet::singleton(Ability::Copy)), + }]), + ability: AbilityInfo::generic_struct( + AbilitySet::singleton(Ability::Drop), + SmallBitVec::new(), + ), + }, + ], + }, + ]; + + for struct_key in struct_keys { + let struct_key_ref = struct_key.as_ref(); + assert!(struct_key.equivalent(&struct_key_ref)); + assert!(struct_key_ref.equivalent(&struct_key)); + assert_eq!(calculate_hash(&struct_key), calculate_hash(&struct_key_ref)); + } + } #[test] fn test_type_tag_cache() {