Skip to content

Commit

Permalink
[gas] optimize dependency gas by avoid allocating new ModuleId
Browse files Browse the repository at this point in the history
  • Loading branch information
@vgao1996
vgao1996 committed Feb 24, 2024
1 parent 4849a24 commit 25f4925
Show file tree
Hide file tree
Showing 18 changed files with 322 additions and 94 deletions.
3 changes: 3 additions & 0 deletions Cargo.lock
View file

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

10 changes: 8 additions & 2 deletions aptos-move/aptos-gas-meter/src/meter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use move_binary_format::{
use move_core_types::{
account_address::AccountAddress,
gas_algebra::{InternalGas, NumArgs, NumBytes},
identifier::IdentStr,
language_storage::ModuleId,
vm_status::StatusCode,
};
Expand Down Expand Up @@ -471,9 +472,14 @@ where
}

#[inline]
fn charge_dependency(&mut self, module_id: &ModuleId, size: NumBytes) -> PartialVMResult<()> {
fn charge_dependency(
&mut self,
addr: &AccountAddress,
_name: &IdentStr,
size: NumBytes,
) -> PartialVMResult<()> {
// TODO: How about 0xA550C18?
if self.feature_version() >= 14 && !module_id.address().is_special() {
if self.feature_version() >= 14 && !addr.is_special() {
self.algebra
.charge_execution(DEPENDENCY_PER_MODULE + DEPENDENCY_PER_BYTE * size)?;
self.algebra.count_dependency(size)?;
Expand Down
13 changes: 9 additions & 4 deletions aptos-move/aptos-gas-profiling/src/profiler.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use move_binary_format::{
};
use move_core_types::{
account_address::AccountAddress,
identifier::Identifier,
identifier::{IdentStr, Identifier},
language_storage::{ModuleId, TypeTag},
};
use move_vm_types::{
Expand Down Expand Up @@ -470,12 +470,17 @@ where
res
}

fn charge_dependency(&mut self, module_id: &ModuleId, size: NumBytes) -> PartialVMResult<()> {
let (cost, res) = self.delegate_charge(|base| base.charge_dependency(module_id, size));
fn charge_dependency(
&mut self,
addr: &AccountAddress,
name: &IdentStr,
size: NumBytes,
) -> PartialVMResult<()> {
let (cost, res) = self.delegate_charge(|base| base.charge_dependency(addr, name, size));

if !cost.is_zero() {
self.dependencies.push(Dependency {
id: module_id.clone(),
id: ModuleId::new(*addr, name.to_owned()),
size,
cost,
});
Expand Down
7 changes: 5 additions & 2 deletions aptos-move/aptos-memory-usage-tracker/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ use move_binary_format::{
errors::{PartialVMError, PartialVMResult, VMResult},
file_format::CodeOffset,
};
use move_core_types::{language_storage::ModuleId, vm_status::StatusCode};
use move_core_types::{
account_address::AccountAddress, identifier::IdentStr, language_storage::ModuleId,
vm_status::StatusCode,
};
use move_vm_types::{
gas::{GasMeter as MoveGasMeter, SimpleInstruction},
views::{TypeView, ValueView},
Expand Down Expand Up @@ -161,7 +164,7 @@ where

fn charge_create_ty(&mut self, num_nodes: NumTypeNodes) -> PartialVMResult<()>;

fn charge_dependency(&mut self, module_id: &ModuleId, size: NumBytes) -> PartialVMResult<()>;
fn charge_dependency(&mut self, addr: &AccountAddress, name: &IdentStr, size: NumBytes) -> PartialVMResult<()>;
}

#[inline]
Expand Down
25 changes: 17 additions & 8 deletions aptos-move/aptos-vm/src/aptos_vm.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -686,7 +686,11 @@ impl AptosVM {
// result in shallow-loading of the modules and therefore subtle changes in
// the error semantics.
if self.gas_feature_version >= 14 {
session.check_dependencies_and_charge_gas(gas_meter, [entry_fn.module().clone()])?;
let module_id = entry_fn.module();
session.check_dependencies_and_charge_gas(gas_meter, [(
module_id.address(),
module_id.name(),
)])?;
}

Check warning on line 695 in aptos-move/aptos-vm/src/aptos_vm.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/aptos_vm.rs#L685-L695 

Added lines #L685 - L695 were not covered by tests
let function =
Expand Down Expand Up @@ -1009,7 +1013,11 @@ impl AptosVM {
// result in shallow-loading of the modules and therefore subtle changes in
// the error semantics.
if self.gas_feature_version >= 14 {
session.check_dependencies_and_charge_gas(gas_meter, [payload.module().clone()])?;
let module_id = payload.module();
session.check_dependencies_and_charge_gas(gas_meter, [(
module_id.address(),
module_id.name(),
)])?;
}

Check warning on line 1022 in aptos-move/aptos-vm/src/aptos_vm.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/aptos_vm.rs#L1012-L1022 

Added lines #L1012 - L1022 were not covered by tests
// If txn args are not valid, we'd still consider the transaction as executed but
Expand Down Expand Up @@ -1191,9 +1199,11 @@ impl AptosVM {
if self.gas_feature_version >= 14 {
// Charge all modules in the bundle that is about to be published.
for (module, blob) in modules.iter().zip(bundle.iter()) {
let module_id = &module.self_id();

Check warning on line 1202 in aptos-move/aptos-vm/src/aptos_vm.rs

View check run for this annotation

Codecov / codecov/patch

aptos-move/aptos-vm/src/aptos_vm.rs#L1202 

Added line #L1202 was not covered by tests
gas_meter
.charge_dependency(
&module.self_id(),
module_id.address(),
module_id.name(),
NumBytes::new(blob.code().len() as u64),
)
.map_err(|err| err.finish(Location::Undefined))?;
Expand All @@ -1205,7 +1215,7 @@ impl AptosVM {
// been published yet.
let module_ids_in_bundle = modules
.iter()
.map(|module| module.self_id())
.map(|module| (module.self_addr(), module.self_name()))
.collect::<BTreeSet<_>>();

session.check_dependencies_and_charge_gas(
Expand All @@ -1214,11 +1224,10 @@ impl AptosVM {
.iter()
.flat_map(|module| {
module
.immediate_dependencies()
.into_iter()
.chain(module.immediate_friends())
.immediate_dependencies_iter()
.chain(module.immediate_friends_iter())
})
.filter(|module_id| !module_ids_in_bundle.contains(module_id)),
.filter(|addr_and_name| !module_ids_in_bundle.contains(addr_and_name)),
)?;

// TODO: Revisit the order of traversal. Consider switching to alphabetical order.
Expand Down
97 changes: 66 additions & 31 deletions aptos-move/e2e-benchmark/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,21 @@
// SPDX-License-Identifier: Apache-2.0

#[cfg(test)]
mod test{
use aptos_language_e2e_tests::{account::Account, executor::{ExecFuncTimerDynamicArgs, FakeExecutor, GasMeterType}};
use aptos_transaction_generator_lib::{publishing::{module_simple::{AutomaticArgs, LoopType, MultiSigConfig}, publish_util::{Package, PackageHandler}}, EntryPoints};
mod test {
use aptos_language_e2e_tests::{
account::Account,
executor::{ExecFuncTimerDynamicArgs, FakeExecutor, GasMeterType},
};
use aptos_transaction_generator_lib::{
publishing::{
module_simple::{AutomaticArgs, LoopType, MultiSigConfig},
publish_util::{Package, PackageHandler},
},
EntryPoints,
};
use aptos_types::{transaction::TransactionPayload, PeerId};
use rand::{rngs::StdRng, SeedableRng};


pub fn execute_txn(
executor: &mut FakeExecutor,
account: &Account,
Expand All @@ -29,13 +37,21 @@ mod test{
assert!(txn_output.status().status().unwrap().is_success());
}

fn execute_and_time_entry_point(entry_point: &EntryPoints, package: &Package, publisher_address: &PeerId, executor: &mut FakeExecutor, iterations: u64) -> u128 {
fn execute_and_time_entry_point(
entry_point: &EntryPoints,
package: &Package,
publisher_address: &PeerId,
executor: &mut FakeExecutor,
iterations: u64,
) -> u128 {
let mut rng = StdRng::seed_from_u64(14);
let entry_fun = entry_point.create_payload(
package.get_module_id(entry_point.module_name()),
Some(&mut rng),
Some(publisher_address),
).into_entry_function();
let entry_fun = entry_point
.create_payload(
package.get_module_id(entry_point.module_name()),
Some(&mut rng),
Some(publisher_address),
)
.into_entry_function();

executor.exec_func_record_running_time_with_dynamic_args(
entry_fun.module(),
Expand All @@ -46,12 +62,14 @@ mod test{
match entry_point.automatic_args() {
AutomaticArgs::None => ExecFuncTimerDynamicArgs::NoArgs,
AutomaticArgs::Signer => ExecFuncTimerDynamicArgs::DistinctSigners,
AutomaticArgs::SignerAndMultiSig => {
match entry_point.multi_sig_additional_num() {
MultiSigConfig::Publisher => ExecFuncTimerDynamicArgs::DistinctSignersAndFixed(vec![publisher_address.clone()]),
_ => todo!(),
}
}
AutomaticArgs::SignerAndMultiSig => match entry_point.multi_sig_additional_num() {
MultiSigConfig::Publisher => {
ExecFuncTimerDynamicArgs::DistinctSignersAndFixed(vec![
publisher_address.clone()
])
},
_ => todo!(),
},
},
GasMeterType::RegularMeter,
)
Expand Down Expand Up @@ -134,13 +152,23 @@ mod test{
let mut package_handler = PackageHandler::new(entry_point.package_name());
let mut rng = StdRng::seed_from_u64(14);
let package = package_handler.pick_package(&mut rng, *publisher.address());
execute_txn(&mut executor, &publisher, 0, package.publish_transaction_payload());
execute_txn(
&mut executor,
&publisher,
0,
package.publish_transaction_payload(),
);
if let Some(init_entry_point) = entry_point.initialize_entry_point() {
execute_txn(&mut executor, &publisher, 1, init_entry_point.create_payload(
package.get_module_id(init_entry_point.module_name()),
Some(&mut rng),
Some(publisher.address()),
));
execute_txn(
&mut executor,
&publisher,
1,
init_entry_point.create_payload(
package.get_module_id(init_entry_point.module_name()),
Some(&mut rng),
Some(publisher.address()),
),
);
}

let elapsed_micros = execute_and_time_entry_point(
Expand All @@ -154,14 +182,25 @@ mod test{
10
} else {
100
}
},
);
println!(
"{}us\texpected {}us\t{:?}: ",
elapsed_micros, expected_time, entry_point
);
println!("{}us\texpected {}us\t{:?}: ", elapsed_micros, expected_time, entry_point);

if elapsed_micros as f32 > expected_time as f32 * (1.0 + ALLOWED_REGRESSION) + 2.0 {
results.push(format!("Performance regression detected: {}us\texpected {}us\t{:?}: ", elapsed_micros, expected_time, entry_point));
} else if elapsed_micros as f32 + 2.0 < expected_time as f32 * (1.0 - ALLOWED_IMPROVEMENT) {
results.push(format!("Performance improvement detected: {}us\texpected {}us\t{:?}: ", elapsed_micros, expected_time, entry_point));
results.push(format!(
"Performance regression detected: {}us\texpected {}us\t{:?}: ",
elapsed_micros, expected_time, entry_point
));
} else if elapsed_micros as f32 + 2.0
< expected_time as f32 * (1.0 - ALLOWED_IMPROVEMENT)
{
results.push(format!(
"Performance improvement detected: {}us\texpected {}us\t{:?}: ",
elapsed_micros, expected_time, entry_point
));
}
// } else {
// println!("Skipping multisig {entry_point:?}");
Expand All @@ -174,8 +213,6 @@ mod test{
assert!(results.is_empty());
}



// anyhow = { workspace = true }
// aptos = { workspace = true }
// aptos-abstract-gas-usage = { workspace = true }
Expand All @@ -198,9 +235,7 @@ mod test{
// move-vm-test-utils = { workspace = true }
// walkdir = { workspace = true }


// use aptos_language_e2e_tests::executor::FakeExecutor;
// use aptos_transaction_generator_lib::EntryPoints;
// use aptos_types::PeerId;

}
29 changes: 23 additions & 6 deletions aptos-move/e2e-tests/src/executor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ use aptos_gas_algebra::DynamicExpression;
use aptos_gas_meter::{StandardGasAlgebra, StandardGasMeter};
use aptos_gas_profiling::{GasProfiler, TransactionGasLog};
use aptos_gas_schedule::{
AptosGasParameters, InitialGasSchedule, MiscGasParameters, NativeGasParameters, LATEST_GAS_FEATURE_VERSION
AptosGasParameters, InitialGasSchedule, MiscGasParameters, NativeGasParameters,
LATEST_GAS_FEATURE_VERSION,
};
use aptos_keygen::KeyGen;
use aptos_memory_usage_tracker::MemoryTrackedGasMeter;
Expand Down Expand Up @@ -834,9 +835,13 @@ impl FakeExecutor {

let mut extra_accounts = match &dynamic_args {
ExecFuncTimerDynamicArgs::DistinctSigners
| ExecFuncTimerDynamicArgs::DistinctSignersAndFixed(_) => {
(0..iterations).map(|_| self.new_account_at(AccountAddress::random()).address().clone()).collect::<Vec<_>>()
},
| ExecFuncTimerDynamicArgs::DistinctSignersAndFixed(_) => (0..iterations)
.map(|_| {
self.new_account_at(AccountAddress::random())
.address()
.clone()
})
.collect::<Vec<_>>(),
_ => vec![],
};

Expand Down Expand Up @@ -906,8 +911,20 @@ impl FakeExecutor {

let start = Instant::now();
let result = match gas_meter_type {
GasMeterType::RegularMeter => session.execute_function_bypass_visibility(module, &fun_name, ty, arg, regular.as_mut().unwrap()),
GasMeterType::UnmeteredGasMeter => session.execute_function_bypass_visibility(module, &fun_name, ty, arg, unmetered.as_mut().unwrap()),
GasMeterType::RegularMeter => session.execute_function_bypass_visibility(
module,
&fun_name,
ty,
arg,
regular.as_mut().unwrap(),
),
GasMeterType::UnmeteredGasMeter => session.execute_function_bypass_visibility(
module,
&fun_name,
ty,
arg,
unmetered.as_mut().unwrap(),
),
};
let elapsed = start.elapsed();
if let Err(err) = result {
Expand Down
Loading

0 comments on commit 25f4925

Please sign in to comment.