[move][move-vm][rewrite][cleanup 3/n] Add constants to arena, use pointers for them

external-crates/move/crates/move-cli/tests/sandbox_tests/print_stack_trace/args.exp

@@ -21,7 +21,7 @@ Call Stack:
 
         Code:
             [10] LdU64(1)
-            [11] VecImmBorrow(2)
+            [11] VecImmBorrow(Bool)
             [12] StLoc(0)
           > [13] CallGeneric(0)
             [14] StLoc(2)

external-crates/move/crates/move-vm-runtime/src/cache/arena.rs

@@ -3,14 +3,21 @@
 
 #![allow(unsafe_code)]
 
+use std::alloc::Layout;
+
 use bumpalo::Bump;
+use move_binary_format::errors::{PartialVMError, PartialVMResult};
+use move_core_types::vm_status::StatusCode;
 
 // -------------------------------------------------------------------------------------------------
 // Types - Arenas for Cache Allocations
 // -------------------------------------------------------------------------------------------------
 
 pub struct Arena(Bump);
 
+/// Size of a given arena -- a loaded package must fit in this.
+const ARENA_SIZE: usize = 10_000_000;
+
 // -------------------------------------------------------------------------------------------------
 // Impls
 // -------------------------------------------------------------------------------------------------
@@ -23,16 +30,57 @@ impl Default for Arena {
 
 impl Arena {
     pub fn new() -> Self {
-        Arena(Bump::new())
+        let bump = Bump::new();
+        bump.set_allocation_limit(Some(ARENA_SIZE));
+        Arena(bump)
+    }
+
+    /// SAFETY: it is the caller's responsibility to ensure that `self` is not shared across
+    /// threads during this call. This should be fine as the translation step that uses an arena
+    /// should happen in a thread that holds that arena, with no other contention for allocation
+    /// into it, and nothing should allocate into a LoadedModule after it is loaded.
+    pub fn alloc_slice<T>(&self, items: Vec<T>) -> PartialVMResult<*mut [T]> {
+        let len = items.len();
+        let layout = Layout::array::<T>(len).map_err(|_| {
+            PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
+                .with_message("Could not compute type layout".to_string())
+        })?;
+
+        // Allocate memory for the slice
+        if let Ok(ptr) = self.0.try_alloc_layout(layout) {
+            let mut_ptr = ptr.as_ptr() as *mut T;
+
+            // Move the items into the allocated memory -- essentially `memcpy`
+            unsafe {
+                std::ptr::copy_nonoverlapping(items.as_ptr(), mut_ptr, len);
+            }
+
+            // Prevent the original vector from dropping its data
+            std::mem::forget(items);
+
+            // Return the raw pointer to the allocated slice
+            unsafe { Ok(std::slice::from_raw_parts_mut(mut_ptr, len)) }
+        } else {
+            Err(
+                PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
+                    .with_message("Could not allocate memory for slice".to_string()),
+            )
+        }
     }
 
     /// SAFETY: it is the caller's responsibility to ensure that `self` is not shared across
     /// threads during this call. This should be fine as the translation step that uses an arena
     /// should happen in a thread that holds that arena, with no other contention for allocation
     /// into it, and nothing should allocate into a LoadedModule after it is loaded.
-    pub fn alloc_slice<T>(&self, items: impl ExactSizeIterator<Item = T>) -> *mut [T] {
-        let slice = self.0.alloc_slice_fill_iter(items);
-        slice as *mut [T]
+    pub fn alloc_item<T>(&self, item: T) -> PartialVMResult<*mut T> {
+        if let Ok(ptr) = self.0.try_alloc(item) {
+            Ok(ptr as *mut T)
+        } else {
+            Err(
+                PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)
+                    .with_message("Could not allocate memory for slice".to_string()),
+            )
+        }
     }
 }
 

external-crates/move/crates/move-vm-runtime/src/cache/identifier_interner.rs

@@ -10,12 +10,19 @@ use move_core_types::{
 use lasso::{Spur, ThreadedRodeo};
 
 /// A wrapper around a lasso ThreadedRoade with some niceties to make it easier to use in the VM.
-#[derive(Debug, Default)]
+#[derive(Debug)]
 pub struct IdentifierInterner(ThreadedRodeo);
 
 pub type IdentifierKey = Spur;
 
+const STRING_SLOTS: usize = 1_000_000_000;
+
 impl IdentifierInterner {
+    pub fn new() -> Self {
+        let rodeo = ThreadedRodeo::with_capacity(lasso::Capacity::for_strings(STRING_SLOTS));
+        Self(rodeo)
+    }
+
     /// Resolve a string in the interner or produce an invariant violation (as they should always be
     /// there). The `key_type` is used to make a more-informative error message.
     pub fn resolve_string(&self, key: &IdentifierKey, key_type: &str) -> PartialVMResult<String> {

external-crates/move/crates/move-vm-runtime/src/cache/move_cache.rs

@@ -6,8 +6,8 @@
 // and publishing packages to the VM.
 
 use crate::{
-    cache::identifier_interner::IdentifierInterner, jit, natives::functions::NativeFunctions,
-    shared::types::PackageStorageId, validation::verification,
+    jit, natives::functions::NativeFunctions, shared::types::PackageStorageId,
+    validation::verification,
 };
 use move_vm_config::runtime::VMConfig;
 use parking_lot::RwLock;
@@ -44,7 +44,6 @@ pub struct MoveCache {
     pub(crate) natives: Arc<NativeFunctions>,
     pub(crate) vm_config: Arc<VMConfig>,
     pub(crate) package_cache: Arc<RwLock<PackageCache>>,
-    pub(crate) string_cache: Arc<IdentifierInterner>,
 }
 
 // -------------------------------------------------------------------------------------------------
@@ -57,7 +56,6 @@ impl MoveCache {
             natives,
             vm_config,
             package_cache: Arc::new(RwLock::new(HashMap::new())),
-            string_cache: Arc::new(IdentifierInterner::default()),
         }
     }
 
@@ -92,10 +90,6 @@ impl MoveCache {
     pub fn package_cache(&self) -> &RwLock<PackageCache> {
         &self.package_cache
     }
-
-    pub fn string_interner(&self) -> &IdentifierInterner {
-        &self.string_cache
-    }
 }
 
 impl Package {
@@ -124,13 +118,11 @@ impl Clone for MoveCache {
             natives,
             vm_config,
             package_cache,
-            string_cache,
         } = self;
         Self {
             natives: natives.clone(),
             vm_config: vm_config.clone(),
             package_cache: package_cache.clone(),
-            string_cache: string_cache.clone(),
         }
     }
 }

external-crates/move/crates/move-vm-runtime/src/dev_utils/in_memory_test_adapter.rs

@@ -120,7 +120,8 @@ impl VMTestAdapter<InMemoryStorage> for InMemoryTestAdapter {
         package: SerializedPackage,
     ) -> VMResult<(verif_ast::Package, MoveVM<'extensions>)> {
         let Some(storage_id) = package.linkage_table.get(&runtime_id).cloned() else {
-            // TODO: VM error instead?
+            // NB: This is a panic because it means something went really wrong -- we should always
+            // panic if we expect a package to exist and it is not there.
             panic!("Did not find runtime ID {runtime_id} in linkage context.");
         };
         assert_eq!(storage_id, package.storage_id);
@@ -140,7 +141,8 @@ impl VMTestAdapter<InMemoryStorage> for InMemoryTestAdapter {
         package: verif_ast::Package,
     ) -> VMResult<()> {
         let Some(storage_id) = package.linkage_table.get(&runtime_id).cloned() else {
-            // TODO: VM error instead?
+            // NB: This is a panic because it means something went really wrong -- we should always
+            // panic if we expect a package to exist and it is not there.
             panic!("Did not find runtime ID {runtime_id} in linkage context.");
         };
         assert!(storage_id == package.storage_id);
@@ -171,7 +173,6 @@ impl VMTestAdapter<InMemoryStorage> for InMemoryTestAdapter {
     // This will generate the linkage context based on the transitive dependencies of the
     // provided package modules if the package's dependencies are in the data cache, or error
     // otherwise.
-    // TODO: It would be great, longer term, to move this to the trait and reuse it.
     fn generate_linkage_context(
         &self,
         runtime_package_id: RuntimePackageId,

external-crates/move/crates/move-vm-runtime/src/dev_utils/storage.rs

@@ -316,26 +316,57 @@ impl<'a, 'b, S: ModuleResolver> ModuleResolver for DeltaStorage<'a, 'b, S> {
         &self,
         ids: &[AccountAddress],
     ) -> Result<Vec<Option<SerializedPackage>>, Self::Error> {
-        ids.iter()
+        // First pass: Split IDs into those found in delta.accounts() and those not found
+        let (found_in_delta, not_found_in_delta): (Vec<_>, Vec<_>) = ids
+            .iter()
+            .partition(|storage_id| self.delta.accounts().contains_key(storage_id));
+
+        // Prepare results for those found in delta
+        let mut results: Vec<Option<SerializedPackage>> = found_in_delta
+            .iter()
             .map(|storage_id| {
-                if let Some(account_storage) = self.delta.accounts().get(storage_id) {
-                    let module_bytes: Vec<_> = account_storage
-                        .modules()
-                        .values()
-                        .map(|op| op.clone().ok())
-                        .collect::<Option<_>>()
-                        .unwrap_or_default();
-
-                    Ok(Some(SerializedPackage::raw_package(
-                        module_bytes,
-                        *storage_id,
-                    )))
-                } else {
-                    // TODO: Can optimize this to do a two-pass bulk lookup if we want
-                    Ok(self.base.get_packages(&[*storage_id])?[0].clone())
-                }
+                let account_storage = self
+                    .delta
+                    .accounts()
+                    .get(storage_id)
+                    .expect("Key must exist as it was filtered earlier");
+                let module_bytes: Vec<_> = account_storage
+                    .modules()
+                    .values()
+                    .map(|op| op.clone().ok())
+                    .collect::<Option<_>>()
+                    .unwrap_or_default();
+
+                Ok(Some(SerializedPackage::raw_package(
+                    module_bytes,
+                    *storage_id,
+                )))
             })
-            .collect::<Result<Vec<_>, Self::Error>>()
+            .collect::<Result<Vec<_>, Self::Error>>()?;
+
+        // Perform a bulk lookup for IDs not found in delta
+        if !not_found_in_delta.is_empty() {
+            let base_results = self.base.get_packages(&not_found_in_delta)?;
+
+            // Extend results with those from the base lookup
+            results.extend(base_results);
+        }
+
+        // Match the output order of the input `ids`
+        let mut final_results = Vec::with_capacity(ids.len());
+        let mut found_in_delta_iter = found_in_delta.into_iter();
+        let mut not_found_in_delta_iter = not_found_in_delta.into_iter();
+        for id in ids {
+            if found_in_delta_iter.as_slice().first() == Some(id) {
+                found_in_delta_iter.next();
+                final_results.push(results.remove(0)); // Add result from `results`
+            } else if not_found_in_delta_iter.as_slice().first() == Some(id) {
+                not_found_in_delta_iter.next();
+                final_results.push(results.remove(0)); // Add result from `base.get_packages`
+            }
+        }
+
+        Ok(final_results)
     }
 }
 

external-crates/move/crates/move-vm-runtime/src/execution/dispatch_tables.rs

@@ -52,7 +52,7 @@ use tracing::error;
 /// the transaction.
 ///
 /// TODO(tzakian): The representation can be optimized to use a more efficient data structure for
-/// vtable/cross-package function resolution but we will keep it simple for now.
+/// vtable/cross-package function resolution (e.g., caching) but we keep it simple for now.
 #[derive(Debug, Clone)]
 pub struct VMDispatchTables {
     pub(crate) vm_config: Arc<VMConfig>,