Test each benchmark case in own #[test]

[ucover]

Updates the impl_benchmark_test_suite! and extends the benchmarks! macros.
Closes #9110

Syntax extension

It is now possible to use impl_benchmark_test_suite! within the benchmarks! macro.

Example:

benchmarks! {
	dummy {
		…
	}: {
		…
	}

	impl_benchmark_test_suite!(Pallet, crate::tests::new_test_ext(), crate::tests::Test) // ; allowed here
}

Behavioural change

When impl_benchmark_test_suite! is used in a benchmarks! call, it now generates one #[test] function for each bench case.

Old output:

test benchmarking::benchmark_tests::test_benchmarks ... ok

New output:

test benchmarking::bench_sort_vector ... ok
test benchmarking::bench_accumulate_dummy ... ok
test benchmarking::bench_set_dummy_benchmark ... ok

👉 benchmarks! and impl_benchmark_test_suite! stay backwards compatible.

Parallel benchmarks

Since each bench case has its own #[test] function, it is possible to run them in parallel.
A good example is the contracts pallet:

Result with old syntax (after compilation):

cargo test --features=runtime-benchmarks -- --test-threads 16  58,85s user 0,61s system 108% cpu 54,619 total

Result with new syntax (after compilation):

cargo test --features=runtime-benchmarks -- --test-threads 16  65,28s user 0,87s system 264% cpu 24,981 total

Open points

• I am not sure if I ran all test. Some down-stream tests would be good for the compatibility check.
• Comments need to be checked for changes.
• I have not bumped the version.

polkadot address: 13XwRjzNXFGUBwi8uRDU4AgFuTai7rFPRQ9MVA3M7ci8d23r

[cla-bot-2021]

User @ucover, please sign the CLA here.

[shawntabrizi]

thank you, looks good to me besides the comments above

[gui1117]

looks good to me once doc about new feature is added

[ucover]

looks good to me once doc about new feature is added

So how does this work? Where do you want it documented?
@thiolliere

[gui1117]

looks good to me once doc about new feature is added

So how does this work? Where do you want it documented? @thiolliere

I just mean this #9860 (comment) benchmark have a new feature: improved test suite when impl_benchmark_test_suite is written inside the macro. So that needs to be documented in the rust-doc of the item benchmark: here

substrate/frame/benchmarking/src/lib.rs

