Skip to content

Commit

Permalink
[comments] Address Rati's and Zekun's comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@georgemitenkov
georgemitenkov committed Nov 11, 2024
1 parent 20628fa commit ac6d378
Show file tree
Hide file tree
Showing 13 changed files with 231 additions and 323 deletions.
87 changes: 45 additions & 42 deletions aptos-move/aptos-vm-types/src/module_and_script_storage/state_view_adapter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::module_and_script_storage::module_storage::AptosModuleStorage;
use ambassador::Delegate;
use aptos_types::{
error::PanicError,
state_store::{state_key::StateKey, state_value::StateValueMetadata, StateView},
state_store::{state_key::StateKey, state_value::StateValueMetadata, StateView, TStateView},
vm::modules::AptosModuleExtension,
};
use bytes::Bytes;
Expand All @@ -28,7 +28,7 @@ use move_vm_types::{
code::{ModuleBytesStorage, ModuleCode},
module_storage_error,
};
use std::sync::Arc;
use std::{ops::Deref, sync::Arc};

/// Avoids orphan rule to implement [ModuleBytesStorage] for [StateView].
struct StateViewAdapter<'s, S> {
Expand All @@ -48,6 +48,14 @@ impl<'s, S: StateView> ModuleBytesStorage for StateViewAdapter<'s, S> {
}
}

impl<'s, S: StateView> Deref for StateViewAdapter<'s, S> {
type Target = S;

fn deref(&self) -> &Self::Target {
&self.state_view
}
}

/// A (not thread-safe) implementation of code storage on top of a state view. It is never built
/// directly by clients - only via [AsAptosCodeStorage] trait. Can be used to resolve both modules
/// and cached scripts.
Expand Down Expand Up @@ -84,7 +92,7 @@ impl<'s, S: StateView, E: WithRuntimeEnvironment> AptosCodeStorageAdapter<'s, S,
}

/// Drains cached verified modules from the code storage, transforming them into format used by
/// global caches. Should only be called when the code storage borrows [StateView].
/// global caches.
pub fn into_verified_module_code_iter(
self,
) -> Result<
Expand All @@ -96,47 +104,42 @@ impl<'s, S: StateView, E: WithRuntimeEnvironment> AptosCodeStorageAdapter<'s, S,
>,
PanicError,
> {
let state_view = match self.storage.module_storage().byte_storage().state_view {
BorrowedOrOwned::Borrowed(state_view) => state_view,
BorrowedOrOwned::Owned(_) => {
return Err(PanicError::CodeInvariantError(
"Verified modules should only be extracted from borrowed state".to_string(),
))
},
};

let mut modules_to_add = vec![];
for (key, verified_code) in self
let (state_view, verified_modules_iter) = self
.storage
.into_module_storage()
.into_verified_modules_iter()
{
// We have cached the module previously, so we must be able to find it in storage.
let extension = state_view
.get_state_value(&StateKey::module_id(&key))
.map_err(|err| {
let msg = format!(
"Failed to retrieve module {}::{} from storage {:?}",
key.address(),
key.name(),
err
);
PanicError::CodeInvariantError(msg)
})?
.map(AptosModuleExtension::new)
.ok_or_else(|| {
let msg = format!(
"Module {}::{} should exist, but it does not anymore",
key.address(),
key.name()
);
PanicError::CodeInvariantError(msg)
})?;

let module = ModuleCode::from_verified_ref(verified_code, Arc::new(extension));
modules_to_add.push((key, Arc::new(module)))
}
Ok(modules_to_add.into_iter())
.unpack_into_verified_modules_iter();

Ok(verified_modules_iter
.map(|(key, verified_code)| {
// We have cached the module previously, so we must be able to find it in storage.
let extension = state_view
.get_state_value(&StateKey::module_id(&key))
.map_err(|err| {
let msg = format!(
"Failed to retrieve module {}::{} from storage {:?}",
key.address(),
key.name(),
err
);
PanicError::CodeInvariantError(msg)
})?
.map_or_else(
|| {
let msg = format!(
"Module {}::{} should exist, but it does not anymore",
key.address(),
key.name()
);
Err(PanicError::CodeInvariantError(msg))
},
|state_value| Ok(AptosModuleExtension::new(state_value)),
)?;

let module = ModuleCode::from_arced_verified(verified_code, Arc::new(extension));
Ok((key, Arc::new(module)))
})
.collect::<Result<Vec<_>, PanicError>>()?
.into_iter())
}
}

Expand Down
51 changes: 24 additions & 27 deletions aptos-move/aptos-vm/src/block_executor/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,11 @@ use aptos_vm_logging::{
alert, flush_speculative_logs, init_speculative_logs, prelude::CRITICAL_ERRORS,
};
use aptos_vm_types::{
abstract_write_op::AbstractResourceWriteOp, module_and_script_storage::AsAptosCodeStorage,
module_write_set::ModuleWrite, output::VMOutput, resolver::ResourceGroupSize,
abstract_write_op::AbstractResourceWriteOp,
module_and_script_storage::{AptosCodeStorageAdapter, AsAptosCodeStorage},
module_write_set::ModuleWrite,
output::VMOutput,
resolver::ResourceGroupSize,
};
use move_binary_format::{
errors::{Location, VMError},
Expand Down Expand Up @@ -454,26 +457,23 @@ impl BlockAptosVM {
let struct_name_index_map_size = runtime_environment
.struct_name_index_map_size()
.map_err(|err| err.finish(Location::Undefined).into_vm_status())?;
if struct_name_index_map_size > module_cache_config.max_struct_name_index_map_size {
module_cache.flush_unchecked();
if struct_name_index_map_size > module_cache_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_is_greater_than(module_cache_config.max_module_cache_size_in_bytes)
{
module_cache.flush_unchecked();
if module_cache.size_in_bytes() > module_cache_config.max_module_cache_size_in_bytes {
module_cache.flush_unsync();
}

// Finally, to avoid cold starts, fetch the framework code prior to block execution.
if module_cache.num_modules() == 0 && module_cache_config.prefetch_framework_code {
prefetch_aptos_framework(environment.clone(), state_view, &module_cache).map_err(
|err| {
alert!("Failed to load Aptos framework to module cache: {:?}", err);
VMError::from(err).into_vm_status()
},
)?;
let code_storage = state_view.as_aptos_code_storage(environment.clone());
prefetch_aptos_framework(code_storage, &module_cache).map_err(|err| {
alert!("Failed to load Aptos framework to module cache: {:?}", err);
VMError::from(err).into_vm_status()
})?;
}

let executor = BlockExecutor::<
Expand All @@ -491,8 +491,6 @@ impl BlockAptosVM {

let ret = executor.execute_block(environment, signature_verified_block, state_view);
if !module_cache_manager.mark_done() {
// Something is wrong as we were not able to mark execution as done. Return an
// error.
return Err(VMStatus::error(
StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR,
Some("Unable to mark block execution as done".to_string()),
Expand Down Expand Up @@ -560,13 +558,10 @@ impl BlockAptosVM {
/// If Aptos framework exists, loads "transaction_validation.move" and all its transitive
/// dependencies from storage into provided module cache. If loading fails for any reason, a panic
/// error is returned.
fn prefetch_aptos_framework(
environment: AptosEnvironment,
state_view: &impl StateView,
fn prefetch_aptos_framework<S: StateView>(
code_storage: AptosCodeStorageAdapter<S, AptosEnvironment>,
module_cache: &GlobalModuleCache<ModuleId, CompiledModule, Module, AptosModuleExtension>,
) -> Result<(), PanicError> {
let code_storage = state_view.as_aptos_code_storage(environment);

// If framework code exists in storage, the transitive closure will be verified and cached.
let maybe_loaded = code_storage
.fetch_verified_module(&AccountAddress::ONE, ident_str!("transaction_validation"))
Expand All @@ -576,13 +571,11 @@ fn prefetch_aptos_framework(
PanicError::CodeInvariantError(format!("Unable to fetch Aptos framework: {:?}", err))
})?;

if let Some(module) = maybe_loaded {
drop(module);

if maybe_loaded.is_some() {
// Framework must have been loaded. Drain verified modules from local cache into
// global cache.
let verified_module_code_iter = code_storage.into_verified_module_code_iter()?;
module_cache.insert_verified_unchecked(verified_module_code_iter)?;
module_cache.insert_verified_unsync(verified_module_code_iter)?;
}
Ok(())
}
Expand All @@ -598,10 +591,12 @@ mod test {
let state_view = executor.get_state_view();

let environment = AptosEnvironment::new_with_delayed_field_optimization_enabled(state_view);
let code_storage = state_view.as_aptos_code_storage(environment);

let module_cache = GlobalModuleCache::empty();
assert_eq!(module_cache.num_modules(), 0);

let result = prefetch_aptos_framework(environment, state_view, &module_cache);
let result = prefetch_aptos_framework(code_storage, &module_cache);
assert!(result.is_ok());
assert!(module_cache.num_modules() > 0);
}
Expand All @@ -612,10 +607,12 @@ mod test {

let environment =
AptosEnvironment::new_with_delayed_field_optimization_enabled(&state_view);
let code_storage = state_view.as_aptos_code_storage(environment);

let module_cache = GlobalModuleCache::empty();
assert_eq!(module_cache.num_modules(), 0);

let result = prefetch_aptos_framework(environment, &state_view, &module_cache);
let result = prefetch_aptos_framework(code_storage, &module_cache);
assert!(result.is_ok());
assert_eq!(module_cache.num_modules(), 0);
}
Expand Down
18 changes: 9 additions & 9 deletions aptos-move/aptos-vm/tests/sharded_block_executor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,11 +308,11 @@ mod test_utils {
.into_iter()
.map(|t| t.into_txn())
.collect();
let executor = AptosVMBlockExecutor::new();
if let Some(module_cache_manager) = executor.module_cache_manager() {
let block_executor = AptosVMBlockExecutor::new();
if let Some(module_cache_manager) = block_executor.module_cache_manager() {
module_cache_manager.mark_ready(None, None);
}
let unsharded_txn_output = executor
let unsharded_txn_output = block_executor
.execute_block_no_limit(&ordered_txns, executor.data_store())
.unwrap();
compare_txn_outputs(unsharded_txn_output, sharded_txn_output);
Expand Down Expand Up @@ -362,11 +362,11 @@ mod test_utils {
)
.unwrap();

let executor = AptosVMBlockExecutor::new();
if let Some(module_cache_manager) = executor.module_cache_manager() {
let block_executor = AptosVMBlockExecutor::new();
if let Some(module_cache_manager) = block_executor.module_cache_manager() {
module_cache_manager.mark_ready(None, None);
}
let unsharded_txn_output = executor
let unsharded_txn_output = block_executor
.execute_block_no_limit(&execution_ordered_txns, executor.data_store())
.unwrap();
compare_txn_outputs(unsharded_txn_output, sharded_txn_output);
Expand Down Expand Up @@ -420,11 +420,11 @@ mod test_utils {
)
.unwrap();

let executor = AptosVMBlockExecutor::new();
if let Some(module_cache_manager) = executor.module_cache_manager() {
let block_executor = AptosVMBlockExecutor::new();
if let Some(module_cache_manager) = block_executor.module_cache_manager() {
module_cache_manager.mark_ready(None, None);
}
let unsharded_txn_output = executor
let unsharded_txn_output = block_executor
.execute_block_no_limit(&execution_ordered_txns, executor.data_store())
.unwrap();
compare_txn_outputs(unsharded_txn_output, sharded_txn_output);
Expand Down
10 changes: 5 additions & 5 deletions aptos-move/block-executor/src/captured_reads.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,11 +295,11 @@ impl DelayedFieldRead {
/// Represents a module read, either from immutable cross-block cache, or from code [SyncCodeCache]
/// used by block executor (per-block cache). This way, when transaction needs to read a module
/// from [SyncCodeCache] it can first check the read-set here.
enum ModuleRead<DC, VC, S, V> {
enum ModuleRead<DC, VC, S> {
/// Read from the cross-block module cache.
GlobalCache,
/// Read from per-block cache ([SyncCodeCache]) used by parallel execution.
PerBlockCache(Option<(Arc<ModuleCode<DC, VC, S>>, V)>),
PerBlockCache(Option<(Arc<ModuleCode<DC, VC, S>>, Option<TxnIndex>)>),
}

/// Represents a result of a read from [CapturedReads] when they are used as the transaction-level
Expand All @@ -326,7 +326,7 @@ pub(crate) struct CapturedReads<T: Transaction, K, DC, VC, S> {

#[deprecated]
pub(crate) deprecated_module_reads: Vec<T::Key>,
module_reads: hashbrown::HashMap<K, ModuleRead<DC, VC, S, Option<TxnIndex>>>,
module_reads: hashbrown::HashMap<K, ModuleRead<DC, VC, S>>,

/// If there is a speculative failure (e.g. delta application failure, or an observed
/// inconsistency), the transaction output is irrelevant (must be discarded and transaction
Expand Down Expand Up @@ -1578,12 +1578,12 @@ mod test {
assert!(!valid);

// Without invalid module (and if it is not captured), validation should pass.
global_module_cache.remove(&1);
assert!(global_module_cache.remove(&1));
captured_reads.module_reads.remove(&1);
assert!(captured_reads.validate_module_reads(&global_module_cache, &per_block_module_cache));

// Validation fails if we captured a cross-block module which does not exist anymore.
global_module_cache.remove(&0);
assert!(global_module_cache.remove(&0));
let valid =
captured_reads.validate_module_reads(&global_module_cache, &per_block_module_cache);
assert!(!valid);
Expand Down
Loading

0 comments on commit ac6d378

Please sign in to comment.