diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index e7842e5c4ba3b..17c02eca17b4a 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -1134,13 +1134,7 @@ impl_runtime_apis! { #[cfg(feature = "runtime-benchmarks")] impl frame_benchmarking::Benchmark for Runtime { fn dispatch_benchmark( - pallet: Vec, - benchmark: Vec, - lowest_range_values: Vec, - highest_range_values: Vec, - steps: Vec, - repeat: u32, - extra: bool, + config: frame_benchmarking::BenchmarkConfig ) -> Result, sp_runtime::RuntimeString> { use frame_benchmarking::{Benchmarking, BenchmarkBatch, add_benchmark, TrackedStorageKey}; // Trying to add benchmarks directly to the Session Pallet caused cyclic dependency issues. @@ -1170,7 +1164,7 @@ impl_runtime_apis! { ]; let mut batches = Vec::::new(); - let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist, extra); + let params = (&config, &whitelist); add_benchmark!(params, batches, pallet_babe, Babe); add_benchmark!(params, batches, pallet_balances, Balances); diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index cebdcbcfecd25..03d60dbec5851 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -648,30 +648,11 @@ macro_rules! benchmark_backend { ] } - fn instance(&self, components: &[($crate::BenchmarkParameter, u32)]) - -> Result Result<(), &'static str>>, &'static str> - { - $( - let $common = $common_from; - )* - $( - // Prepare instance - let $param = components.iter() - .find(|&c| c.0 == $crate::BenchmarkParameter::$param) - .unwrap().1; - )* - $( - let $pre_id : $pre_ty = $pre_ex; - )* - $( $param_instancer ; )* - $( $post )* - - Ok(Box::new(move || -> Result<(), &'static str> { $eval; Ok(()) })) - } - - fn verify(&self, components: &[($crate::BenchmarkParameter, u32)]) - -> Result Result<(), &'static str>>, &'static str> - { + fn instance( + &self, + components: &[($crate::BenchmarkParameter, u32)], + verify: bool + ) -> Result Result<(), &'static str>>, &'static str> { $( let $common = $common_from; )* @@ -687,7 +668,13 @@ macro_rules! benchmark_backend { $( $param_instancer ; )* $( $post )* - Ok(Box::new(move || -> Result<(), &'static str> { $eval; $postcode; Ok(()) })) + Ok(Box::new(move || -> Result<(), &'static str> { + $eval; + if verify { + $postcode; + } + Ok(()) + })) } } }; @@ -736,26 +723,16 @@ macro_rules! selected_benchmark { } } - fn instance(&self, components: &[($crate::BenchmarkParameter, u32)]) - -> Result Result<(), &'static str>>, &'static str> - { - match self { - $( - Self::$bench => < - $bench as $crate::BenchmarkingSetup - >::instance(&$bench, components), - )* - } - } - - fn verify(&self, components: &[($crate::BenchmarkParameter, u32)]) - -> Result Result<(), &'static str>>, &'static str> - { + fn instance( + &self, + components: &[($crate::BenchmarkParameter, u32)], + verify: bool + ) -> Result Result<(), &'static str>>, &'static str> { match self { $( Self::$bench => < $bench as $crate::BenchmarkingSetup - >::verify(&$bench, components), + >::instance(&$bench, components, verify), )* } } @@ -791,7 +768,8 @@ macro_rules! impl_benchmark { highest_range_values: &[u32], steps: &[u32], repeat: u32, - whitelist: &[$crate::TrackedStorageKey] + whitelist: &[$crate::TrackedStorageKey], + verify: bool, ) -> Result, &'static str> { // Map the input to the selected benchmark. let extrinsic = sp_std::str::from_utf8(extrinsic) @@ -826,6 +804,7 @@ macro_rules! impl_benchmark { repeat: u32, c: Vec<($crate::BenchmarkParameter, u32)>, results: &mut Vec<$crate::BenchmarkResults>, + verify: bool, | -> Result<(), &'static str> { // Run the benchmark `repeat` times. for _ in 0..repeat { @@ -833,7 +812,7 @@ macro_rules! impl_benchmark { // benchmark. let closure_to_benchmark = < SelectedBenchmark as $crate::BenchmarkingSetup - >::instance(&selected_benchmark, &c)?; + >::instance(&selected_benchmark, &c, verify)?; // Set the block number to at least 1 so events are deposited. if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { @@ -847,43 +826,49 @@ macro_rules! impl_benchmark { // Reset the read/write counter so we don't count operations in the setup process. $crate::benchmarking::reset_read_write_count(); - // Time the extrinsic logic. - frame_support::debug::trace!( - target: "benchmark", - "Start Benchmark: {:?}", c - ); + if verify { + closure_to_benchmark()?; + } else { + // Time the extrinsic logic. + frame_support::debug::trace!( + target: "benchmark", + "Start Benchmark: {:?}", c + ); - let start_extrinsic = $crate::benchmarking::current_time(); - closure_to_benchmark()?; - let finish_extrinsic = $crate::benchmarking::current_time(); - let elapsed_extrinsic = finish_extrinsic - start_extrinsic; - // Commit the changes to get proper write count - $crate::benchmarking::commit_db(); - frame_support::debug::trace!( - target: "benchmark", - "End Benchmark: {} ns", elapsed_extrinsic - ); - let read_write_count = $crate::benchmarking::read_write_count(); - frame_support::debug::trace!( - target: "benchmark", - "Read/Write Count {:?}", read_write_count - ); + let start_extrinsic = $crate::benchmarking::current_time(); - // Time the storage root recalculation. - let start_storage_root = $crate::benchmarking::current_time(); - $crate::storage_root(); - let finish_storage_root = $crate::benchmarking::current_time(); - let elapsed_storage_root = finish_storage_root - start_storage_root; + closure_to_benchmark()?; - results.push($crate::BenchmarkResults { - components: c.clone(), - extrinsic_time: elapsed_extrinsic, - storage_root_time: elapsed_storage_root, - reads: read_write_count.0, - repeat_reads: read_write_count.1, - writes: read_write_count.2, - repeat_writes: read_write_count.3, - }); + let finish_extrinsic = $crate::benchmarking::current_time(); + let elapsed_extrinsic = finish_extrinsic - start_extrinsic; + // Commit the changes to get proper write count + $crate::benchmarking::commit_db(); + frame_support::debug::trace!( + target: "benchmark", + "End Benchmark: {} ns", elapsed_extrinsic + ); + let read_write_count = $crate::benchmarking::read_write_count(); + frame_support::debug::trace!( + target: "benchmark", + "Read/Write Count {:?}", read_write_count + ); + + // Time the storage root recalculation. + let start_storage_root = $crate::benchmarking::current_time(); + $crate::storage_root(); + let finish_storage_root = $crate::benchmarking::current_time(); + let elapsed_storage_root = finish_storage_root - start_storage_root; + + results.push($crate::BenchmarkResults { + components: c.clone(), + extrinsic_time: elapsed_extrinsic, + storage_root_time: elapsed_storage_root, + reads: read_write_count.0, + repeat_reads: read_write_count.1, + writes: read_write_count.2, + repeat_writes: read_write_count.3, + }); + } // Wipe the DB back to the genesis state. $crate::benchmarking::wipe_db(); @@ -893,7 +878,11 @@ macro_rules! impl_benchmark { }; if components.is_empty() { - repeat_benchmark(repeat, Default::default(), &mut results)?; + if verify { + // If `--verify` is used, run the benchmark once to verify it would complete. + repeat_benchmark(1, Default::default(), &mut Vec::new(), true)?; + } + repeat_benchmark(repeat, Default::default(), &mut results, false)?; } else { // Select the component we will be benchmarking. Each component will be benchmarked. for (idx, (name, low, high)) in components.iter().enumerate() { @@ -929,7 +918,11 @@ macro_rules! impl_benchmark { ) .collect(); - repeat_benchmark(repeat, c, &mut results)?; + if verify { + // If `--verify` is used, run the benchmark once to verify it would complete. + repeat_benchmark(1, Default::default(), &mut Vec::new(), true)?; + } + repeat_benchmark(repeat, c, &mut results, false)?; } } } @@ -962,17 +955,17 @@ macro_rules! impl_benchmark_test { let execute_benchmark = | c: Vec<($crate::BenchmarkParameter, u32)> | -> Result<(), &'static str> { - // Set up the verification state + // Set up the benchmark, return execution + verification function. let closure_to_verify = < SelectedBenchmark as $crate::BenchmarkingSetup - >::verify(&selected_benchmark, &c)?; + >::instance(&selected_benchmark, &c, true)?; // Set the block number to at least 1 so events are deposited. if $crate::Zero::is_zero(&frame_system::Module::::block_number()) { frame_system::Module::::set_block_number(1.into()); } - // Run verification + // Run execution + verification closure_to_verify()?; // Reset the state @@ -1015,7 +1008,7 @@ macro_rules! impl_benchmark_test { /// First create an object that holds in the input parameters for the benchmark: /// /// ```ignore -/// let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist); +/// let params = (&config, &whitelist); /// ``` /// /// The `whitelist` is a parameter you pass to control the DB read/write tracking. @@ -1059,18 +1052,29 @@ macro_rules! impl_benchmark_test { macro_rules! add_benchmark { ( $params:ident, $batches:ident, $name:ident, $( $location:tt )* ) => ( let name_string = stringify!($name).as_bytes(); - let (pallet, benchmark, lowest_range_values, highest_range_values, steps, repeat, whitelist, extra) = $params; + let (config, whitelist) = $params; + let $crate::BenchmarkConfig { + pallet, + benchmark, + lowest_range_values, + highest_range_values, + steps, + repeat, + verify, + extra, + } = config; if &pallet[..] == &name_string[..] || &pallet[..] == &b"*"[..] { if &pallet[..] == &b"*"[..] || &benchmark[..] == &b"*"[..] { - for benchmark in $( $location )*::benchmarks(extra).into_iter() { + for benchmark in $( $location )*::benchmarks(*extra).into_iter() { $batches.push($crate::BenchmarkBatch { results: $( $location )*::run_benchmark( benchmark, &lowest_range_values[..], &highest_range_values[..], &steps[..], - repeat, + *repeat, whitelist, + *verify, )?, pallet: name_string.to_vec(), benchmark: benchmark.to_vec(), @@ -1083,8 +1087,9 @@ macro_rules! add_benchmark { &lowest_range_values[..], &highest_range_values[..], &steps[..], - repeat, + *repeat, whitelist, + *verify, )?, pallet: name_string.to_vec(), benchmark: benchmark.clone(), diff --git a/frame/benchmarking/src/tests.rs b/frame/benchmarking/src/tests.rs index 127645d430564..94f3574100739 100644 --- a/frame/benchmarking/src/tests.rs +++ b/frame/benchmarking/src/tests.rs @@ -176,10 +176,11 @@ fn benchmarks_macro_works() { let closure = >::instance( &selected, &[(BenchmarkParameter::b, 1)], + true, ).expect("failed to create closure"); new_test_ext().execute_with(|| { - assert_eq!(closure(), Ok(())); + assert_ok!(closure()); }); } @@ -193,6 +194,7 @@ fn benchmarks_macro_rename_works() { let closure = >::instance( &selected, &[(BenchmarkParameter::b, 1)], + true, ).expect("failed to create closure"); new_test_ext().execute_with(|| { @@ -210,9 +212,10 @@ fn benchmarks_macro_works_for_non_dispatchable() { let closure = >::instance( &selected, &[(BenchmarkParameter::x, 1)], + true, ).expect("failed to create closure"); - assert_eq!(closure(), Ok(())); + assert_ok!(closure()); } #[test] @@ -220,14 +223,28 @@ fn benchmarks_macro_verify_works() { // Check postcondition for benchmark `set_value` is valid. let selected = SelectedBenchmark::set_value; - let closure = >::verify( + let closure = >::instance( &selected, &[(BenchmarkParameter::b, 1)], + true, ).expect("failed to create closure"); new_test_ext().execute_with(|| { assert_ok!(closure()); }); + + // Check postcondition for benchmark `bad_verify` is invalid. + let selected = SelectedBenchmark::bad_verify; + + let closure = >::instance( + &selected, + &[(BenchmarkParameter::x, 10000)], + true, + ).expect("failed to create closure"); + + new_test_ext().execute_with(|| { + assert_err!(closure(), "You forgot to sort!"); + }); } #[test] diff --git a/frame/benchmarking/src/utils.rs b/frame/benchmarking/src/utils.rs index 8c25f035802fc..347334e24d5f6 100644 --- a/frame/benchmarking/src/utils.rs +++ b/frame/benchmarking/src/utils.rs @@ -63,19 +63,32 @@ pub struct BenchmarkResults { pub repeat_writes: u32, } +/// Configuration used to setup and run runtime benchmarks. +#[derive(Encode, Decode, Default, Clone, PartialEq, Debug)] +pub struct BenchmarkConfig { + /// The encoded name of the pallet to benchmark. + pub pallet: Vec, + /// The encoded name of the benchmark/extrinsic to run. + pub benchmark: Vec, + /// An optional manual override to the lowest values used in the `steps` range. + pub lowest_range_values: Vec, + /// An optional manual override to the highest values used in the `steps` range. + pub highest_range_values: Vec, + /// The number of samples to take across the range of values for components. + pub steps: Vec, + /// The number of times to repeat a benchmark. + pub repeat: u32, + /// Enable an extra benchmark iteration which runs the verification logic for a benchmark. + pub verify: bool, + /// Enable benchmarking of "extra" extrinsics, i.e. those that are not directly used in a pallet. + pub extra: bool, +} + sp_api::decl_runtime_apis! { /// Runtime api for benchmarking a FRAME runtime. pub trait Benchmark { /// Dispatch the given benchmark. - fn dispatch_benchmark( - pallet: Vec, - benchmark: Vec, - lowest_range_values: Vec, - highest_range_values: Vec, - steps: Vec, - repeat: u32, - extra: bool, - ) -> Result, RuntimeString>; + fn dispatch_benchmark(config: BenchmarkConfig) -> Result, RuntimeString>; } } @@ -175,7 +188,8 @@ pub trait Benchmarking { highest_range_values: &[u32], steps: &[u32], repeat: u32, - whitelist: &[TrackedStorageKey] + whitelist: &[TrackedStorageKey], + verify: bool, ) -> Result, &'static str>; } @@ -188,10 +202,11 @@ pub trait BenchmarkingSetup { fn components(&self) -> Vec<(BenchmarkParameter, u32, u32)>; /// Set up the storage, and prepare a closure to run the benchmark. - fn instance(&self, components: &[(BenchmarkParameter, u32)]) -> Result Result<(), &'static str>>, &'static str>; - - /// Set up the storage, and prepare a closure to test and verify the benchmark - fn verify(&self, components: &[(BenchmarkParameter, u32)]) -> Result Result<(), &'static str>>, &'static str>; + fn instance( + &self, + components: &[(BenchmarkParameter, u32)], + verify: bool + ) -> Result Result<(), &'static str>>, &'static str>; } /// Grab an account, seeded by a name and index. diff --git a/frame/staking/src/benchmarking.rs b/frame/staking/src/benchmarking.rs index 77eecb2ef04d5..156b2f81c8429 100644 --- a/frame/staking/src/benchmarking.rs +++ b/frame/staking/src/benchmarking.rs @@ -715,7 +715,8 @@ mod tests { let closure_to_benchmark = >::instance( &selected_benchmark, - &c + &c, + true ).unwrap(); assert_ok!(closure_to_benchmark()); diff --git a/utils/frame/benchmarking-cli/src/command.rs b/utils/frame/benchmarking-cli/src/command.rs index 688e393bd605a..b0791f88ce5f6 100644 --- a/utils/frame/benchmarking-cli/src/command.rs +++ b/utils/frame/benchmarking-cli/src/command.rs @@ -75,6 +75,7 @@ impl BenchmarkCmd { self.highest_range_values.clone(), self.steps.clone(), self.repeat, + !self.no_verify, self.extra, ).encode(), extensions, diff --git a/utils/frame/benchmarking-cli/src/lib.rs b/utils/frame/benchmarking-cli/src/lib.rs index c2a228fc86aef..8cbb3c786876c 100644 --- a/utils/frame/benchmarking-cli/src/lib.rs +++ b/utils/frame/benchmarking-cli/src/lib.rs @@ -72,6 +72,10 @@ pub struct BenchmarkCmd { #[structopt(long)] pub heap_pages: Option, + /// Disable verification logic when running benchmarks. + #[structopt(long)] + pub no_verify: bool, + /// Display and run extra benchmarks that would otherwise not be needed for weight construction. #[structopt(long)] pub extra: bool,