Lines 58 to 191 in 0d44d83

	/// Construct pallet benchmarks for weighing dispatchables.
	///
	/// Works around the idea of complexity parameters, named by a single letter (which is usually
	/// upper cased in complexity notation but is lower-cased for use in this macro).
	///
	/// Complexity parameters ("parameters") have a range which is a `u32` pair. Every time a benchmark
	/// is prepared and run, this parameter takes a concrete value within the range. There is an
	/// associated instancing block, which is a single expression that is evaluated during
	/// preparation. It may use `?` (`i.e. `return Err(...)`) to bail with a string error. Here's a
	/// few examples:
	///
	/// ```ignore
	/// // These two are equivalent:
	/// let x in 0 .. 10;
	/// let x in 0 .. 10 => ();
	/// // This one calls a setup function and might return an error (which would be terminal).
	/// let y in 0 .. 10 => setup(y)?;
	/// // This one uses a code block to do lots of stuff:
	/// let z in 0 .. 10 => {
	/// let a = z * z / 5;
	/// let b = do_something(a)?;
	/// combine_into(z, b);
	/// }
	/// ```
	///
	/// Note that due to parsing restrictions, if the `from` expression is not a single token (i.e. a
	/// literal or constant), then it must be parenthesised.
	///
	/// The macro allows for a number of "arms", each representing an individual benchmark. Using the
	/// simple syntax, the associated dispatchable function maps 1:1 with the benchmark and the name of
	/// the benchmark is the same as that of the associated function. However, extended syntax allows
	/// for arbitrary expresions to be evaluated in a benchmark (including for example,
	/// `on_initialize`).
	///
	/// Note that the ranges are *inclusive* on both sides. This is in contrast to ranges in Rust which
	/// are left-inclusive right-exclusive.
	///
	/// Each arm may also have a block of code which is run prior to any instancing and a block of code
	/// which is run afterwards. All code blocks may draw upon the specific value of each parameter
	/// at any time. Local variables are shared between the two pre- and post- code blocks, but do not
	/// leak from the interior of any instancing expressions.
	///
	/// Example:
	/// ```ignore
	/// benchmarks! {
	/// where_clause { where T::A: From<u32> } // Optional line to give additional bound on `T`.
	///
	/// // first dispatchable: foo; this is a user dispatchable and operates on a `u8` vector of
	/// // size `l`
	/// foo {
	/// let caller = account::<T>(b"caller", 0, benchmarks_seed);
	/// let l in 1 .. MAX_LENGTH => initialize_l(l);
	/// }: _(Origin::Signed(caller), vec![0u8; l])
	///
	/// // second dispatchable: bar; this is a root dispatchable and accepts a `u8` vector of size
	/// // `l`.
	/// // In this case, we explicitly name the call using `bar` instead of `_`.
	/// bar {
	/// let l in 1 .. MAX_LENGTH => initialize_l(l);
	/// }: bar(Origin::Root, vec![0u8; l])
	///
	/// // third dispatchable: baz; this is a user dispatchable. It isn't dependent on length like the
	/// // other two but has its own complexity `c` that needs setting up. It uses `caller` (in the
	/// // pre-instancing block) within the code block. This is only allowed in the param instancers
	/// // of arms.
	/// baz1 {
	/// let caller = account::<T>(b"caller", 0, benchmarks_seed);
	/// let c = 0 .. 10 => setup_c(&caller, c);
	/// }: baz(Origin::Signed(caller))
	///
	/// // this is a second benchmark of the baz dispatchable with a different setup.
	/// baz2 {
	/// let caller = account::<T>(b"caller", 0, benchmarks_seed);
	/// let c = 0 .. 10 => setup_c_in_some_other_way(&caller, c);
	/// }: baz(Origin::Signed(caller))
	///
	/// // You may optionally specify the origin type if it can't be determined automatically like
	/// // this.
	/// baz3 {
	/// let caller = account::<T>(b"caller", 0, benchmarks_seed);
	/// let l in 1 .. MAX_LENGTH => initialize_l(l);
	/// }: baz<T::Origin>(Origin::Signed(caller), vec![0u8; l])
	///
	/// // this is benchmarking some code that is not a dispatchable.
	/// populate_a_set {
	/// let x in 0 .. 10_000;
	/// let mut m = Vec::<u32>::new();
	/// for i in 0..x {
	/// m.insert(i);
	/// }
	/// }: { m.into_iter().collect::<BTreeSet>() }
	/// }
	/// ```
	///
	/// Test functions are automatically generated for each benchmark and are accessible to you when you
	/// run `cargo test`. All tests are named `test_benchmark_<benchmark_name>`, implemented on the
	/// Pallet struct, and run them in a test externalities environment. The test function runs your
	/// benchmark just like a regular benchmark, but only testing at the lowest and highest values for
	/// each component. The function will return `Ok(())` if the benchmarks return no errors.
	///
	/// You can optionally add a `verify` code block at the end of a benchmark to test any final state
	/// of your benchmark in a unit test. For example:
	///
	/// ```ignore
	/// sort_vector {
	/// let x in 1 .. 10000;
	/// let mut m = Vec::<u32>::new();
	/// for i in (0..x).rev() {
	/// m.push(i);
	/// }
	/// }: {
	/// m.sort();
	/// } verify {
	/// ensure!(m[0] == 0, "You forgot to sort!")
	/// }
	/// ```
	///
	/// These `verify` blocks will not affect your benchmark results!
	///
	/// You can construct benchmark tests like so:
	///
	/// ```ignore
	/// #[test]
	/// fn test_benchmarks() {
	/// new_test_ext().execute_with(|| {
	/// assert_ok!(Pallet::<Test>::test_benchmark_dummy());
	/// assert_err!(Pallet::<Test>::test_benchmark_other_name(), "Bad origin");
	/// assert_ok!(Pallet::<Test>::test_benchmark_sort_vector());
	/// assert_err!(Pallet::<Test>::test_benchmark_broken_benchmark(), "You forgot to sort!");
	/// });
	/// }
	/// ```
	#[macro_export]
	macro_rules! benchmarks {

[gui1117]

I wonder if it would be difficult to do a test that ensures those test are correctly generated, maybe by ensuring they fail somewhere, anyway it is fine for now.

[gui1117]

bot merge

[ghost]

Trying merge.

[KiChjang]

Sorry I was late into the game, but I think a follow-up to this would be to figure out a nicer syntax for the impl_benchmark_test_suite macro. Since parsing and expanding the macro falls within the responsibility of benchmarks!, we don't actually need to keep macro syntax for impl_benchmark_test_suite anymore, and can think of a more intuitive one or even a less verbose one for it.

[gui1117]

we could also make it an attribute macro so that people can have the automatic formatting

[shawntabrizi]

/tip medium

[substrate-tip-bot]

Please fix the following problems before calling the tip bot again:

• Contributor did not properly post their Polkadot or Kusama address. Make sure the pull request has: "{network} address: {address}".

[ucover]

Nice bot! Thanks very much 😄

I added my polkadot address to the MR description.
@shawntabrizi @substrate-tip-bot

[athei]

You deserve it. That was bugging me for some time.

[shawntabrizi]

/tip medium

[substrate-tip-bot]

A medium tip was successfully submitted for ucover (13XwRjzNXFGUBwi8uRDU4AgFuTai7rFPRQ9MVA3M7ci8d23r on polkadot).

https://polkadot.js.org/apps/#/treasury/tips