Skip to content

Commit

Permalink
Bench runner fix genesis storage (from #5083)
Browse files Browse the repository at this point in the history
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Fix changes_to_storage

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Better error messages

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

genesis-preset flag

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Bench runner fix genesis storage

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

clippy
  • Loading branch information
ggwpez authored and bkontur committed Sep 2, 2024
1 parent bc2ea69 commit b74e6ca
Show file tree
Hide file tree
Showing 6 changed files with 81 additions and 37 deletions.
2 changes: 1 addition & 1 deletion substrate/primitives/state-machine/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ mod ext;
pub mod fuzzing;
#[cfg(feature = "std")]
mod in_memory_backend;
pub(crate) mod overlayed_changes;
pub mod overlayed_changes; // FAIL-CI
#[cfg(feature = "std")]
mod read_only;
mod stats;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,12 @@ pub enum ExecutionMode {
Runtime,
}

#[allow(missing_docs)]
#[derive(Debug, Default, Clone)]
#[cfg_attr(test, derive(PartialEq))]
struct InnerValue<V> {
pub struct InnerValue<V> {
/// Current value. None if value has been deleted.
value: V,
pub value: V,
/// The set of extrinsic indices where the values has been changed.
extrinsics: Extrinsics,
}
Expand All @@ -80,7 +81,7 @@ struct InnerValue<V> {
pub struct OverlayedEntry<V> {
/// The individual versions of that value.
/// One entry per transactions during that the value was actually written.
transactions: Transactions<V>,
pub transactions: Transactions<V>,
}

impl<V> Default for OverlayedEntry<V> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

//! The overlayed changes to state.

mod changeset;
pub mod changeset; // FAIL-CI
mod offchain;

use self::changeset::OverlayedChangeSet;
Expand Down Expand Up @@ -93,7 +93,7 @@ impl Extrinsics {
/// It allows changes to be modified using nestable transactions.
pub struct OverlayedChanges<H: Hasher> {
/// Top level storage changes.
top: OverlayedChangeSet,
pub top: OverlayedChangeSet,
/// Child storage changes. The map key is the child storage key without the common prefix.
children: Map<StorageKey, (OverlayedChangeSet, ChildInfo)>,
/// Offchain related changes.
Expand Down Expand Up @@ -813,7 +813,9 @@ where
/// or an owned extension.
#[cfg(feature = "std")]
pub enum OverlayedExtension<'a> {
#[allow(missing_docs)]
MutRef(&'a mut Box<dyn Extension>),
#[allow(missing_docs)]
Owned(Box<dyn Extension>),
}

Expand Down
89 changes: 59 additions & 30 deletions substrate/utils/frame/benchmarking-cli/src/pallet/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ use sp_core::{
use sp_externalities::Extensions;
use sp_genesis_builder::{PresetId, Result as GenesisBuildResult};
use sp_keystore::{testing::MemoryKeystore, KeystoreExt};
use sp_runtime::traits::Hash;
use sp_runtime::{traits::Hash, RuntimeString};
use sp_state_machine::{OverlayedChanges, StateMachine};
use sp_trie::{proof_size_extension::ProofSizeExt, recorder::Recorder};
use sp_wasm_interface::HostFunctions;
Expand Down Expand Up @@ -162,9 +162,6 @@ generate the genesis state is deprecated. Please remove the `--chain`/`--dev`/`-
point `--runtime` to your runtime blob and set `--genesis-builder=runtime`. This warning may \
become a hard error any time after December 2024.";

/// The preset that we expect to find in the GenesisBuilder runtime API.
const GENESIS_PRESET: &str = "development";

impl PalletCmd {
/// Runs the command and benchmarks a pallet.
#[deprecated(
Expand Down Expand Up @@ -214,9 +211,7 @@ impl PalletCmd {
return self.output_from_results(&batches)
}

let (genesis_storage, genesis_changes) =
self.genesis_storage::<Hasher, ExtraHostFunctions>(&chain_spec)?;
let mut changes = genesis_changes.clone();
let genesis_storage = self.genesis_storage::<Hasher, ExtraHostFunctions>(&chain_spec)?;

let cache_size = Some(self.database_cache_size as usize);
let state_with_tracking = BenchmarkingState::<Hasher>::new(
Expand Down Expand Up @@ -262,7 +257,7 @@ impl PalletCmd {
Self::exec_state_machine(
StateMachine::new(
state,
&mut changes,
&mut Default::default(),
&executor,
"Benchmark_benchmark_metadata",
&(self.extra).encode(),
Expand Down Expand Up @@ -347,7 +342,6 @@ impl PalletCmd {
for (s, selected_components) in all_components.iter().enumerate() {
// First we run a verification
if !self.no_verify {
let mut changes = genesis_changes.clone();
let state = &state_without_tracking;
// Don't use these results since verification code will add overhead.
let _batch: Vec<BenchmarkBatch> = match Self::exec_state_machine::<
Expand All @@ -357,7 +351,7 @@ impl PalletCmd {
>(
StateMachine::new(
state,
&mut changes,
&mut Default::default(),
&executor,
"Benchmark_dispatch_benchmark",
&(
Expand Down Expand Up @@ -389,7 +383,6 @@ impl PalletCmd {
}
// Do one loop of DB tracking.
{
let mut changes = genesis_changes.clone();
let state = &state_with_tracking;
let batch: Vec<BenchmarkBatch> = match Self::exec_state_machine::<
std::result::Result<Vec<BenchmarkBatch>, String>,
Expand All @@ -398,7 +391,7 @@ impl PalletCmd {
>(
StateMachine::new(
state, // todo remove tracking
&mut changes,
&mut Default::default(),
&executor,
"Benchmark_dispatch_benchmark",
&(
Expand Down Expand Up @@ -432,7 +425,6 @@ impl PalletCmd {
}
// Finally run a bunch of loops to get extrinsic timing information.
for r in 0..self.external_repeat {
let mut changes = genesis_changes.clone();
let state = &state_without_tracking;
let batch = match Self::exec_state_machine::<
std::result::Result<Vec<BenchmarkBatch>, String>,
Expand All @@ -441,7 +433,7 @@ impl PalletCmd {
>(
StateMachine::new(
state, // todo remove tracking
&mut changes,
&mut Default::default(),
&executor,
"Benchmark_dispatch_benchmark",
&(
Expand Down Expand Up @@ -567,16 +559,13 @@ impl PalletCmd {
Ok(benchmarks_to_run)
}

/// Produce a genesis storage and genesis changes.
/// Build the genesis storage by either the Genesis Builder API, chain spec or nothing.
///
/// It would be easier to only return one type, but there is no easy way to convert them.
// TODO: Re-write `BenchmarkingState` to not be such a clusterfuck and only accept
// `OverlayedChanges` instead of a mix between `OverlayedChanges` and `State`. But this can only
// be done once we deprecated and removed the legacy interface :(
/// Behaviour can be controlled by the `--genesis-builder` flag.
fn genesis_storage<H: Hash, F: HostFunctions>(
&self,
chain_spec: &Option<Box<dyn ChainSpec>>,
) -> Result<(sp_storage::Storage, OverlayedChanges<H>)> {
) -> Result<sp_storage::Storage> {
Ok(match (self.genesis_builder, self.runtime.is_some()) {
(Some(GenesisBuilder::None), _) => Default::default(),
(Some(GenesisBuilder::Spec), _) | (None, false) => {
Expand All @@ -589,13 +578,34 @@ impl PalletCmd {
.build_storage()
.map_err(|e| format!("{ERROR_CANNOT_BUILD_GENESIS}\nError: {e}"))?;

(storage, Default::default())
storage
},
(Some(GenesisBuilder::Runtime), _) | (None, true) =>
(Default::default(), self.genesis_from_runtime::<H, F>()?),
self.genesis_from_runtime::<H, F>().and_then(Self::changes_to_storage)?,
})
}

/// Convert some overlayed changes to a storage.
fn changes_to_storage<H: Hash>(
mut changes: OverlayedChanges<H>,
) -> Result<sp_storage::Storage> {
let mut top = BTreeMap::<Vec<u8>, Vec<u8>>::new();
// The backend state here does not matter:
let state = BenchmarkingState::<H>::new(Default::default(), None, false, false)?;

let changes = changes.drain_storage_changes(&state, sp_storage::StateVersion::V1)?;

for (k, v) in changes.main_storage_changes {
if let Some(v) = v {
top.insert(k, v);
} else {
top.remove(&k);
}
}

Ok(sp_storage::Storage { top, children_default: Default::default() })
}

/// Generate the genesis changeset by the runtime API.
fn genesis_from_runtime<H: Hash, F: HostFunctions>(&self) -> Result<OverlayedChanges<H>> {
let state = BenchmarkingState::<H>::new(
Expand Down Expand Up @@ -643,13 +653,14 @@ impl PalletCmd {
let base_genesis_json = serde_json::from_slice::<serde_json::Value>(&base_genesis_json)
.map_err(|e| format!("GenesisBuilder::get_preset returned invalid JSON: {:?}", e))?;

let preset_name = RuntimeString::Owned(self.genesis_builder_preset.clone());
let dev_genesis_json: Option<Vec<u8>> = Self::exec_state_machine(
StateMachine::new(
&state,
&mut Default::default(),
&executor,
"GenesisBuilder_get_preset",
&Some::<PresetId>(GENESIS_PRESET.into()).encode(), // Use the default preset
&Some::<PresetId>(preset_name).encode(),
&mut Self::build_extensions(executor.clone(), state.recorder()),
&runtime_code,
CallContext::Offchain,
Expand All @@ -666,9 +677,10 @@ impl PalletCmd {
})?;
json_merge(&mut genesis_json, dev);
} else {
log::warn!(
"Could not find genesis preset '{GENESIS_PRESET}'. Falling back to default."
);
return Err(format!(
"GenesisBuilder::get_preset returned no data for preset `{}`. Manually specify `--genesis-builder-preset` or use a different `--genesis-builder`.",
self.genesis_builder_preset).into()
)
}

let json_pretty_str = serde_json::to_string_pretty(&genesis_json)
Expand Down Expand Up @@ -738,8 +750,17 @@ impl PalletCmd {
state: &'a BenchmarkingState<H>,
) -> Result<FetchedCode<'a, BenchmarkingState<H>, H>> {
if let Some(runtime) = &self.runtime {
let runtime = std::path::absolute(runtime)
.map_err(|e| format!("Could not get absolute path for runtime file: {e}"))?;
log::info!("Loading WASM from {}", runtime.display());
let code = fs::read(runtime)?;

let code = fs::read(&runtime).map_err(|e| {
format!(
"Could not load runtime file from path: {}, error: {}",
runtime.display(),
e
)
})?;
let hash = sp_core::blake2_256(&code).to_vec();
let wrapped_code = WrappedRuntimeCode(Cow::Owned(code));

Expand Down Expand Up @@ -990,19 +1011,27 @@ impl PalletCmd {

if let Some(output_path) = &self.output {
if !output_path.is_dir() && output_path.file_name().is_none() {
return Err("Output file or path is invalid!".into())
return Err(format!(
"Output path is neither a directory nor a file: {:?}",
output_path
)
.into())
}
}

if let Some(header_file) = &self.header {
if !header_file.is_file() {
return Err("Header file is invalid!".into())
return Err(format!("Header file could not be found: {:?}", &header_file).into())
};
}

if let Some(handlebars_template_file) = &self.template {
if !handlebars_template_file.is_file() {
return Err("Handlebars template file is invalid!".into())
return Err(format!(
"Handlebars template file could not be found: {:?}",
&handlebars_template_file
)
.into())
};
}

Expand Down
7 changes: 7 additions & 0 deletions substrate/utils/frame/benchmarking-cli/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,13 @@ pub struct PalletCmd {
#[arg(long, value_enum)]
pub genesis_builder: Option<GenesisBuilder>,

/// The preset that we expect to find in the GenesisBuilder runtime API.
///
/// This can be useful when a runtime has a dedicated benchmarking preset instead of using the
/// default one.
#[arg(long, default_value = "development")]
pub genesis_builder_preset: String,

/// DEPRECATED: This argument has no effect.
#[arg(long = "execution")]
pub execution: Option<String>,
Expand Down
7 changes: 6 additions & 1 deletion substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,12 @@ pub(crate) fn write_results(
benchmarks: results.clone(),
};

let mut output_file = fs::File::create(&file_path)?;
let file_path = std::path::absolute(&file_path).map_err(|e| {
format!("Could not get absolute path for: {:?}. Error: {:?}", &file_path, e)
})?;
let mut output_file = fs::File::create(&file_path).map_err(|e| {
format!("Could not write weight file to: {:?}. Error: {:?}", &file_path, e)
})?;
handlebars
.render_template_to_write(&template, &hbs_data, &mut output_file)
.map_err(|e| io_error(&e.to_string()))?;
Expand Down

0 comments on commit b74e6ca

Please sign in to comment.