From 14758a1e2c846ae37126fc5de7fc30d59408c40d Mon Sep 17 00:00:00 2001 From: Balaji Arun Date: Wed, 18 Oct 2023 20:44:16 -0700 Subject: [PATCH] [execution] change HashMap to BTreeMap in VMChangeSet (#10582) * fix * lints + tests --- aptos-move/aptos-vm-types/src/change_set.rs | 60 +++++++++---------- .../src/tests/test_change_set.rs | 8 +-- .../aptos-vm-types/src/tests/test_output.rs | 5 +- aptos-move/aptos-vm-types/src/tests/utils.rs | 10 ++-- aptos-move/aptos-vm/src/block_executor/mod.rs | 10 ++-- .../src/move_vm_ext/respawned_session.rs | 10 ++-- .../aptos-vm/src/move_vm_ext/session.rs | 10 ++-- .../src/proptest_types/types.rs | 12 ++-- aptos-move/block-executor/src/task.rs | 10 ++-- 9 files changed, 68 insertions(+), 67 deletions(-) diff --git a/aptos-move/aptos-vm-types/src/change_set.rs b/aptos-move/aptos-vm-types/src/change_set.rs index 0e07a8363e64f..d060215c76148 100644 --- a/aptos-move/aptos-vm-types/src/change_set.rs +++ b/aptos-move/aptos-vm-types/src/change_set.rs @@ -13,8 +13,8 @@ use aptos_types::{ use move_binary_format::errors::Location; use move_core_types::vm_status::{err_msg, StatusCode, VMStatus}; use std::collections::{ - hash_map::Entry::{Occupied, Vacant}, - HashMap, + btree_map::Entry::{Occupied, Vacant}, + BTreeMap, }; /// A change set produced by the VM. @@ -23,10 +23,10 @@ use std::collections::{ /// VM. For storage backends, use `ChangeSet`. #[derive(Debug, Clone, Eq, PartialEq)] pub struct VMChangeSet { - resource_write_set: HashMap, - module_write_set: HashMap, - aggregator_write_set: HashMap, - aggregator_delta_set: HashMap, + resource_write_set: BTreeMap, + module_write_set: BTreeMap, + aggregator_write_set: BTreeMap, + aggregator_delta_set: BTreeMap, events: Vec, } @@ -49,19 +49,19 @@ macro_rules! squash_writes_pair { impl VMChangeSet { pub fn empty() -> Self { Self { - resource_write_set: HashMap::new(), - module_write_set: HashMap::new(), - aggregator_write_set: HashMap::new(), - aggregator_delta_set: HashMap::new(), + resource_write_set: BTreeMap::new(), + module_write_set: BTreeMap::new(), + aggregator_write_set: BTreeMap::new(), + aggregator_delta_set: BTreeMap::new(), events: vec![], } } pub fn new( - resource_write_set: HashMap, - module_write_set: HashMap, - aggregator_write_set: HashMap, - aggregator_delta_set: HashMap, + resource_write_set: BTreeMap, + module_write_set: BTreeMap, + aggregator_write_set: BTreeMap, + aggregator_delta_set: BTreeMap, events: Vec, checker: &dyn CheckChangeSet, ) -> anyhow::Result { @@ -92,8 +92,8 @@ impl VMChangeSet { // There should be no aggregator writes if we have a change set from // storage. - let mut resource_write_set = HashMap::new(); - let mut module_write_set = HashMap::new(); + let mut resource_write_set = BTreeMap::new(); + let mut module_write_set = BTreeMap::new(); for (state_key, write_op) in write_set { if matches!(state_key.inner(), StateKeyInner::AccessPath(ap) if ap.is_code()) { @@ -109,8 +109,8 @@ impl VMChangeSet { let change_set = Self { resource_write_set, module_write_set, - aggregator_write_set: HashMap::new(), - aggregator_delta_set: HashMap::new(), + aggregator_write_set: BTreeMap::new(), + aggregator_delta_set: BTreeMap::new(), events, }; checker.check_change_set(&change_set)?; @@ -172,11 +172,11 @@ impl VMChangeSet { .chain(self.aggregator_write_set.iter_mut()) } - pub fn resource_write_set(&self) -> &HashMap { + pub fn resource_write_set(&self) -> &BTreeMap { &self.resource_write_set } - pub fn module_write_set(&self) -> &HashMap { + pub fn module_write_set(&self) -> &BTreeMap { &self.module_write_set } @@ -189,11 +189,11 @@ impl VMChangeSet { .extend(additional_aggregator_writes) } - pub fn aggregator_v1_write_set(&self) -> &HashMap { + pub fn aggregator_v1_write_set(&self) -> &BTreeMap { &self.aggregator_write_set } - pub fn aggregator_v1_delta_set(&self) -> &HashMap { + pub fn aggregator_v1_delta_set(&self) -> &BTreeMap { &self.aggregator_delta_set } @@ -222,23 +222,23 @@ impl VMChangeSet { aggregator_delta_set .into_iter() .map(into_write) - .collect::, VMStatus>>()?; + .collect::, VMStatus>>()?; aggregator_write_set.extend(materialized_aggregator_delta_set.into_iter()); Ok(Self { resource_write_set, module_write_set, aggregator_write_set, - aggregator_delta_set: HashMap::new(), + aggregator_delta_set: BTreeMap::new(), events, }) } fn squash_additional_aggregator_changes( - aggregator_write_set: &mut HashMap, - aggregator_delta_set: &mut HashMap, - additional_aggregator_write_set: HashMap, - additional_aggregator_delta_set: HashMap, + aggregator_write_set: &mut BTreeMap, + aggregator_delta_set: &mut BTreeMap, + additional_aggregator_write_set: BTreeMap, + additional_aggregator_delta_set: BTreeMap, ) -> anyhow::Result<(), VMStatus> { use WriteOp::*; @@ -321,8 +321,8 @@ impl VMChangeSet { } fn squash_additional_writes( - write_set: &mut HashMap, - additional_write_set: HashMap, + write_set: &mut BTreeMap, + additional_write_set: BTreeMap, ) -> anyhow::Result<(), VMStatus> { for (key, additional_write_op) in additional_write_set.into_iter() { match write_set.entry(key) { diff --git a/aptos-move/aptos-vm-types/src/tests/test_change_set.rs b/aptos-move/aptos-vm-types/src/tests/test_change_set.rs index d0dcc527ea6a7..6164ea5713a38 100644 --- a/aptos-move/aptos-vm-types/src/tests/test_change_set.rs +++ b/aptos-move/aptos-vm-types/src/tests/test_change_set.rs @@ -20,7 +20,7 @@ use move_core_types::{ language_storage::{ModuleId, StructTag}, vm_status::{StatusCode, VMStatus}, }; -use std::collections::HashMap; +use std::collections::BTreeMap; /// Testcases: /// ```text @@ -89,7 +89,7 @@ macro_rules! write_set_2 { macro_rules! expected_write_set { ($d:ident) => { - HashMap::from([ + BTreeMap::from([ mock_create(format!("0{}", $d), 0), mock_modify(format!("1{}", $d), 1), mock_delete(format!("2{}", $d)), @@ -164,13 +164,13 @@ fn test_successful_squash() { &expected_write_set!(descriptor) ); - let expected_aggregator_write_set = HashMap::from([ + let expected_aggregator_write_set = BTreeMap::from([ mock_create("18a", 136), mock_modify("19a", 138), mock_modify("22a", 122), mock_delete("23a"), ]); - let expected_aggregator_delta_set = HashMap::from([ + let expected_aggregator_delta_set = BTreeMap::from([ mock_add("15a", 15), mock_add("16a", 116), mock_add("17a", 134), diff --git a/aptos-move/aptos-vm-types/src/tests/test_output.rs b/aptos-move/aptos-vm-types/src/tests/test_output.rs index c30b443473e23..e9ec16d9b0c4e 100644 --- a/aptos-move/aptos-vm-types/src/tests/test_output.rs +++ b/aptos-move/aptos-vm-types/src/tests/test_output.rs @@ -12,7 +12,7 @@ use aptos_types::{ }; use claims::{assert_err, assert_matches, assert_ok}; use move_core_types::vm_status::{AbortLocation, VMStatus}; -use std::collections::{BTreeMap, HashMap}; +use std::collections::BTreeMap; fn assert_eq_outputs(vm_output: &VMOutput, txn_output: TransactionOutput) { let vm_output_writes = &vm_output @@ -77,7 +77,8 @@ fn test_ok_output_equality_with_deltas() { .clone() .into_transaction_output_with_materialized_deltas(vec![mock_modify("3", 400)]); - let expected_aggregator_write_set = HashMap::from([mock_modify("2", 2), mock_modify("3", 400)]); + let expected_aggregator_write_set = + BTreeMap::from([mock_modify("2", 2), mock_modify("3", 400)]); assert_eq!( materialized_vm_output.change_set().resource_write_set(), vm_output.change_set().resource_write_set() diff --git a/aptos-move/aptos-vm-types/src/tests/utils.rs b/aptos-move/aptos-vm-types/src/tests/utils.rs index cd3c1f8f46de1..a6dfd66e960be 100644 --- a/aptos-move/aptos-vm-types/src/tests/utils.rs +++ b/aptos-move/aptos-vm-types/src/tests/utils.rs @@ -10,7 +10,7 @@ use aptos_types::{ write_set::WriteOp, }; use move_core_types::vm_status::VMStatus; -use std::collections::HashMap; +use std::collections::BTreeMap; pub(crate) struct MockChangeSetChecker; @@ -57,10 +57,10 @@ pub(crate) fn build_change_set( aggregator_delta_set: impl IntoIterator, ) -> VMChangeSet { VMChangeSet::new( - HashMap::from_iter(resource_write_set), - HashMap::from_iter(module_write_set), - HashMap::from_iter(aggregator_write_set), - HashMap::from_iter(aggregator_delta_set), + BTreeMap::from_iter(resource_write_set), + BTreeMap::from_iter(module_write_set), + BTreeMap::from_iter(aggregator_write_set), + BTreeMap::from_iter(aggregator_delta_set), vec![], &MockChangeSetChecker, ) diff --git a/aptos-move/aptos-vm/src/block_executor/mod.rs b/aptos-move/aptos-vm/src/block_executor/mod.rs index d7dfbe851c046..c0398d511374e 100644 --- a/aptos-move/aptos-vm/src/block_executor/mod.rs +++ b/aptos-move/aptos-vm/src/block_executor/mod.rs @@ -38,7 +38,7 @@ use aptos_vm_types::output::VMOutput; use move_core_types::vm_status::VMStatus; use once_cell::sync::OnceCell; use rayon::{prelude::*, ThreadPool}; -use std::{collections::HashMap, sync::Arc}; +use std::{collections::BTreeMap, sync::Arc}; impl BlockExecutorTransaction for PreprocessedTransaction { type Event = ContractEvent; @@ -90,7 +90,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput { /// Should never be called after incorporate_delta_writes, as it /// will consume vm_output to prepare an output with deltas. - fn resource_write_set(&self) -> HashMap { + fn resource_write_set(&self) -> BTreeMap { self.vm_output .lock() .as_ref() @@ -102,7 +102,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput { /// Should never be called after incorporate_delta_writes, as it /// will consume vm_output to prepare an output with deltas. - fn module_write_set(&self) -> HashMap { + fn module_write_set(&self) -> BTreeMap { self.vm_output .lock() .as_ref() @@ -114,7 +114,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput { /// Should never be called after incorporate_delta_writes, as it /// will consume vm_output to prepare an output with deltas. - fn aggregator_v1_write_set(&self) -> HashMap { + fn aggregator_v1_write_set(&self) -> BTreeMap { self.vm_output .lock() .as_ref() @@ -126,7 +126,7 @@ impl BlockExecutorTransactionOutput for AptosTransactionOutput { /// Should never be called after incorporate_delta_writes, as it /// will consume vm_output to prepare an output with deltas. - fn aggregator_v1_delta_set(&self) -> HashMap { + fn aggregator_v1_delta_set(&self) -> BTreeMap { self.vm_output .lock() .as_ref() diff --git a/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs b/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs index 2af771383bc50..3252533965678 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/respawned_session.rs @@ -151,7 +151,7 @@ mod test { use aptos_language_e2e_tests::data_store::FakeDataStore; use aptos_types::write_set::WriteOp; use aptos_vm_types::check_change_set::CheckChangeSet; - use std::collections::HashMap; + use std::collections::BTreeMap; /// A mock for testing. Always succeeds on checking a change set. struct NoOpChangeSetChecker; @@ -187,23 +187,23 @@ mod test { base_view.set_legacy(key("aggregator_both"), serialize(&60)); base_view.set_legacy(key("aggregator_delta_set"), serialize(&70)); - let resource_write_set = HashMap::from([ + let resource_write_set = BTreeMap::from([ (key("resource_both"), write(80)), (key("resource_write_set"), write(90)), ]); - let module_write_set = HashMap::from([ + let module_write_set = BTreeMap::from([ (key("module_both"), write(100)), (key("module_write_set"), write(110)), ]); - let aggregator_write_set = HashMap::from([ + let aggregator_write_set = BTreeMap::from([ (key("aggregator_both"), write(120)), (key("aggregator_write_set"), write(130)), ]); let aggregator_delta_set = - HashMap::from([(key("aggregator_delta_set"), delta_add(1, 1000))]); + BTreeMap::from([(key("aggregator_delta_set"), delta_add(1, 1000))]); let change_set = VMChangeSet::new( resource_write_set, diff --git a/aptos-move/aptos-vm/src/move_vm_ext/session.rs b/aptos-move/aptos-vm/src/move_vm_ext/session.rs index 72227b0b8ddfa..a16caee63bc1a 100644 --- a/aptos-move/aptos-vm/src/move_vm_ext/session.rs +++ b/aptos-move/aptos-vm/src/move_vm_ext/session.rs @@ -34,7 +34,7 @@ use move_vm_runtime::{move_vm::MoveVM, session::Session}; use serde::{Deserialize, Serialize}; use std::{ borrow::BorrowMut, - collections::{BTreeMap, HashMap}, + collections::BTreeMap, ops::{Deref, DerefMut}, sync::Arc, }; @@ -318,10 +318,10 @@ impl<'r, 'l> SessionExt<'r, 'l> { new_slot_metadata, }; - let mut resource_write_set = HashMap::new(); - let mut module_write_set = HashMap::new(); - let mut aggregator_write_set = HashMap::new(); - let mut aggregator_delta_set = HashMap::new(); + let mut resource_write_set = BTreeMap::new(); + let mut module_write_set = BTreeMap::new(); + let mut aggregator_write_set = BTreeMap::new(); + let mut aggregator_delta_set = BTreeMap::new(); for (addr, account_changeset) in change_set.into_inner() { let (modules, resources) = account_changeset.into_inner(); diff --git a/aptos-move/block-executor/src/proptest_types/types.rs b/aptos-move/block-executor/src/proptest_types/types.rs index ea80c7d74cf1d..da054a5c80cc7 100644 --- a/aptos-move/block-executor/src/proptest_types/types.rs +++ b/aptos-move/block-executor/src/proptest_types/types.rs @@ -25,7 +25,7 @@ use once_cell::sync::OnceCell; use proptest::{arbitrary::Arbitrary, collection::vec, prelude::*, proptest, sample::Index}; use proptest_derive::Arbitrary; use std::{ - collections::{hash_map::DefaultHasher, BTreeSet, HashMap}, + collections::{hash_map::DefaultHasher, BTreeMap, BTreeSet}, convert::TryInto, fmt::Debug, hash::{Hash, Hasher}, @@ -571,7 +571,7 @@ where { type Txn = MockTransaction; - fn resource_write_set(&self) -> HashMap { + fn resource_write_set(&self) -> BTreeMap { self.writes .iter() .filter(|(k, _)| k.module_path().is_none()) @@ -579,7 +579,7 @@ where .collect() } - fn module_write_set(&self) -> HashMap { + fn module_write_set(&self) -> BTreeMap { self.writes .iter() .filter(|(k, _)| k.module_path().is_some()) @@ -589,11 +589,11 @@ where // Aggregator v1 writes are included in resource_write_set for tests (writes are produced // for all keys including ones for v1_aggregators without distinguishing). - fn aggregator_v1_write_set(&self) -> HashMap { - HashMap::new() + fn aggregator_v1_write_set(&self) -> BTreeMap { + BTreeMap::new() } - fn aggregator_v1_delta_set(&self) -> HashMap { + fn aggregator_v1_delta_set(&self) -> BTreeMap { self.deltas.iter().cloned().collect() } diff --git a/aptos-move/block-executor/src/task.rs b/aptos-move/block-executor/src/task.rs index 228eab71cacb1..4299106e3299e 100644 --- a/aptos-move/block-executor/src/task.rs +++ b/aptos-move/block-executor/src/task.rs @@ -11,7 +11,7 @@ use aptos_types::{ fee_statement::FeeStatement, write_set::{TransactionWrite, WriteOp}, }; -use std::{collections::HashMap, fmt::Debug, hash::Hash}; +use std::{collections::BTreeMap, fmt::Debug, hash::Hash}; /// The execution result of a transaction #[derive(Debug)] @@ -78,18 +78,18 @@ pub trait TransactionOutput: Send + Sync + Debug { /// aggregator_v1. fn resource_write_set( &self, - ) -> HashMap<::Key, ::Value>; + ) -> BTreeMap<::Key, ::Value>; fn module_write_set( &self, - ) -> HashMap<::Key, ::Value>; + ) -> BTreeMap<::Key, ::Value>; fn aggregator_v1_write_set( &self, - ) -> HashMap<::Key, ::Value>; + ) -> BTreeMap<::Key, ::Value>; /// Get the aggregator deltas of a transaction from its output. - fn aggregator_v1_delta_set(&self) -> HashMap<::Key, DeltaOp>; + fn aggregator_v1_delta_set(&self) -> BTreeMap<::Key, DeltaOp>; /// Get the events of a transaction from its output. fn get_events(&self) -> Vec<::Event>;