From a78c6c90efafcbf33002a08c6a709e19eee2330d Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 2 Dec 2022 10:49:18 +0000 Subject: [PATCH 1/4] Calls can be 'Weightless' Signed-off-by: Oliver Tale-Yazdi --- frame/benchmarking/src/lib.rs | 25 +++++++++++++++++++++++++ frame/benchmarking/src/utils.rs | 6 ++++++ 2 files changed, 31 insertions(+) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index a221eccb82c85..6dca7cbcae564 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -881,6 +881,13 @@ macro_rules! impl_bench_name_tests { "WARNING: benchmark error skipped - {}", stringify!($name), ); + }, + $crate::BenchmarkError::Weightless => { + // This is considered a success condition. + $crate::log::error!( + "WARNING: benchmark weightless skipped - {}", + stringify!($name), + ); } } }, @@ -1640,6 +1647,14 @@ macro_rules! impl_test_function { .expect("benchmark name is always a valid string!"), ); } + $crate::BenchmarkError::Weightless => { + // This is considered a success condition. + $crate::log::error!( + "WARNING: benchmark weightless skipped - {}", + $crate::str::from_utf8(benchmark_name) + .expect("benchmark name is always a valid string!"), + ); + } } }, Ok(Ok(())) => (), @@ -1792,6 +1807,16 @@ macro_rules! add_benchmark { .expect("benchmark name is always a valid string!") ); None + }, + Err($crate::BenchmarkError::Weightless) => { + $crate::log::error!( + "WARNING: benchmark weightless skipped - {}", + $crate::str::from_utf8(benchmark) + .expect("benchmark name is always a valid string!") + ); + let mut dummy = $crate::BenchmarkResult::default(); + dummy.components = selected_components.clone(); + Some(vec![dummy.clone(), dummy.clone()]) } }; diff --git a/frame/benchmarking/src/utils.rs b/frame/benchmarking/src/utils.rs index 753e8c1c684ee..654b1c34c0658 100644 --- a/frame/benchmarking/src/utils.rs +++ b/frame/benchmarking/src/utils.rs @@ -162,6 +162,11 @@ pub enum BenchmarkError { /// The benchmarking pipeline is allowed to fail here, and we should simply /// skip processing these results. Skip, + /// No weight can be determined; set the weight of this call to zero. + /// + /// You can also use `Override` instead, but this is easier to use since `Override` expects the + /// correct components to be present. + Weightless, } impl From for &'static str { @@ -170,6 +175,7 @@ impl From for &'static str { BenchmarkError::Stop(s) => s, BenchmarkError::Override(_) => "benchmark override", BenchmarkError::Skip => "benchmark skip", + BenchmarkError::Weightless => "benchmark weightless", } } } From 3bfe859e7a0b113649ba99f2ed704efdbca68681 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 2 Dec 2022 10:49:48 +0000 Subject: [PATCH 2/4] Fix (child)-bounties benches Signed-off-by: Oliver Tale-Yazdi --- frame/bounties/src/benchmarking.rs | 21 +++++++++++---------- frame/child-bounties/src/benchmarking.rs | 10 ++++++---- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/frame/bounties/src/benchmarking.rs b/frame/bounties/src/benchmarking.rs index 84f5665d402a1..adadb3411ac65 100644 --- a/frame/bounties/src/benchmarking.rs +++ b/frame/bounties/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; -use frame_benchmarking::{account, benchmarks_instance_pallet, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks_instance_pallet, whitelisted_caller, BenchmarkError}; use frame_system::RawOrigin; use sp_runtime::traits::Bounded; @@ -31,13 +31,14 @@ use pallet_treasury::Pallet as Treasury; const SEED: u32 = 0; // Create bounties that are approved for use in `on_initialize`. -fn create_approved_bounties, I: 'static>(n: u32) -> Result<(), &'static str> { +fn create_approved_bounties, I: 'static>(n: u32) -> Result<(), BenchmarkError> { for i in 0..n { let (caller, _curator, _fee, value, reason) = setup_bounty::(i, T::MaximumReasonLength::get()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; let bounty_id = BountyCount::::get() - 1; - let approve_origin = T::ApproveOrigin::successful_origin(); + let approve_origin = + T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin, bounty_id)?; } ensure!(BountyApprovals::::get().len() == n as usize, "Not all bounty approved"); @@ -62,13 +63,14 @@ fn setup_bounty, I: 'static>( } fn create_bounty, I: 'static>( -) -> Result<(AccountIdLookupOf, BountyIndex), &'static str> { +) -> Result<(AccountIdLookupOf, BountyIndex), BenchmarkError> { let (caller, curator, fee, value, reason) = setup_bounty::(0, T::MaximumReasonLength::get()); let curator_lookup = T::Lookup::unlookup(curator.clone()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; let bounty_id = BountyCount::::get() - 1; - let approve_origin = T::ApproveOrigin::successful_origin(); + let approve_origin = + T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; Treasury::::on_initialize(T::BlockNumber::zero()); Bounties::::propose_curator(approve_origin, bounty_id, curator_lookup.clone(), fee)?; @@ -97,7 +99,7 @@ benchmarks_instance_pallet! { let (caller, curator, fee, value, reason) = setup_bounty::(0, T::MaximumReasonLength::get()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; let bounty_id = BountyCount::::get() - 1; - let approve_origin = T::SpendOrigin::successful_origin(); + let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; }: _(approve_origin, bounty_id) propose_curator { @@ -106,10 +108,9 @@ benchmarks_instance_pallet! { let curator_lookup = T::Lookup::unlookup(curator); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; let bounty_id = BountyCount::::get() - 1; - let approve_origin = T::SpendOrigin::successful_origin(); - Bounties::::approve_bounty(approve_origin, bounty_id)?; + let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; Treasury::::on_initialize(T::BlockNumber::zero()); - let approve_origin = T::SpendOrigin::successful_origin(); }: _(approve_origin, bounty_id, curator_lookup, fee) // Worst case when curator is inactive and any sender unassigns the curator. @@ -128,7 +129,7 @@ benchmarks_instance_pallet! { let curator_lookup = T::Lookup::unlookup(curator.clone()); Bounties::::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?; let bounty_id = BountyCount::::get() - 1; - let approve_origin = T::SpendOrigin::successful_origin(); + let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; Bounties::::approve_bounty(approve_origin.clone(), bounty_id)?; Treasury::::on_initialize(T::BlockNumber::zero()); Bounties::::propose_curator(approve_origin, bounty_id, curator_lookup, fee)?; diff --git a/frame/child-bounties/src/benchmarking.rs b/frame/child-bounties/src/benchmarking.rs index 697ed40e0071f..04a8583f286f1 100644 --- a/frame/child-bounties/src/benchmarking.rs +++ b/frame/child-bounties/src/benchmarking.rs @@ -21,7 +21,7 @@ use super::*; -use frame_benchmarking::{account, benchmarks, whitelisted_caller}; +use frame_benchmarking::{account, benchmarks, whitelisted_caller, BenchmarkError}; use frame_system::RawOrigin; use crate::Pallet as ChildBounties; @@ -94,7 +94,7 @@ fn setup_child_bounty(user: u32, description: u32) -> BenchmarkChildB fn activate_bounty( user: u32, description: u32, -) -> Result, &'static str> { +) -> Result, BenchmarkError> { let mut child_bounty_setup = setup_child_bounty::(user, description); let curator_lookup = T::Lookup::unlookup(child_bounty_setup.curator.clone()); Bounties::::propose_bounty( @@ -105,7 +105,9 @@ fn activate_bounty( child_bounty_setup.bounty_id = Bounties::::bounty_count() - 1; - Bounties::::approve_bounty(RawOrigin::Root.into(), child_bounty_setup.bounty_id)?; + let approve_origin = + T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + Bounties::::approve_bounty(approve_origin, child_bounty_setup.bounty_id)?; Treasury::::on_initialize(T::BlockNumber::zero()); Bounties::::propose_curator( RawOrigin::Root.into(), @@ -124,7 +126,7 @@ fn activate_bounty( fn activate_child_bounty( user: u32, description: u32, -) -> Result, &'static str> { +) -> Result, BenchmarkError> { let mut bounty_setup = activate_bounty::(user, description)?; let child_curator_lookup = T::Lookup::unlookup(bounty_setup.child_curator.clone()); From 166de202eb17711bec5f3f40e6ea729d65d1e77c Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 2 Dec 2022 13:44:53 +0000 Subject: [PATCH 3/4] Just use one dummy value Signed-off-by: Oliver Tale-Yazdi --- frame/benchmarking/src/lib.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 6dca7cbcae564..51c3e4b0e2f64 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1814,9 +1814,12 @@ macro_rules! add_benchmark { $crate::str::from_utf8(benchmark) .expect("benchmark name is always a valid string!") ); - let mut dummy = $crate::BenchmarkResult::default(); - dummy.components = selected_components.clone(); - Some(vec![dummy.clone(), dummy.clone()]) + let dummy = $crate::BenchmarkResult { + components: selected_components.clone(), + .. Default::default() + }; + // The linear regression logic needs at least two values. + Some(vec![dummy]) } }; From a7ecf5bfb854cd6876f9074c797978c7a28cdc2e Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 2 Dec 2022 13:46:01 +0000 Subject: [PATCH 4/4] :facepalm: Signed-off-by: Oliver Tale-Yazdi --- frame/benchmarking/src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 51c3e4b0e2f64..29fa0b6a6af70 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1814,12 +1814,10 @@ macro_rules! add_benchmark { $crate::str::from_utf8(benchmark) .expect("benchmark name is always a valid string!") ); - let dummy = $crate::BenchmarkResult { + Some(vec![$crate::BenchmarkResult { components: selected_components.clone(), .. Default::default() - }; - // The linear regression logic needs at least two values. - Some(vec![dummy]) + }]) } };