From 643818d65df12fee7401e0337e9b24af89cfbfca Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 19 Aug 2021 14:34:56 +0200 Subject: [PATCH] Custom Benchmark Errors and Override (#9517) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * initial idea * update benchmark test to frame v2 * fix some errors * fixes for elec phrag * fix tests * update extrinsic time and docs * fix import * undo extra changes * helper function * wrong way * Update frame/benchmarking/src/utils.rs Co-authored-by: Bastian Köcher * doesnt need encode/decode * fix benchmark return Co-authored-by: Bastian Köcher --- frame/benchmarking/src/analysis.rs | 14 +- frame/benchmarking/src/lib.rs | 80 +++++++----- frame/benchmarking/src/tests.rs | 120 ++++++++++-------- frame/benchmarking/src/utils.rs | 66 ++++++++-- frame/collective/src/benchmarking.rs | 2 +- frame/democracy/src/benchmarking.rs | 50 ++++---- .../src/benchmarking.rs | 2 +- frame/elections-phragmen/src/benchmarking.rs | 13 +- frame/elections-phragmen/src/lib.rs | 2 +- frame/elections-phragmen/src/weights.rs | 9 ++ frame/im-online/src/benchmarking.rs | 6 +- utils/frame/benchmarking-cli/src/command.rs | 4 +- utils/frame/benchmarking-cli/src/writer.rs | 18 ++- 13 files changed, 252 insertions(+), 134 deletions(-) diff --git a/frame/benchmarking/src/analysis.rs b/frame/benchmarking/src/analysis.rs index 4944de8553f86..2bb20ebe2e7f8 100644 --- a/frame/benchmarking/src/analysis.rs +++ b/frame/benchmarking/src/analysis.rs @@ -17,7 +17,7 @@ //! Tools for analyzing the benchmark results. -use crate::BenchmarkResults; +use crate::BenchmarkResult; use core::convert::TryFrom; use linregress::{FormulaRegressionBuilder, RegressionDataBuilder}; use std::collections::BTreeMap; @@ -76,7 +76,7 @@ impl TryFrom> for AnalysisChoice { impl Analysis { // Useful for when there are no components, and we just need an median value of the benchmark // results. Note: We choose the median value because it is more robust to outliers. - fn median_value(r: &Vec, selector: BenchmarkSelector) -> Option { + fn median_value(r: &Vec, selector: BenchmarkSelector) -> Option { if r.is_empty() { return None } @@ -104,7 +104,7 @@ impl Analysis { }) } - pub fn median_slopes(r: &Vec, selector: BenchmarkSelector) -> Option { + pub fn median_slopes(r: &Vec, selector: BenchmarkSelector) -> Option { if r[0].components.is_empty() { return Self::median_value(r, selector) } @@ -199,7 +199,7 @@ impl Analysis { }) } - pub fn min_squares_iqr(r: &Vec, selector: BenchmarkSelector) -> Option { + pub fn min_squares_iqr(r: &Vec, selector: BenchmarkSelector) -> Option { if r[0].components.is_empty() { return Self::median_value(r, selector) } @@ -279,7 +279,7 @@ impl Analysis { }) } - pub fn max(r: &Vec, selector: BenchmarkSelector) -> Option { + pub fn max(r: &Vec, selector: BenchmarkSelector) -> Option { let median_slopes = Self::median_slopes(r, selector); let min_squares = Self::min_squares_iqr(r, selector); @@ -402,8 +402,8 @@ mod tests { storage_root_time: u128, reads: u32, writes: u32, - ) -> BenchmarkResults { - BenchmarkResults { + ) -> BenchmarkResult { + BenchmarkResult { components, extrinsic_time, storage_root_time, diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 673f6dee6fc7e..fb602f0732b7d 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -644,7 +644,7 @@ macro_rules! benchmark_backend { &self, components: &[($crate::BenchmarkParameter, u32)], verify: bool - ) -> Result<$crate::Box Result<(), &'static str>>, &'static str> { + ) -> Result<$crate::Box Result<(), $crate::BenchmarkError>>, &'static str> { $( // Prepare instance let $param = components.iter() @@ -658,7 +658,7 @@ macro_rules! benchmark_backend { $( $param_instancer ; )* $( $post )* - Ok($crate::Box::new(move || -> Result<(), &'static str> { + Ok($crate::Box::new(move || -> Result<(), $crate::BenchmarkError> { $eval; if verify { $postcode; @@ -717,7 +717,7 @@ macro_rules! selected_benchmark { &self, components: &[($crate::BenchmarkParameter, u32)], verify: bool - ) -> Result<$crate::Box Result<(), &'static str>>, &'static str> { + ) -> Result<$crate::Box Result<(), $crate::BenchmarkError>>, &'static str> { match self { $( Self::$bench => < @@ -741,7 +741,7 @@ macro_rules! impl_benchmark { ( $( $name_skip_meta:ident ),* ) ) => { impl, $instance: $instance_bound )? > - $crate::Benchmarking<$crate::BenchmarkResults> for Pallet + $crate::Benchmarking for Pallet where T: frame_system::Config, $( $where_clause )* { fn benchmarks(extra: bool) -> $crate::Vec<$crate::BenchmarkMetadata> { @@ -772,13 +772,13 @@ macro_rules! impl_benchmark { whitelist: &[$crate::TrackedStorageKey], verify: bool, internal_repeats: u32, - ) -> Result<$crate::Vec<$crate::BenchmarkResults>, &'static str> { + ) -> Result<$crate::Vec<$crate::BenchmarkResult>, $crate::BenchmarkError> { // Map the input to the selected benchmark. let extrinsic = $crate::sp_std::str::from_utf8(extrinsic) .map_err(|_| "`extrinsic` is not a valid utf8 string!")?; let selected_benchmark = match extrinsic { $( stringify!($name) => SelectedBenchmark::$name, )* - _ => return Err("Could not find extrinsic."), + _ => return Err("Could not find extrinsic.".into()), }; // Add whitelist to DB including whitelisted caller @@ -790,7 +790,7 @@ macro_rules! impl_benchmark { whitelist.push(whitelisted_caller_key.into()); $crate::benchmarking::set_whitelist(whitelist); - let mut results: $crate::Vec<$crate::BenchmarkResults> = $crate::Vec::new(); + let mut results: $crate::Vec<$crate::BenchmarkResult> = $crate::Vec::new(); // Always do at least one internal repeat... for _ in 0 .. internal_repeats.max(1) { @@ -852,13 +852,13 @@ macro_rules! impl_benchmark { let elapsed_storage_root = finish_storage_root - start_storage_root; let skip_meta = [ $( stringify!($name_skip_meta).as_ref() ),* ]; - let read_and_written_keys = if (&skip_meta).contains(&extrinsic) { + let read_and_written_keys = if skip_meta.contains(&extrinsic) { $crate::vec![(b"Skipped Metadata".to_vec(), 0, 0, false)] } else { $crate::benchmarking::get_read_and_written_keys() }; - results.push($crate::BenchmarkResults { + results.push($crate::BenchmarkResult { components: c.to_vec(), extrinsic_time: elapsed_extrinsic, storage_root_time: elapsed_storage_root, @@ -893,14 +893,14 @@ macro_rules! impl_benchmark { /// by the `impl_benchmark_test_suite` macro. However, it is not an error if a pallet /// author chooses not to implement benchmarks. #[allow(unused)] - fn test_bench_by_name(name: &[u8]) -> Result<(), &'static str> { + fn test_bench_by_name(name: &[u8]) -> Result<(), $crate::BenchmarkError> { let name = $crate::sp_std::str::from_utf8(name) - .map_err(|_| "`name` is not a valid utf8 string!")?; + .map_err(|_| -> $crate::BenchmarkError { "`name` is not a valid utf8 string!".into() })?; match name { $( stringify!($name) => { $crate::paste::paste! { Self::[< test_benchmark_ $name >]() } } )* - _ => Err("Could not find test for requested benchmark."), + _ => Err("Could not find test for requested benchmark.".into()), } } } @@ -925,7 +925,7 @@ macro_rules! impl_benchmark_test { where T: frame_system::Config, $( $where_clause )* { #[allow(unused)] - fn [] () -> Result<(), &'static str> { + fn [] () -> Result<(), $crate::BenchmarkError> { let selected_benchmark = SelectedBenchmark::$name; let components = < SelectedBenchmark as $crate::BenchmarkingSetup @@ -933,7 +933,7 @@ macro_rules! impl_benchmark_test { let execute_benchmark = | c: $crate::Vec<($crate::BenchmarkParameter, u32)> - | -> Result<(), &'static str> { + | -> Result<(), $crate::BenchmarkError> { // Set up the benchmark, return execution + verification function. let closure_to_verify = < SelectedBenchmark as $crate::BenchmarkingSetup @@ -1213,8 +1213,15 @@ macro_rules! impl_benchmark_test_suite { anything_failed = true; }, Ok(Err(err)) => { - println!("{}: {}", String::from_utf8_lossy(benchmark_name), err); - anything_failed = true; + match err { + $crate::BenchmarkError::Stop(err) => { + println!("{}: {:?}", String::from_utf8_lossy(benchmark_name), err); + anything_failed = true; + }, + $crate::BenchmarkError::Override(_) => { + // This is still considered a success condition. + } + } }, Ok(Ok(_)) => (), } @@ -1328,25 +1335,40 @@ macro_rules! add_benchmark { internal_repeats, } = config; if &pallet[..] == &name_string[..] { - $batches.push($crate::BenchmarkBatch { - pallet: name_string.to_vec(), - instance: instance_string.to_vec(), - benchmark: benchmark.clone(), - results: $( $location )*::run_benchmark( - &benchmark[..], - &selected_components[..], - whitelist, - *verify, - *internal_repeats, - ).map_err(|e| { + let benchmark_result = $( $location )*::run_benchmark( + &benchmark[..], + &selected_components[..], + whitelist, + *verify, + *internal_repeats, + ); + + let final_results = match benchmark_result { + Ok(results) => results, + Err($crate::BenchmarkError::Override(mut result)) => { + // Insert override warning as the first storage key. + result.keys.insert(0, + (b"Benchmark Override".to_vec(), 0, 0, false) + ); + $crate::vec![result] + }, + Err($crate::BenchmarkError::Stop(e)) => { $crate::show_benchmark_debug_info( instance_string, benchmark, selected_components, verify, e, - ) - })? + ); + return Err(e.into()); + }, + }; + + $batches.push($crate::BenchmarkBatch { + pallet: name_string.to_vec(), + instance: instance_string.to_vec(), + benchmark: benchmark.clone(), + results: final_results, }); } ) diff --git a/frame/benchmarking/src/tests.rs b/frame/benchmarking/src/tests.rs index 9cb5043a0dd7d..af9a4e7f4a85b 100644 --- a/frame/benchmarking/src/tests.rs +++ b/frame/benchmarking/src/tests.rs @@ -28,47 +28,44 @@ use sp_runtime::{ }; use sp_std::prelude::*; +#[frame_support::pallet] mod pallet_test { - use frame_support::pallet_prelude::Get; + use frame_support::pallet_prelude::*; + use frame_system::pallet_prelude::*; - frame_support::decl_storage! { - trait Store for Module as Test where - ::OtherEvent: Into<::Event> - { - pub Value get(fn value): Option; - } - } + #[pallet::pallet] + #[pallet::generate_store(pub(super) trait Store)] + pub struct Pallet(_); - frame_support::decl_module! { - pub struct Module for enum Call where - origin: T::Origin, ::OtherEvent: Into<::Event> - { - #[weight = 0] - fn set_value(origin, n: u32) -> frame_support::dispatch::DispatchResult { - let _sender = frame_system::ensure_signed(origin)?; - Value::put(n); - Ok(()) - } + #[pallet::config] + pub trait Config: frame_system::Config { + type LowerBound: Get; + type UpperBound: Get; + } - #[weight = 0] - fn dummy(origin, _n: u32) -> frame_support::dispatch::DispatchResult { - let _sender = frame_system::ensure_none(origin)?; - Ok(()) - } + #[pallet::storage] + #[pallet::getter(fn heartbeat_after)] + pub(crate) type Value = StorageValue<_, u32, OptionQuery>; + + #[pallet::call] + impl Pallet { + #[pallet::weight(0)] + pub fn set_value(origin: OriginFor, n: u32) -> DispatchResult { + let _sender = frame_system::ensure_signed(origin)?; + Value::::put(n); + Ok(()) } - } - pub trait OtherConfig { - type OtherEvent; - } + #[pallet::weight(0)] + pub fn dummy(origin: OriginFor, _n: u32) -> DispatchResult { + let _sender = frame_system::ensure_none(origin)?; + Ok(()) + } - pub trait Config: frame_system::Config + OtherConfig - where - Self::OtherEvent: Into<::Event>, - { - type Event; - type LowerBound: Get; - type UpperBound: Get; + #[pallet::weight(0)] + pub fn always_error(_origin: OriginFor) -> DispatchResult { + return Err("I always fail".into()) + } } } @@ -118,27 +115,18 @@ parameter_types! { } impl pallet_test::Config for Test { - type Event = Event; type LowerBound = LowerBound; type UpperBound = UpperBound; } -impl pallet_test::OtherConfig for Test { - type OtherEvent = Event; -} - fn new_test_ext() -> sp_io::TestExternalities { GenesisConfig::default().build_storage().unwrap().into() } mod benchmarks { - use super::{ - new_test_ext, - pallet_test::{self, Value}, - Test, - }; - use crate::{account, BenchmarkParameter, BenchmarkingSetup}; - use frame_support::{assert_err, assert_ok, ensure, traits::Get, StorageValue}; + use super::{new_test_ext, pallet_test::Value, Test}; + use crate::{account, BenchmarkError, BenchmarkParameter, BenchmarkResult, BenchmarkingSetup}; + use frame_support::{assert_err, assert_ok, ensure, traits::Get}; use frame_system::RawOrigin; use sp_std::prelude::*; @@ -148,8 +136,7 @@ mod benchmarks { crate::benchmarks! { where_clause { where - ::OtherEvent: Into<::Event> + Clone, - ::Event: Clone, + crate::tests::Origin: From::AccountId>>, } set_value { @@ -157,7 +144,7 @@ mod benchmarks { let caller = account::("caller", 0, 0); }: _ (RawOrigin::Signed(caller), b.into()) verify { - assert_eq!(Value::get(), Some(b)); + assert_eq!(Value::::get(), Some(b)); } other_name { @@ -206,7 +193,7 @@ mod benchmarks { let caller = account::("caller", 0, 0); }: set_value(RawOrigin::Signed(caller), b.into()) verify { - assert_eq!(Value::get(), Some(b)); + assert_eq!(Value::::get(), Some(b)); } #[skip_meta] @@ -215,7 +202,21 @@ mod benchmarks { let caller = account::("caller", 0, 0); }: set_value(RawOrigin::Signed(caller), b.into()) verify { - assert_eq!(Value::get(), Some(b)); + assert_eq!(Value::::get(), Some(b)); + } + + override_benchmark { + let b in 1 .. 1000; + let caller = account::("caller", 0, 0); + }: { + Err(BenchmarkError::Override( + BenchmarkResult { + extrinsic_time: 1_234_567_890, + reads: 1337, + writes: 420, + ..Default::default() + } + ))?; } } @@ -306,6 +307,23 @@ mod benchmarks { }); } + #[test] + fn benchmark_override_works() { + let selected = SelectedBenchmark::override_benchmark; + + let closure = >::instance( + &selected, + &[(BenchmarkParameter::b, 1)], + true, + ) + .expect("failed to create closure"); + + new_test_ext().execute_with(|| { + let result = closure(); + assert!(matches!(result, Err(BenchmarkError::Override(_)))); + }); + } + #[test] fn benchmarks_generate_unit_tests() { new_test_ext().execute_with(|| { diff --git a/frame/benchmarking/src/utils.rs b/frame/benchmarking/src/utils.rs index 8be25f7f5e9c5..64eb611a187b0 100644 --- a/frame/benchmarking/src/utils.rs +++ b/frame/benchmarking/src/utils.rs @@ -18,7 +18,11 @@ //! Interfaces, types and utils for benchmarking a FRAME runtime. use codec::{Decode, Encode}; -use frame_support::traits::StorageInfo; +use frame_support::{ + dispatch::{DispatchError, DispatchErrorWithPostInfo}, + pallet_prelude::*, + traits::StorageInfo, +}; use sp_io::hashing::blake2_256; use sp_std::{prelude::Box, vec::Vec}; use sp_storage::TrackedStorageKey; @@ -73,7 +77,7 @@ pub struct BenchmarkBatch { /// The extrinsic (or benchmark name) of this benchmark. pub benchmark: Vec, /// The results from this benchmark. - pub results: Vec, + pub results: Vec, } // TODO: could probably make API cleaner here. @@ -87,16 +91,16 @@ pub struct BenchmarkBatchSplitResults { /// The extrinsic (or benchmark name) of this benchmark. pub benchmark: Vec, /// The extrinsic timing results from this benchmark. - pub time_results: Vec, + pub time_results: Vec, /// The db tracking results from this benchmark. - pub db_results: Vec, + pub db_results: Vec, } -/// Results from running benchmarks on a FRAME pallet. +/// Result from running benchmarks on a FRAME pallet. /// Contains duration of the function call in nanoseconds along with the benchmark parameters /// used for that benchmark result. #[derive(Encode, Decode, Default, Clone, PartialEq, Debug)] -pub struct BenchmarkResults { +pub struct BenchmarkResult { pub components: Vec<(BenchmarkParameter, u32)>, pub extrinsic_time: u128, pub storage_root_time: u128, @@ -108,6 +112,50 @@ pub struct BenchmarkResults { pub keys: Vec<(Vec, u32, u32, bool)>, } +impl BenchmarkResult { + pub fn from_weight(w: Weight) -> Self { + Self { extrinsic_time: (w as u128) / 1_000, ..Default::default() } + } +} + +/// Possible errors returned from the benchmarking pipeline. +/// +/// * Stop: The benchmarking pipeline should stop and return the inner string. +/// * WeightOverride: The benchmarking pipeline is allowed to fail here, and we should use the +/// included weight instead. +#[derive(Clone, PartialEq, Debug)] +pub enum BenchmarkError { + Stop(&'static str), + Override(BenchmarkResult), +} + +impl From for &'static str { + fn from(e: BenchmarkError) -> Self { + match e { + BenchmarkError::Stop(s) => s, + BenchmarkError::Override(_) => "benchmark override", + } + } +} + +impl From<&'static str> for BenchmarkError { + fn from(s: &'static str) -> Self { + Self::Stop(s) + } +} + +impl From for BenchmarkError { + fn from(e: DispatchErrorWithPostInfo) -> Self { + Self::Stop(e.into()) + } +} + +impl From for BenchmarkError { + fn from(e: DispatchError) -> Self { + Self::Stop(e.into()) + } +} + /// Configuration used to setup and run runtime benchmarks. #[derive(Encode, Decode, Default, Clone, PartialEq, Debug)] pub struct BenchmarkConfig { @@ -235,7 +283,7 @@ pub trait Benchmarking { } /// The pallet benchmarking trait. -pub trait Benchmarking { +pub trait Benchmarking { /// Get the benchmarks available for this pallet. Generally there is one benchmark per /// extrinsic, so these are sometimes just called "extrinsics". /// @@ -251,7 +299,7 @@ pub trait Benchmarking { whitelist: &[TrackedStorageKey], verify: bool, internal_repeats: u32, - ) -> Result, &'static str>; + ) -> Result, BenchmarkError>; } /// The required setup for creating a benchmark. @@ -267,7 +315,7 @@ pub trait BenchmarkingSetup { &self, components: &[(BenchmarkParameter, u32)], verify: bool, - ) -> Result Result<(), &'static str>>, &'static str>; + ) -> Result Result<(), BenchmarkError>>, &'static str>; } /// Grab an account, seeded by a name and index. diff --git a/frame/collective/src/benchmarking.rs b/frame/collective/src/benchmarking.rs index ccc20356fbf46..b966279a42ff4 100644 --- a/frame/collective/src/benchmarking.rs +++ b/frame/collective/src/benchmarking.rs @@ -277,7 +277,7 @@ benchmarks_instance! { verify { // All proposals exist and the last proposal has just been updated. assert_eq!(Collective::::proposals().len(), p as usize); - let voting = Collective::::voting(&last_hash).ok_or(Error::::ProposalMissing)?; + let voting = Collective::::voting(&last_hash).ok_or("Proposal Missing")?; assert_eq!(voting.ayes.len(), (m - 3) as usize); assert_eq!(voting.nays.len(), 1); } diff --git a/frame/democracy/src/benchmarking.rs b/frame/democracy/src/benchmarking.rs index ddc3de5906591..487a525719410 100644 --- a/frame/democracy/src/benchmarking.rs +++ b/frame/democracy/src/benchmarking.rs @@ -136,7 +136,7 @@ benchmarks! { } let votes = match VotingOf::::get(&caller) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), r as usize, "Votes were not recorded."); @@ -146,7 +146,7 @@ benchmarks! { verify { let votes = match VotingOf::::get(&caller) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), (r + 1) as usize, "Vote was not recorded."); } @@ -164,7 +164,7 @@ benchmarks! { } let votes = match VotingOf::::get(&caller) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), (r + 1) as usize, "Votes were not recorded."); @@ -179,14 +179,14 @@ benchmarks! { verify { let votes = match VotingOf::::get(&caller) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), (r + 1) as usize, "Vote was incorrectly added"); let referendum_info = Democracy::::referendum_info(referendum_index) .ok_or("referendum doesn't exist")?; let tally = match referendum_info { ReferendumInfo::Ongoing(r) => r.tally, - _ => return Err("referendum not ongoing"), + _ => return Err("referendum not ongoing".into()), }; assert_eq!(tally.nays, 1000u32.into(), "changed vote was not recorded"); } @@ -374,7 +374,7 @@ benchmarks! { if let Some(value) = ReferendumInfoOf::::get(i) { match value { ReferendumInfo::Finished { .. } => (), - ReferendumInfo::Ongoing(_) => return Err("Referendum was not finished"), + ReferendumInfo::Ongoing(_) => return Err("Referendum was not finished".into()), } } } @@ -408,7 +408,7 @@ benchmarks! { if let Some(value) = ReferendumInfoOf::::get(i) { match value { ReferendumInfo::Finished { .. } => (), - ReferendumInfo::Ongoing(_) => return Err("Referendum was not finished"), + ReferendumInfo::Ongoing(_) => return Err("Referendum was not finished".into()), } } } @@ -438,7 +438,7 @@ benchmarks! { for i in 0 .. r { if let Some(value) = ReferendumInfoOf::::get(i) { match value { - ReferendumInfo::Finished { .. } => return Err("Referendum has been finished"), + ReferendumInfo::Finished { .. } => return Err("Referendum has been finished".into()), ReferendumInfo::Ongoing(_) => (), } } @@ -462,7 +462,7 @@ benchmarks! { )?; let (target, balance) = match VotingOf::::get(&caller) { Voting::Delegating { target, balance, .. } => (target, balance), - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(target, old_delegate, "delegation target didn't work"); assert_eq!(balance, delegated_balance, "delegation balance didn't work"); @@ -476,7 +476,7 @@ benchmarks! { } let votes = match VotingOf::::get(&new_delegate) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), r as usize, "Votes were not recorded."); whitelist_account!(caller); @@ -484,13 +484,13 @@ benchmarks! { verify { let (target, balance) = match VotingOf::::get(&caller) { Voting::Delegating { target, balance, .. } => (target, balance), - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(target, new_delegate, "delegation target didn't work"); assert_eq!(balance, delegated_balance, "delegation balance didn't work"); let delegations = match VotingOf::::get(&new_delegate) { Voting::Direct { delegations, .. } => delegations, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(delegations.capital, delegated_balance, "delegation was not recorded."); } @@ -512,7 +512,7 @@ benchmarks! { )?; let (target, balance) = match VotingOf::::get(&caller) { Voting::Delegating { target, balance, .. } => (target, balance), - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(target, the_delegate, "delegation target didn't work"); assert_eq!(balance, delegated_balance, "delegation balance didn't work"); @@ -528,7 +528,7 @@ benchmarks! { } let votes = match VotingOf::::get(&the_delegate) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), r as usize, "Votes were not recorded."); whitelist_account!(caller); @@ -537,7 +537,7 @@ benchmarks! { // Voting should now be direct match VotingOf::::get(&caller) { Voting::Direct { .. } => (), - _ => return Err("undelegation failed"), + _ => return Err("undelegation failed".into()), } } @@ -558,7 +558,7 @@ benchmarks! { let proposal_hash = T::Hashing::hash(&encoded_proposal[..]); match Preimages::::get(proposal_hash) { Some(PreimageStatus::Available { .. }) => (), - _ => return Err("preimage not available") + _ => return Err("preimage not available".into()) } } @@ -580,7 +580,7 @@ benchmarks! { let proposal_hash = T::Hashing::hash(&encoded_proposal[..]); match Preimages::::get(proposal_hash) { Some(PreimageStatus::Available { .. }) => (), - _ => return Err("preimage not available") + _ => return Err("preimage not available".into()) } } @@ -652,7 +652,7 @@ benchmarks! { let votes = match VotingOf::::get(&locker) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), (r + 1) as usize, "Votes were not recorded."); @@ -667,7 +667,7 @@ benchmarks! { verify { let votes = match VotingOf::::get(&locker) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), r as usize, "Vote was not removed"); @@ -689,7 +689,7 @@ benchmarks! { let votes = match VotingOf::::get(&caller) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), r as usize, "Votes not created"); @@ -699,7 +699,7 @@ benchmarks! { verify { let votes = match VotingOf::::get(&caller) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), (r - 1) as usize, "Vote was not removed"); } @@ -718,7 +718,7 @@ benchmarks! { let votes = match VotingOf::::get(&caller) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), r as usize, "Votes not created"); @@ -728,7 +728,7 @@ benchmarks! { verify { let votes = match VotingOf::::get(&caller) { Voting::Direct { votes, .. } => votes, - _ => return Err("Votes are not direct"), + _ => return Err("Votes are not direct".into()), }; assert_eq!(votes.len(), (r - 1) as usize, "Vote was not removed"); } @@ -747,7 +747,7 @@ benchmarks! { match Preimages::::get(proposal_hash) { Some(PreimageStatus::Available { .. }) => (), - _ => return Err("preimage not available") + _ => return Err("preimage not available".into()) } }: enact_proposal(RawOrigin::Root, proposal_hash, 0) verify { @@ -768,7 +768,7 @@ benchmarks! { match Preimages::::get(proposal_hash) { Some(PreimageStatus::Available { .. }) => (), - _ => return Err("preimage not available") + _ => return Err("preimage not available".into()) } }: { assert_eq!( diff --git a/frame/election-provider-multi-phase/src/benchmarking.rs b/frame/election-provider-multi-phase/src/benchmarking.rs index 4aad1d556ce5e..9c734c4823541 100644 --- a/frame/election-provider-multi-phase/src/benchmarking.rs +++ b/frame/election-provider-multi-phase/src/benchmarking.rs @@ -438,7 +438,7 @@ frame_benchmarking::benchmarks! { set_up_data_provider::(v, t); assert!(>::snapshot().is_none()); }: { - >::create_snapshot()? + >::create_snapshot().map_err(|_| "could not create snapshot")?; } verify { assert!(>::snapshot().is_some()); assert_eq!(>::snapshot_metadata().ok_or("snapshot missing")?.voters, v + t); diff --git a/frame/elections-phragmen/src/benchmarking.rs b/frame/elections-phragmen/src/benchmarking.rs index 4e19b64ef7a5f..7cb83b3dd7799 100644 --- a/frame/elections-phragmen/src/benchmarking.rs +++ b/frame/elections-phragmen/src/benchmarking.rs @@ -21,7 +21,9 @@ use super::*; -use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelist}; +use frame_benchmarking::{ + account, benchmarks, impl_benchmark_test_suite, whitelist, BenchmarkError, BenchmarkResult, +}; use frame_support::{dispatch::DispatchResultWithPostInfo, traits::OnInitialize}; use frame_system::RawOrigin; @@ -332,9 +334,16 @@ benchmarks! { } } + // We use the max block weight for this extrinsic for now. See below. + remove_member_without_replacement {}: { + Err(BenchmarkError::Override( + BenchmarkResult::from_weight(T::BlockWeights::get().max_block) + ))?; + } + // -- Root ones #[extra] // this calls into phragmen and consumes a full block for now. - remove_member_without_replacement { + remove_member_without_replacement_extra { // worse case is when we remove a member and we have no runner as a replacement. This // triggers phragmen again. The only parameter is how many candidates will compete for the // new slot. diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 331d34180e98c..8e0a31377f982 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -474,7 +474,7 @@ pub mod pallet { #[pallet::weight(if *has_replacement { T::WeightInfo::remove_member_with_replacement() } else { - T::BlockWeights::get().max_block + T::WeightInfo::remove_member_without_replacement() })] pub fn remove_member( origin: OriginFor, diff --git a/frame/elections-phragmen/src/weights.rs b/frame/elections-phragmen/src/weights.rs index 40d4ead0a4ec2..b60308c4f0a64 100644 --- a/frame/elections-phragmen/src/weights.rs +++ b/frame/elections-phragmen/src/weights.rs @@ -54,6 +54,7 @@ pub trait WeightInfo { fn renounce_candidacy_members() -> Weight; fn renounce_candidacy_runners_up() -> Weight; fn remove_member_with_replacement() -> Weight; + fn remove_member_without_replacement() -> Weight; fn remove_member_wrong_refund() -> Weight; fn clean_defunct_voters(v: u32, d: u32, ) -> Weight; fn election_phragmen(c: u32, v: u32, e: u32, ) -> Weight; @@ -150,6 +151,9 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().reads(5 as Weight)) .saturating_add(T::DbWeight::get().writes(5 as Weight)) } + fn remove_member_without_replacement() -> Weight { + T::BlockWeights::get().max_block + } // Storage: Elections RunnersUp (r:1 w:0) fn remove_member_wrong_refund() -> Weight { (6_697_000 as Weight) @@ -282,6 +286,11 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().reads(5 as Weight)) .saturating_add(RocksDbWeight::get().writes(5 as Weight)) } + fn remove_member_without_replacement() -> Weight { + (76_153_000 as Weight) + .saturating_add(RocksDbWeight::get().reads(5 as Weight)) + .saturating_add(RocksDbWeight::get().writes(5 as Weight)) + } // Storage: Elections RunnersUp (r:1 w:0) fn remove_member_wrong_refund() -> Weight { (6_697_000 as Weight) diff --git a/frame/im-online/src/benchmarking.rs b/frame/im-online/src/benchmarking.rs index 46552cda68c08..4000ce339a169 100644 --- a/frame/im-online/src/benchmarking.rs +++ b/frame/im-online/src/benchmarking.rs @@ -82,7 +82,8 @@ benchmarks! { let (input_heartbeat, signature) = create_heartbeat::(k, e)?; let call = Call::heartbeat(input_heartbeat, signature); }: { - ImOnline::::validate_unsigned(TransactionSource::InBlock, &call)?; + ImOnline::::validate_unsigned(TransactionSource::InBlock, &call) + .map_err(|e| -> &'static str { e.into() })?; } validate_unsigned_and_then_heartbeat { @@ -91,7 +92,8 @@ benchmarks! { let (input_heartbeat, signature) = create_heartbeat::(k, e)?; let call = Call::heartbeat(input_heartbeat, signature); }: { - ImOnline::::validate_unsigned(TransactionSource::InBlock, &call)?; + ImOnline::::validate_unsigned(TransactionSource::InBlock, &call) + .map_err(|e| -> &'static str { e.into() })?; call.dispatch_bypass_filter(RawOrigin::None.into())?; } } diff --git a/utils/frame/benchmarking-cli/src/command.rs b/utils/frame/benchmarking-cli/src/command.rs index 7ba714abe3556..05e380a37ee7f 100644 --- a/utils/frame/benchmarking-cli/src/command.rs +++ b/utils/frame/benchmarking-cli/src/command.rs @@ -19,7 +19,7 @@ use crate::BenchmarkCmd; use codec::{Decode, Encode}; use frame_benchmarking::{ Analysis, BenchmarkBatch, BenchmarkBatchSplitResults, BenchmarkList, BenchmarkParameter, - BenchmarkResults, BenchmarkSelector, + BenchmarkResult, BenchmarkSelector, }; use frame_support::traits::StorageInfo; use linked_hash_map::LinkedHashMap; @@ -48,7 +48,7 @@ fn combine_batches( } let mut all_benchmarks = - LinkedHashMap::<_, (Vec, Vec)>::new(); + LinkedHashMap::<_, (Vec, Vec)>::new(); db_batches .into_iter() diff --git a/utils/frame/benchmarking-cli/src/writer.rs b/utils/frame/benchmarking-cli/src/writer.rs index 8701fb6512622..ae3e2dc0966fe 100644 --- a/utils/frame/benchmarking-cli/src/writer.rs +++ b/utils/frame/benchmarking-cli/src/writer.rs @@ -29,7 +29,7 @@ use serde::Serialize; use crate::BenchmarkCmd; use frame_benchmarking::{ - Analysis, AnalysisChoice, BenchmarkBatchSplitResults, BenchmarkResults, BenchmarkSelector, + Analysis, AnalysisChoice, BenchmarkBatchSplitResults, BenchmarkResult, BenchmarkSelector, RegressionModel, }; use frame_support::traits::StorageInfo; @@ -359,7 +359,7 @@ pub fn write_results( // each benchmark. fn add_storage_comments( comments: &mut Vec, - results: &[BenchmarkResults], + results: &[BenchmarkResult], storage_info: &[StorageInfo], ) { let mut storage_info_map = storage_info @@ -377,6 +377,16 @@ fn add_storage_comments( }; storage_info_map.insert(skip_storage_info.prefix.clone(), &skip_storage_info); + // Special hack to show `Benchmark Override` + let benchmark_override = StorageInfo { + pallet_name: b"Benchmark".to_vec(), + storage_name: b"Override".to_vec(), + prefix: b"Benchmark Override".to_vec(), + max_values: None, + max_size: None, + }; + storage_info_map.insert(benchmark_override.prefix.clone(), &benchmark_override); + // This tracks the keys we already identified, so we only generate a single comment. let mut identified = HashSet::>::new(); @@ -502,7 +512,7 @@ where #[cfg(test)] mod test { use super::*; - use frame_benchmarking::{BenchmarkBatchSplitResults, BenchmarkParameter, BenchmarkResults}; + use frame_benchmarking::{BenchmarkBatchSplitResults, BenchmarkParameter, BenchmarkResult}; fn test_data( pallet: &[u8], @@ -513,7 +523,7 @@ mod test { ) -> BenchmarkBatchSplitResults { let mut results = Vec::new(); for i in 0..5 { - results.push(BenchmarkResults { + results.push(BenchmarkResult { components: vec![(param, i), (BenchmarkParameter::z, 0)], extrinsic_time: (base + slope * i).into(), storage_root_time: (base + slope * i).into(),