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

Custom Benchmark Errors and Override #9517

Merged
15 commits merged into from
Aug 19, 2021
Merged

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Aug 8, 2021

This PR allows the benchmarking pipeline to return a custom BenchmarkError error type.

With this new error type, we can allow a user to define a "default" weight to output if a benchmark would fail, or is otherwise unimplemented:

override_benchmark {
	let b in 1 .. 1000;
	let caller = account::<T::AccountId>("caller", 0, 0);
}: {
	Err(BenchmarkError::Override(
		BenchmarkResult {
			extrinsic_time: 1_234_567_890,
			reads: 1337,
			writes: 420,
			..Default::default()
		}
	))?;
}

Any data set here will then be swallowed by the benchmarking pipeline, and be used in the final output.

polkadot companion: paritytech/polkadot#3600

@shawntabrizi shawntabrizi added A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. B7-runtimenoteworthy labels Aug 8, 2021
@shawntabrizi shawntabrizi added this to In progress in Runtime via automation Aug 8, 2021
@shawntabrizi shawntabrizi moved this from In progress to Please Review in Runtime Aug 8, 2021
frame/elections-phragmen/src/benchmarking.rs Outdated Show resolved Hide resolved
frame/benchmarking/src/utils.rs Outdated Show resolved Hide resolved
#[derive(Encode, Decode, Clone, PartialEq, Debug)]

pub enum BenchmarkError {
Stop(#[codec(skip)] &'static str),
Copy link
Member

Choose a reason for hiding this comment

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

Why do you skip this here?

Copy link
Member Author

@shawntabrizi shawntabrizi Aug 9, 2021

Choose a reason for hiding this comment

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

What is the proper fix for?

error[E0277]: the trait bound `&'static str: WrapperTypeDecode` is not satisfied
   --> frame/benchmarking/src/utils.rs:128:7
    |
128 |     Stop(&'static str),
    |          ^ the trait `WrapperTypeDecode` is not implemented for `&'static str`
    | 
   ::: /Users/shawntabrizi/.cargo/registry/src/github.com-1ecc6299db9ec823/parity-scale-codec-2.2.0/src/codec.rs:284:18
    |
284 |     fn decode<I: Input>(input: &mut I) -> Result<Self, Error>;
    |                  ----- required by this bound in `utils::_::_parity_scale_codec::Decode::decode`
    |
    = note: required because of the requirements on the impl of `Decode` for `&'static str`

Copy link
Member

Choose a reason for hiding this comment

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

Use sp_runtime::RuntimeString

Copy link
Member Author

Choose a reason for hiding this comment

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

Runtime string came with a whole host of other issues, again related to Arbitrary Error -> Static String, then Static String -> Runtime String, since it rust can't figure out to do two jumps.

Instead I removed Encode, Decode cause it just wasnt needed

shawntabrizi and others added 2 commits August 9, 2021 11:13
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@thiolliere
Copy link
Contributor

what is the usecase for this ?

@kianenigma
Copy link
Contributor

what is the usecase for this ?

For XCM, we want to be able to specify a benchmark for all commands and orders, but based on runtime configurations, return custom values (example: return u64::max_value() to signal "don't send this XCM to me since it is not supported"). I think there are similar scenarios as well in substrate (see elections-phragmen)

Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

LGTM.
Just a little unsure the override should be an error.
(but probably too bikesheddy to be worth delaying the PR for)

Runtime automation moved this from Please Review to Needs Audit Aug 18, 2021
@@ -108,6 +112,50 @@ pub struct BenchmarkResults {
pub keys: Vec<(Vec<u8>, u32, u32, bool)>,
}

impl BenchmarkResult {
pub fn from_weight(w: Weight) -> Self {
Self { extrinsic_time: (w as u128) / 1_000, ..Default::default() }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not implement From<Weight> for BenchmarkResult?

Copy link
Contributor

@KiChjang KiChjang left a comment

Choose a reason for hiding this comment

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

Looks okay overall; doesn't quite matter if the comment is addressed or not.

@shawntabrizi
Copy link
Member Author

bot merge

@ghost
Copy link

ghost commented Aug 19, 2021

Waiting for commit status.

@ghost ghost merged commit 643818d into master Aug 19, 2021
@ghost ghost deleted the shawntabrizi-custom-benchmark-error branch August 19, 2021 12:34
Runtime automation moved this from Needs Audit to Done Aug 19, 2021
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime Aug 19, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants