Skip to content

Commit

Permalink
[loader-v2] Small cleanups & tests (#15279)
Browse files Browse the repository at this point in the history
- Fixed naming for global module cache.
- Added more counters, moved some old ones.
- Added unit tests for TransactionSliceMetadata + renaming.
  • Loading branch information
georgemitenkov authored Nov 20, 2024
1 parent 40a7425 commit 47f0bf3
Show file tree
Hide file tree
Showing 34 changed files with 298 additions and 211 deletions.
1 change: 0 additions & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion aptos-move/aptos-debugger/src/aptos_debugger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use aptos_types::{
account_address::AccountAddress,
block_executor::{
config::{BlockExecutorConfig, BlockExecutorConfigFromOnchain, BlockExecutorLocalConfig},
execution_state::TransactionSliceMetadata,
transaction_slice_metadata::TransactionSliceMetadata,
},
contract_event::ContractEvent,
state_store::TStateView,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ use aptos_language_e2e_tests::{
use aptos_types::{
block_executor::{
config::{BlockExecutorConfig, BlockExecutorConfigFromOnchain},
execution_state::TransactionSliceMetadata,
partitioner::PartitionedTransactions,
transaction_slice_metadata::TransactionSliceMetadata,
},
block_metadata::BlockMetadata,
on_chain_config::{OnChainConfig, ValidatorSet},
Expand Down
2 changes: 1 addition & 1 deletion aptos-move/aptos-vm-environment/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,4 @@ once_cell = { workspace = true }
sha3 = { workspace = true }

[dev-dependencies]
aptos-language-e2e-tests = { workspace = true }
aptos-types = { workspace = true, features = ["testing"] }
4 changes: 2 additions & 2 deletions aptos-move/aptos-vm-environment/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,12 +272,12 @@ fn fetch_config_and_update_hash<T: OnChainConfig>(
#[cfg(test)]
pub mod test {
use super::*;
use aptos_language_e2e_tests::data_store::FakeDataStore;
use aptos_types::state_store::MockStateView;

#[test]
fn test_new_environment() {
// This creates an empty state.
let state_view = FakeDataStore::default();
let state_view = MockStateView::empty();
let env = Environment::new(&state_view, false, None);

// Check default values.
Expand Down
2 changes: 1 addition & 1 deletion aptos-move/aptos-vm/src/aptos_vm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ use aptos_types::{
BlockExecutorConfig, BlockExecutorConfigFromOnchain, BlockExecutorLocalConfig,
BlockExecutorModuleCacheLocalConfig,
},
execution_state::TransactionSliceMetadata,
partitioner::PartitionedTransactions,
transaction_slice_metadata::TransactionSliceMetadata,
},
block_metadata::BlockMetadata,
block_metadata_ext::{BlockMetadataExt, BlockMetadataWithRandomness},
Expand Down
4 changes: 3 additions & 1 deletion aptos-move/aptos-vm/src/block_executor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ use aptos_block_executor::{
};
use aptos_infallible::Mutex;
use aptos_types::{
block_executor::{config::BlockExecutorConfig, execution_state::TransactionSliceMetadata},
block_executor::{
config::BlockExecutorConfig, transaction_slice_metadata::TransactionSliceMetadata,
},
contract_event::ContractEvent,
error::PanicError,
executable::ExecutableTestType,
Expand Down
4 changes: 2 additions & 2 deletions aptos-move/aptos-vm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ use crate::sharded_block_executor::{executor_client::ExecutorClient, ShardedBloc
use aptos_block_executor::txn_provider::default::DefaultTxnProvider;
use aptos_types::{
block_executor::{
config::BlockExecutorConfigFromOnchain, execution_state::TransactionSliceMetadata,
partitioner::PartitionedTransactions,
config::BlockExecutorConfigFromOnchain, partitioner::PartitionedTransactions,
transaction_slice_metadata::TransactionSliceMetadata,
},
state_store::StateView,
transaction::{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ use aptos_logger::{info, trace};
use aptos_types::{
block_executor::{
config::{BlockExecutorConfig, BlockExecutorLocalConfig},
execution_state::TransactionSliceMetadata,
partitioner::{ShardId, SubBlock, SubBlocksForShard, TransactionWithDependencies},
transaction_slice_metadata::TransactionSliceMetadata,
},
state_store::StateView,
transaction::{
Expand Down
10 changes: 5 additions & 5 deletions aptos-move/block-executor/src/captured_reads.rs
Original file line number Diff line number Diff line change
Expand Up @@ -648,7 +648,7 @@ where
}

/// For every module read that was captured, checks if the reads are still the same:
/// 1. Entries read from the global cross-block module cache are still valid.
/// 1. Entries read from the global module cache are not overridden.
/// 2. Entries that were not in per-block cache before are still not there.
/// 3. Entries that were in per-block cache have the same commit index.
pub(crate) fn validate_module_reads(
Expand All @@ -661,7 +661,7 @@ where
}

self.module_reads.iter().all(|(key, read)| match read {
ModuleRead::GlobalCache(_) => global_module_cache.contains_valid(key),
ModuleRead::GlobalCache(_) => global_module_cache.contains_not_overridden(key),
ModuleRead::PerBlockCache(previous) => {
let current_version = per_block_module_cache.get_module_version(key);
let previous_version = previous.as_ref().map(|(_, version)| *version);
Expand Down Expand Up @@ -1560,7 +1560,7 @@ mod test {
assert!(captured_reads.validate_module_reads(&global_module_cache, &per_block_module_cache));

// Now, mark one of the entries in invalid. Validations should fail!
global_module_cache.mark_invalid_if_contains(&1);
global_module_cache.mark_overridden(&1);
let valid =
captured_reads.validate_module_reads(&global_module_cache, &per_block_module_cache);
assert!(!valid);
Expand Down Expand Up @@ -1696,7 +1696,7 @@ mod test {

// Assume we republish this module: validation must fail.
let a = mock_deserialized_code(100, MockExtension::new(8));
global_module_cache.mark_invalid_if_contains(&0);
global_module_cache.mark_overridden(&0);
per_block_module_cache
.insert_deserialized_module(
0,
Expand All @@ -1713,6 +1713,6 @@ mod test {
// Assume we re-read the new correct version. Then validation should pass again.
captured_reads.capture_per_block_cache_read(0, Some((a, Some(10))));
assert!(captured_reads.validate_module_reads(&global_module_cache, &per_block_module_cache));
assert!(!global_module_cache.contains_valid(&0));
assert!(!global_module_cache.contains_not_overridden(&0));
}
}
7 changes: 5 additions & 2 deletions aptos-move/block-executor/src/code_cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

use crate::{
captured_reads::CacheRead,
counters::GLOBAL_MODULE_CACHE_MISS_SECONDS,
view::{LatestView, ViewState},
};
use ambassador::delegate_to_methods;
Expand Down Expand Up @@ -144,7 +145,7 @@ impl<'a, T: Transaction, S: TStateView<Key = T::Key>, X: Executable> ModuleCache
}

// Otherwise, it is a miss. Check global cache.
if let Some(module) = self.global_module_cache.get_valid(key) {
if let Some(module) = self.global_module_cache.get(key) {
state
.captured_reads
.borrow_mut()
Expand All @@ -153,6 +154,7 @@ impl<'a, T: Transaction, S: TStateView<Key = T::Key>, X: Executable> ModuleCache
}

// If not global cache, check per-block cache.
let _timer = GLOBAL_MODULE_CACHE_MISS_SECONDS.start_timer();
let read = state
.versioned_map
.module_cache()
Expand All @@ -164,11 +166,12 @@ impl<'a, T: Transaction, S: TStateView<Key = T::Key>, X: Executable> ModuleCache
Ok(read)
},
ViewState::Unsync(state) => {
if let Some(module) = self.global_module_cache.get_valid(key) {
if let Some(module) = self.global_module_cache.get(key) {
state.read_set.borrow_mut().capture_module_read(key.clone());
return Ok(Some((module, Self::Version::default())));
}

let _timer = GLOBAL_MODULE_CACHE_MISS_SECONDS.start_timer();
let read = state
.unsync_map
.module_cache()
Expand Down
Loading

0 comments on commit 47f0bf3

Please sign in to comment.