new proc-macro-based benchmarking syntax

[sam0x17]

Description

This PR introduces a new, simplified, and easier-to-use syntax for defining benchmarks that is based on a proc macro instead of a macro_rules! macro like the current system.

Right now the syntax looks like this:

#[benchmarks]
mod benchmarks {
    use super::*;
    ...
    #[benchmark]
    fn transfer_increasing_users(u: Linear<0, 1_000>) {
        let existential_deposit = T::ExistentialDeposit::get();
        let caller = whitelisted_caller();
        let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
        let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&caller, balance);
        let recipient: T::AccountId = account("recipient", 0, SEED);
        let recipient_lookup = T::Lookup::unlookup(recipient.clone());
        let transfer_amount = existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1u32.into();
        for i in 0..u {
            let new_user: T::AccountId = account("new_user", i, SEED);
            let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&new_user, balance);
        }

        #[extrinsic_call]
        transfer(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount);

        assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
        assert_eq!(Balances::<T, I>::free_balance(&recipient), transfer_amount);
    }
    ...
}

Syntax

The #[benchmarks] macro expects a module containing any number of benchmark puesdo function definitions such as the one shown above to be provided in a block. These function definitions should be annotated with #[benchmark]. A where clause can be provided as an optional argument i.e. #[benchmarks(where <clause here>)].

