Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add Weightless benchmark bailing #12829

Merged
merged 4 commits into from
Dec 2, 2022
Merged

Add Weightless benchmark bailing #12829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a78c6c9
Calls can be 'Weightless'
ggwpez Dec 2, 2022
3bfe859
Fix (child)-bounties benches
ggwpez Dec 2, 2022
166de20
Just use one dummy value
ggwpez Dec 2, 2022
a7ecf5b
:facepalm:
ggwpez Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions frame/benchmarking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
);
}
}
},
Expand Down Expand Up @@ -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(())) => (),
Expand Down Expand Up @@ -1792,6 +1807,17 @@ 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!")
);
Some(vec![$crate::BenchmarkResult {
components: selected_components.clone(),
.. Default::default()
}])
}
};

Expand Down
6 changes: 6 additions & 0 deletions frame/benchmarking/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<BenchmarkError> for &'static str {
Expand All @@ -170,6 +175,7 @@ impl From<BenchmarkError> for &'static str {
BenchmarkError::Stop(s) => s,
BenchmarkError::Override(_) => "benchmark override",
BenchmarkError::Skip => "benchmark skip",
BenchmarkError::Weightless => "benchmark weightless",
}
}
}
Expand Down
21 changes: 11 additions & 10 deletions frame/bounties/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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<T: Config<I>, I: 'static>(n: u32) -> Result<(), &'static str> {
fn create_approved_bounties<T: Config<I>, I: 'static>(n: u32) -> Result<(), BenchmarkError> {
for i in 0..n {
let (caller, _curator, _fee, value, reason) =
setup_bounty::<T, I>(i, T::MaximumReasonLength::get());
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin = T::ApproveOrigin::successful_origin();
let approve_origin =
T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably change all benchmarking code to use try_successful_origin and remove successful_origin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes eventually… this was an oversight when originally writing them.

Bounties::<T, I>::approve_bounty(approve_origin, bounty_id)?;
}
ensure!(BountyApprovals::<T, I>::get().len() == n as usize, "Not all bounty approved");
Expand All @@ -62,13 +63,14 @@ fn setup_bounty<T: Config<I>, I: 'static>(
}

fn create_bounty<T: Config<I>, I: 'static>(
) -> Result<(AccountIdLookupOf<T>, BountyIndex), &'static str> {
) -> Result<(AccountIdLookupOf<T>, BountyIndex), BenchmarkError> {
let (caller, curator, fee, value, reason) =
setup_bounty::<T, I>(0, T::MaximumReasonLength::get());
let curator_lookup = T::Lookup::unlookup(curator.clone());
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin = T::ApproveOrigin::successful_origin();
let approve_origin =
T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Bounties::<T, I>::approve_bounty(approve_origin.clone(), bounty_id)?;
Treasury::<T, I>::on_initialize(T::BlockNumber::zero());
Bounties::<T, I>::propose_curator(approve_origin, bounty_id, curator_lookup.clone(), fee)?;
Expand Down Expand Up @@ -97,7 +99,7 @@ benchmarks_instance_pallet! {
let (caller, curator, fee, value, reason) = setup_bounty::<T, I>(0, T::MaximumReasonLength::get());
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin = T::SpendOrigin::successful_origin();
let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
}: _<T::RuntimeOrigin>(approve_origin, bounty_id)

propose_curator {
Expand All @@ -106,10 +108,9 @@ benchmarks_instance_pallet! {
let curator_lookup = T::Lookup::unlookup(curator);
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin = T::SpendOrigin::successful_origin();
Bounties::<T, I>::approve_bounty(approve_origin, bounty_id)?;
let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Bounties::<T, I>::approve_bounty(approve_origin.clone(), bounty_id)?;
Treasury::<T, I>::on_initialize(T::BlockNumber::zero());
let approve_origin = T::SpendOrigin::successful_origin();
}: _<T::RuntimeOrigin>(approve_origin, bounty_id, curator_lookup, fee)

// Worst case when curator is inactive and any sender unassigns the curator.
Expand All @@ -128,7 +129,7 @@ benchmarks_instance_pallet! {
let curator_lookup = T::Lookup::unlookup(curator.clone());
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
let approve_origin = T::SpendOrigin::successful_origin();
let approve_origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Bounties::<T, I>::approve_bounty(approve_origin.clone(), bounty_id)?;
Treasury::<T, I>::on_initialize(T::BlockNumber::zero());
Bounties::<T, I>::propose_curator(approve_origin, bounty_id, curator_lookup, fee)?;
Expand Down
10 changes: 6 additions & 4 deletions frame/child-bounties/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -94,7 +94,7 @@ fn setup_child_bounty<T: Config>(user: u32, description: u32) -> BenchmarkChildB
fn activate_bounty<T: Config>(
user: u32,
description: u32,
) -> Result<BenchmarkChildBounty<T>, &'static str> {
) -> Result<BenchmarkChildBounty<T>, BenchmarkError> {
let mut child_bounty_setup = setup_child_bounty::<T>(user, description);
let curator_lookup = T::Lookup::unlookup(child_bounty_setup.curator.clone());
Bounties::<T>::propose_bounty(
Expand All @@ -105,7 +105,9 @@ fn activate_bounty<T: Config>(

child_bounty_setup.bounty_id = Bounties::<T>::bounty_count() - 1;

Bounties::<T>::approve_bounty(RawOrigin::Root.into(), child_bounty_setup.bounty_id)?;
let approve_origin =
T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Bounties::<T>::approve_bounty(approve_origin, child_bounty_setup.bounty_id)?;
Treasury::<T>::on_initialize(T::BlockNumber::zero());
Bounties::<T>::propose_curator(
RawOrigin::Root.into(),
Expand All @@ -124,7 +126,7 @@ fn activate_bounty<T: Config>(
fn activate_child_bounty<T: Config>(
user: u32,
description: u32,
) -> Result<BenchmarkChildBounty<T>, &'static str> {
) -> Result<BenchmarkChildBounty<T>, BenchmarkError> {
let mut bounty_setup = activate_bounty::<T>(user, description)?;
let child_curator_lookup = T::Lookup::unlookup(bounty_setup.child_curator.clone());

Expand Down