-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[benchmark] Add option for package overrides #15841
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,7 @@ use crate::{ | |
overrides::OverrideConfig, | ||
workload::TransactionBlock, | ||
}; | ||
use anyhow::{anyhow, bail}; | ||
use aptos_gas_schedule::LATEST_GAS_FEATURE_VERSION; | ||
use anyhow::anyhow; | ||
use aptos_logger::Level; | ||
use aptos_types::on_chain_config::FeatureFlag; | ||
use clap::Parser; | ||
|
@@ -55,26 +54,20 @@ pub struct InitializeCommand { | |
help = "If set, overrides the gas feature version used by the gas schedule" | ||
)] | ||
gas_feature_version: Option<u64>, | ||
|
||
#[clap( | ||
long, | ||
num_args = 1.., | ||
value_delimiter = ' ', | ||
help = "List of space-separated paths to compiled / built packages with Move code" | ||
)] | ||
override_packages: Vec<String>, | ||
} | ||
|
||
impl InitializeCommand { | ||
pub async fn initialize_inputs(self) -> anyhow::Result<()> { | ||
init_logger_and_metrics(self.log_level); | ||
|
||
if !self | ||
.enable_features | ||
.iter() | ||
.all(|f| !self.disable_features.contains(f)) | ||
{ | ||
bail!("Enabled and disabled feature flags cannot overlap") | ||
} | ||
if matches!(self.gas_feature_version, Some(v) if v > LATEST_GAS_FEATURE_VERSION) { | ||
bail!( | ||
"Gas feature version must be at most the latest one: {}", | ||
LATEST_GAS_FEATURE_VERSION | ||
); | ||
} | ||
|
||
let bytes = fs::read(PathBuf::from(&self.transactions_file)).await?; | ||
let txn_blocks: Vec<TransactionBlock> = bcs::from_bytes(&bytes).map_err(|err| { | ||
anyhow!( | ||
|
@@ -84,16 +77,16 @@ impl InitializeCommand { | |
})?; | ||
|
||
// TODO: | ||
// Right now, only features can be overridden. In the future, we may want to support: | ||
// 1. Framework code, e.g., to test performance of new natives or compiler, | ||
// 2. Gas schedule, to track the costs of charging gas or tracking limits. | ||
// 3. BlockExecutorConfigFromOnchain to experiment with different block cutting based | ||
// on gas limits. | ||
// 1. Override gas schedule, to track the costs of charging gas or tracking limits. | ||
// 2. BlockExecutorConfigFromOnchain to experiment with different block cutting based | ||
// on gas limits?. | ||
// 3. Build options for package overrides. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, so this would allow pointing to just source. Perhaps mention that right now, the build option is only used for getting the bytecode version (and that this wont recompile the package with move-2). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now compilation happens with hardcoded options, ideally one should be able to paramterize this, do you have specific things in mind I should add? e.g., how do I enable optimization levels, etc. My original plan was to point to already build package (so that using compiler is external), but I did not find a way to retrieve the package from the filesystem (i.e., we don't have APIs for that, etc.) so instead just used this for now. You can probably suggest more here from compiler side? |
||
let override_config = OverrideConfig::new( | ||
self.enable_features, | ||
self.disable_features, | ||
self.gas_feature_version, | ||
); | ||
self.override_packages, | ||
)?; | ||
|
||
let debugger = build_debugger(self.rest_api.rest_endpoint, self.rest_api.api_key)?; | ||
let inputs = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,34 +3,89 @@ | |
|
||
//! Defines different overrides for on-chain state used for benchmarking. With overrides, past | ||
//! transactions can be replayed on top of a modified state, and we can evaluate how it impacts | ||
georgemitenkov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
//! performance or other things. | ||
//! performance or other things. Supported overrides include: | ||
//! 1. enabling feature flags, | ||
//! 2. disabling feature flags, | ||
//! 3. overriding gas feature version, | ||
//! 4. changing modules (bytecode, metadata, etc.) and package information. | ||
use anyhow::bail; | ||
use aptos_framework::{natives::code::PackageRegistry, BuildOptions, BuiltPackage}; | ||
use aptos_gas_schedule::LATEST_GAS_FEATURE_VERSION; | ||
use aptos_logger::error; | ||
use aptos_types::{ | ||
on_chain_config::{FeatureFlag, Features, GasScheduleV2, OnChainConfig}, | ||
state_store::{state_key::StateKey, state_value::StateValue, StateView}, | ||
}; | ||
use serde::Serialize; | ||
use std::collections::HashMap; | ||
use std::{ | ||
collections::{BTreeSet, HashMap}, | ||
path::PathBuf, | ||
}; | ||
|
||
/// Stores information about compiled Move packages and the build options used to create them. Used | ||
/// by the override configuration to shadow existing on-chain modules with modules defined in these | ||
/// packages. | ||
struct PackageOverride { | ||
packages: Vec<BuiltPackage>, | ||
build_options: BuildOptions, | ||
} | ||
|
||
impl PackageOverride { | ||
/// Uses the provided build options to build multiple packages from the specified paths. | ||
fn new(package_paths: Vec<String>, build_options: BuildOptions) -> anyhow::Result<Self> { | ||
let packages = package_paths | ||
.into_iter() | ||
.map(|path| BuiltPackage::build(PathBuf::from(&path), build_options.clone())) | ||
.collect::<anyhow::Result<_>>()?; | ||
Ok(Self { | ||
packages, | ||
build_options, | ||
}) | ||
} | ||
} | ||
|
||
/// Stores feature flags to enable/disable, essentially overriding on-chain state. | ||
/// Stores all state overrides. | ||
pub struct OverrideConfig { | ||
/// Feature flags to enable. Invariant: does not overlap with disabled features. | ||
additional_enabled_features: Vec<FeatureFlag>, | ||
/// Feature flags to disable. Invariant: does not overlap with enabled features. | ||
additional_disabled_features: Vec<FeatureFlag>, | ||
/// Gas feature version to use. Invariant: must be at most the latest version. | ||
gas_feature_version: Option<u64>, | ||
/// Information about overridden packages. | ||
package_override: PackageOverride, | ||
} | ||
|
||
impl OverrideConfig { | ||
pub fn new( | ||
additional_enabled_features: Vec<FeatureFlag>, | ||
additional_disabled_features: Vec<FeatureFlag>, | ||
gas_feature_version: Option<u64>, | ||
) -> Self { | ||
Self { | ||
override_packages: Vec<String>, | ||
) -> anyhow::Result<Self> { | ||
let build_options = BuildOptions::move_2(); | ||
let package_override = PackageOverride::new(override_packages, build_options)?; | ||
|
||
if !additional_enabled_features | ||
.iter() | ||
.all(|f| !additional_disabled_features.contains(f)) | ||
{ | ||
bail!("Enabled and disabled feature flags cannot overlap") | ||
} | ||
if matches!(gas_feature_version, Some(v) if v > LATEST_GAS_FEATURE_VERSION) { | ||
bail!( | ||
"Gas feature version must be at most the latest one: {}", | ||
LATEST_GAS_FEATURE_VERSION | ||
); | ||
} | ||
|
||
Ok(Self { | ||
additional_enabled_features, | ||
additional_disabled_features, | ||
gas_feature_version, | ||
} | ||
package_override, | ||
}) | ||
} | ||
|
||
pub(crate) fn get_state_override( | ||
|
@@ -73,6 +128,123 @@ impl OverrideConfig { | |
state_override.insert(gas_schedule_state_key, gas_schedule_state_value); | ||
} | ||
|
||
// Override packages. | ||
let mut overridden_package_registries = HashMap::new(); | ||
for package in &self.package_override.packages { | ||
// Modify existing package metadata or add new one. | ||
let addresses = package | ||
.modules() | ||
.map(|m| *m.self_addr()) | ||
.collect::<BTreeSet<_>>(); | ||
assert_eq!( | ||
addresses.len(), | ||
1, | ||
"Modules in the same package must have the same address" | ||
); | ||
|
||
let package_address = addresses | ||
.last() | ||
.expect("Package must contain at least one module"); | ||
let package_registry_state_key = | ||
StateKey::resource(package_address, &PackageRegistry::struct_tag()) | ||
.expect("Should always be able to create state key for package registry"); | ||
|
||
let old_package_registry_state_value = | ||
match overridden_package_registries.remove(&package_registry_state_key) { | ||
Some(state_value) => state_value, | ||
georgemitenkov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
None => state_view | ||
.get_state_value(&package_registry_state_key) | ||
.unwrap_or_else(|err| { | ||
panic!( | ||
"Failed to fetch package registry at {}: {:?}", | ||
package_address, err | ||
) | ||
}) | ||
.expect("Package registry for override must always exist"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an easy workflow to emulate publishing a new package and using that elsewhere? (but i guess for that purpose, can always include the new one inside the updated package)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I know of, package system was built on top of the modules so it is quite hacky |
||
}; | ||
|
||
let metadata = package.extract_metadata().unwrap_or_else(|err| { | ||
panic!( | ||
"Failed to extract metadata for package {}: {:?}", | ||
package.name(), | ||
err | ||
) | ||
}); | ||
let new_package_registry_state_value = old_package_registry_state_value | ||
.map_bytes(|bytes| { | ||
let mut package_registry = bcs::from_bytes::<PackageRegistry>(&bytes) | ||
.expect("Package registry should deserialize"); | ||
|
||
let mut metadata_idx = None; | ||
for (idx, package_metadata) in package_registry.packages.iter().enumerate() { | ||
if package_metadata.name == metadata.name { | ||
metadata_idx = Some(idx); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we can assign it here and only handle None below? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure, you need to clone metadata then (Rust complains)? Even though it is clearly that metadata cannot be moved if we fallback to break. |
||
break; | ||
} | ||
} | ||
match metadata_idx { | ||
Some(idx) => { | ||
package_registry.packages[idx] = metadata; | ||
}, | ||
None => { | ||
package_registry.packages.push(metadata); | ||
}, | ||
} | ||
|
||
let bytes = bcs::to_bytes(&package_registry) | ||
.expect("Package registry should serialize"); | ||
Ok(bytes.into()) | ||
}) | ||
.expect("Modifying package never returns an error"); | ||
|
||
overridden_package_registries | ||
.insert(package_registry_state_key, new_package_registry_state_value); | ||
|
||
// Modify all existing modules or add new ones. | ||
let bytecode_version = self.package_override.build_options.bytecode_version; | ||
for module in package.modules() { | ||
let mut module_bytes = vec![]; | ||
module | ||
.serialize_for_version(bytecode_version, &mut module_bytes) | ||
.unwrap_or_else(|err| { | ||
panic!( | ||
"Failed to serialize module {}::{}: {:?}", | ||
module.self_addr(), | ||
module.self_name(), | ||
err | ||
) | ||
}); | ||
|
||
let state_key = StateKey::module(module.self_addr(), module.self_name()); | ||
let onchain_state_value = | ||
state_view | ||
.get_state_value(&state_key) | ||
.unwrap_or_else(|err| { | ||
panic!( | ||
"Failed to fetch module {}::{}: {:?}", | ||
module.self_addr(), | ||
module.self_name(), | ||
err | ||
) | ||
}); | ||
let state_value = match onchain_state_value { | ||
georgemitenkov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Some(state_value) => { | ||
state_value.map_bytes(|_| Ok(module_bytes.into())).unwrap() | ||
}, | ||
|
||
None => StateValue::new_legacy(module_bytes.into()), | ||
}; | ||
if state_override.insert(state_key, state_value).is_some() { | ||
panic!( | ||
"Overriding module {}::{} more than once", | ||
module.self_addr(), | ||
module.self_name() | ||
); | ||
} | ||
} | ||
} | ||
state_override.extend(overridden_package_registries); | ||
|
||
state_override | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another cool feature this brings (and I think we should be explicit about it in the readme) is that you can compile using different compiler versions/optimization levels/experimental compiler stuff and just point to the built packages, giving the ability to compare them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes yes yes! Exactly, but for that I need to modify CLI to pass flags to build option, or instead get already build packages from the path (ideally)