Each benchmark function definition should be zero arg, or should specify one or more named parameters (that meets the requirements of frame_benchmarking::BenchmarkParameter with a type that implements frame_support::benchmarking::ParamRange. Right now the only struct implementing this trait is Linear, which has two generic u32 parameters`:

pub struct Linear<const A: u32, const B: u32>;

These generic parameters represent the (inclusive) start and end range of the benchmark parameter. So if you want to specify a range of 1 to 100,000 (inclusive) for the variable x, you would use:

#[benchmark]
fn my_benchmark(x: Linear<1, 100_000>) {

As mentioned before, you can also specify multiple parameters if necessary:

#[benchmark]
fn my_benchmark(x: Linear<100, 500>, y: Linear<200, 600>) {

These benchmark function definitions can be optionally annotated with the extra and skip_meta i.e. #[benchmark(skip_meta, extra)] and these function the same way they used to function when they were attributes in the old benchmarking syntax

Note that while these benchmark function definitions appear to be function definitions, currently they are not compiled as such (mainly because the signature would be incorrect with respect to the benchmark parameters). If generating a function that resembles the benchmark function definition is desirable, I could easily add this by changing the signature at compile time to transform things like x: Linear<0, 10> to x: u32. If this would be of practical use beyond the structs and trait impls already created by the macro then let me know!

The #[extrinsic_call] acts as a bisector between the setup code, the extrinsic call, and any verification code that needs to go at the end of the benchmark. The extrinsic call can be either block style (pass a block to #[extrinsic_call]), or must be a function call do an extrinsic where the first argument is the origin.

UPDATE: now we use #[block] for the block-style call, and #[extrinsic_call] for an actual extrinsic call.

Instance Benchmarks

You can specify #[instance_benchmarks] in place of #[benchmarks] if your pallet requires this.

Outer Macro Pattern

The new setup uses proc macros and the outer macro pattern, similar to the way pallets themselves are defined, with the caveat that individual benchmark function definitions can technically be parsed and expanded on their own -- the only reason we need the outer macro pattern is once all benchmarks within a benchmarks! {} block have been processed, we need to iterate over them and implement some traits such as run_benchmarks that require knowledge of all of the benchmarks, so this is why we need the outer macro pattern.

Status

• parsing for new #[benchmark] attribute
• parsing "bisection" technique for picking out the #[extrinsic_call] and using this to separate the setup code from the verification code
• new ParamRange trait and a generic Linear struct that implements it
• parsing support for ParamRange
• parsing for new benchmarks! {} outer macro
• basic expansion for #[benchmark] functions
• expansion of benchmarks! {} macro
• conversion of the Balances pallet to the new syntax
• decide on syntax for specifying that we want to code-gen benchmark tests (previously was impl_benchmark_test_suite!, maybe we want to keep this? Update: yes, we need this
• parsing + expansion for test suite syntax, if applicable
• proper handling of where clauses
• docs
• UI tests

Related Issues

• closes Migrate existing benchmarks! macro to attribute macros #10848

[sam0x17]

I now have the entire balances pallet benchmarking suite emitting code and compiling using the new syntax, and this is using an entirely new pipeline that doesn't use the old macros at all. New bit that I'm starting now is I need to make new syntax for doing the impl_benchmark_test_suite! expansion.

[sam0x17]

For anyone who might need it, this is the one-liner to expand all output of the balances pallet benchmarking code:

cargo expand --tests --lib benchmarking --features=runtime-benchmarks

[sam0x17]

Some high level open questions:

1. do we want new syntax for specifying that benchmark test suites should be generated, if so, what should that look like? Right now the old syntax works fine inline with the new syntax, so we could address this in a future PR, but I've already been looking into it either way. Update: I am working on implementing this, the new syntax does not work out of the box with the old syntax so I do need to do this so we can have complete benchmark tests for the balances pallet.
2. Is it correct that within groups of benchmarks, the entire group is always made up of all instance benchmarks or all non-instance benchmarks. If so I can simplify some syntax as mentioned above. I'm 90% sure this is correct. update: This is correct.
3. Do we want to generate function definitions as well resembling the benchmark function syntax, as mentioned above? update: Looks like no

cc @kianenigma @shawntabrizi @bkchr

[sam0x17]

@ggwpez one question that arose when converting the message-queue pallet benchmarks to use the new syntax:

In situations like

substrate/frame/message-queue/src/benchmarking.rs

Line 172 in 9e7a9f9

MessageQueue::<T>::execute_overweight(RawOrigin::Signed(whitelisted_caller()).into(), 0u32.into(), 0u32, ((msgs - 1) as u32).into(), Weight::MAX).unwrap()

, would it be preferable that the .unwrap() is executed as part of the extrinsic call (i.e. would it be preferable that the overhead of .unwrap() is included in what we measure during benchmarking), or would it be better to execute this .unwrap() and other chained methods during the verify stage?

The same principle applies here for this .ok():

substrate/frame/message-queue/src/benchmarking.rs

Line 48 in 9e7a9f9

neighbours = MessageQueue::<T>::ready_ring_knit(&mid).ok();

There are two options I could follow to handle this:

1. update my parsing code to also allow for method chains within the extrinsic call, allowing lines like this to remain unchanged
2. require the programmer to refactor lines like this so they resemble the following (force methods chained on the extrinsic call to execute during the verify stage):

#[extrinsic_call]
let call = MessageQueue::<T>::execute_overweight(RawOrigin::Signed(whitelisted_caller()).into(), 0u32.into(), 0u32, ((msgs - 1) as u32).into(), Weight::MAX);

call.unwrap(); // executed during verification stage

(2) would be easier, but I can do either. I suspect it might be necessary to have the .unwrap() as part of the extrinsic call, but I don't know for sure.

[KiChjang]

Just one bit about the syntax -- is it possible to make benchmarks! the outer macro become an attribute macro instead, i.e. very much similar to how the pallet macro is an attribute macro over a rust module?

[sam0x17]

@KiChjang

Just one bit about the syntax -- is it possible to make benchmarks! the outer macro become an attribute macro instead, i.e. very much similar to how the pallet macro is an attribute macro over a rust module?

I originally used this approach. The reason I moved away from it is that custom inner attribute macros are unstable, so you would have to have a file called benchmarking.rs with another module (presumably called benchmarking) inside of that, which you would then attach the attribute macro to. If custom inner attribute macros were stable, I could do #![benchmarks] at the top of the file, and this whole thing would work perfectly, but yeah, for these reasons I opted for just having benchmarks! {} and having that inject into whatever module it is run in.

Happy to do it the other way (wouldn't be a difficult change), but it would mean we would have more nesting of modules than we have now.

[KiChjang]

Regarding the unwrap or ok being part of the extrinsic call, fundamentally speaking, I don't see why that should be the case. You are measuring the overhead of an extrinsic only, and anything extra is strictly speaking not part of an extrinsic call, thus option 2 is much more preferable.

[sam0x17]

Regarding the unwrap or ok being part of the extrinsic call, fundamentally speaking, I don't see why that should be the case. You are measuring the overhead of an extrinsic only, and anything extra is strictly speaking not part of an extrinsic call, thus option 2 is much more preferable.

@KiChjang I was hoping that was the case -- in that case the easier option is also an option more accurate than what we currently do in several places :)

[KiChjang]

@KiChjang

Just one bit about the syntax -- is it possible to make benchmarks! the outer macro become an attribute macro instead, i.e. very much similar to how the pallet macro is an attribute macro over a rust module?

I originally used this approach. The reason I moved away from it is that custom inner attribute macros are unstable, so you would have to have a file called benchmarking.rs with another module (presumably called benchmarking) inside of that, which you would then attach the attribute macro to. If custom inner attribute macros were stable, I could do #![benchmarks] at the top of the file, and this whole thing would work perfectly, but yeah, for these reasons I opted for just having benchmarks! {} and having that inject into whatever module it is run in.

Happy to do it the other way (wouldn't be a difficult change), but it would mean we would have more nesting of modules than we have now.

I was wondering about that as I finished typing, but this seems easily solvable with a re-export of the inner module types, much like how we often place a use pallet::*; on the pallet macro.

[sam0x17]

Ok @KiChjang and I spoke, it looks like we can avoid the double nesting problem the same way we do with pallet modules, by doing something like this:

#[benchmarks]
mod benchmarks {
    ...
}

Part of the expansion would inject a use benchmarks::* line that matches the visibility of the module, which solves the problem of having something like benchmarks::benchmarks::whatever

This also opens up the possibility of having a number of args directly on the #[benchmark] attribute, so will be refactoring a few things tonight and updating the description above accordingly :)

[kianenigma]

Regarding #12924 (comment), I think forcing programmer to put everything in one function is a bit to restrictive. I certainly remember writing benchmarks when the area that I wanted to measure was multiple function calls, also including some calls to .ok and such.

Also happy to hear that Keith's suggestion is feasible, as it looks a lot nicer. Then, we can see a pallet like:

#[pallet::pallet]
mod pallet { .. }

#[pallet::benchmarks]
mod benchmarks { .. }

[sam0x17]

@kianenigma

I certainly remember writing benchmarks when the area that I wanted to measure was multiple function calls, also including some calls to .ok and such.

I could allow a block, but in these situations, how do you specify the origin? This is one of the things that gets passed to the underlying benchmarking impls. For example given the extrinsic call below from the transfer method in the balances pallet, it extracts the first arg of the function call as the origin (RawOrigin::Signed(caller.clone())):

	#[benchmark]
	fn transfer() {
		let existential_deposit = T::ExistentialDeposit::get();
		let caller = whitelisted_caller();

		// Give some multiple of the existential deposit
		let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
		let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&caller, balance);

		// Transfer `e - 1` existential deposits + 1 unit, which guarantees to create one account,
		// and reap this user.
		let recipient: T::AccountId = account("recipient", 0, SEED);
		let recipient_lookup = T::Lookup::unlookup(recipient.clone());
		let transfer_amount =
			existential_deposit.saturating_mul((ED_MULTIPLIER - 1).into()) + 1u32.into();

		#[extrinsic_call]
		transfer(RawOrigin::Signed(caller.clone()), recipient_lookup, transfer_amount);

		assert_eq!(Balances::<T, I>::free_balance(&caller), Zero::zero());
		assert_eq!(Balances::<T, I>::free_balance(&recipient), transfer_amount);
	}

How would this work with multiple calls? Or you just mean you want to allow chaining (which would be fine, I just have to extend some things in that case)

[ggwpez]

I could allow a block, but in these situations, how do you specify the origin?

We definitely need blocks. Just pure simple Rust code blocks, no parsing of that or anything. For example in the lottery pallet. There is no origin required, its just a code block.
Otherwise we cannot benchmark arbitrary runtime code. Sorry if that was not clear so far.
Maybe it makes sense to have a different attribute than #[extrinsic_call] in that case, not sure.

Regarding the unwrap: This also only happens in a code-block and therefore happens inside the benchmarked code.
You can get around that by using temporary variables, so I would not focus on optimizing out one unwrap() call.

[sam0x17]

We could just have origin as an arg on the attribute

…

On Fri, Jan 6, 2023 at 9:05 AM Oliver Tale-Yazdi ***@***.***> wrote: I could allow a block, but in these situations, how do you specify the origin? We definitely need blocks. Just pure simple Rust code blocks, no parsing of that or anything. For example in the lottery <https://github.com/paritytech/substrate/blob/b6f849b3e569fc01f716aa09aba46761ad5f4178/frame/lottery/src/benchmarking.rs#L124> pallet. There is no origin required, its just a code block. Otherwise we cannot benchmark arbitrary runtime code. Sorry if that was not clear so far. Maybe it makes sense to have a different attribute than #[extrinsic_call] in that case, not sure. — Reply to this email directly, view it on GitHub <#12924 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOE4LICYPRTNCYQEASOCDTWRARBXANCNFSM6AAAAAAS5UDDUM> . You are receiving this because you were assigned.Message ID: ***@***.***>

[ggwpez]

We could just have origin as an arg on the attribute

You mean on the #[extrinsic_call]?
Then it looks less like Rust… I think we should try to make it as Rusty as possible. I still dont see where the problem is with the origin. Blocks dont have origin. What I mean:

Origin needed here:

stop_repeat {
	let origin = …
}: _<T::RuntimeOrigin>(origin)
verify {
}

Origin not needed nor wanted here:

on_initialize_end {
	// …
}: {
	// Generate `MaxGenerateRandom` numbers for worst case scenario
	for i in 0 .. T::MaxGenerateRandom::get() {
		Lottery::<T>::generate_random_number(i);
	}
	// Start lottery has block 15 configured for payout
	Lottery::<T>::on_initialize(15u32.into());
}
verify {
	// …
}

[sam0x17]

Got it, I didn't realize the 3 different variants had different expansions 🙃

[sam0x17]

100% working now, finishing up docs 🎉

[ggwpez]

100% working now, finishing up docs tada

Looking really good, nice 🎉 !

For whats its worth, I think we could opt for a more compact syntax for the attributes:

#[benchmark(extra, skip_meta)]

instead of

#[benchmark]
#[extra]
#[skip_meta]

What do you think?

[sam0x17]

Yes I think this would be great. Adding now

…

On Jan 8, 2023, at 3:50 PM, Oliver Tale-Yazdi ***@***.***> wrote: 100% working now, finishing up docs tada Looking really good, nice 🎉 ! For whats its worth, I think we could opt for a more compact syntax for the attributes: #[benchmark(extra, skip_meta)] instead of #[benchmark] #[extra] #[skip_meta] What do you think? — Reply to this email directly, view it on GitHub <#12924 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOE4LPGKQQDH7JJ2COZFFTWRMSDHANCNFSM6AAAAAAS5UDDUM>. You are receiving this because you were assigned.

[sam0x17]

added.

[sam0x17]

Need to add slightly more details in docs about the extra and skip_meta options and then this is g2g

[bkchr]

You are exporting the macro from frame-support, but require here that frame-benchmarking is present in the Cargo.toml. This is a macro anti pattern. The crate from where you re-export the macro is the crate that you search for in the Cargo.toml.

[sam0x17]

Yeah the reason I need frame-benchmarking is because this is where all the benchmarking-related traits and structs are. I put things in frame support for two reasons:

1. if I put this in frame-benchmarking there would be a name collision with benchmarks!
2. frame-benchmarking does not have a sub-crate for procedural macros (like the one in frame_support), so if we do move this over to frame-benchmarking, we will have to make a sub-crate and deal with the name collision issue.

Open to any suggestions. Discussion would be great to have on the follow-up issue here: https://github.com/paritytech/substrate/issues/13132

[bkchr]

2. frame-benchmarking does not have a sub-crate for procedural macros (like the one in frame_support), so if we do move this over to frame-benchmarking, we will have to make a sub-crate and deal with the name collision issue.

You can still reuse frame-support-procedural, but just re-export the macro from frame-benchmarking?

1. if I put this in frame-benchmarking there would be a name collision with benchmarks!

When you have a declarative macro and a type with the same name, you import both with the same name. Then when you use Rust resolves the correct thing you want. Does that maybe also works for the attribute macro? Or we rename the proc-macro.

[sam0x17]

Does that maybe also works for the attribute macro? Or we rename the proc-macro.

This is a great suggestion if this is true. I will try it out and see! Then we could have everything live under frame_benchmarking

[sam0x17]

update: this seems to work, see #13235

[Polkadot-Forum]

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/substrate-developer-newsletter-february-2023-notable-updates-for-frame-devs-new-deep-dives-released-calling-builders-to-apply-to-rfps-and-bounties/2137/1