Migrate existing benchmarks! macro to attribute macros

[bkchr]

Currently benchmarks are written in the following form:

benchmarks! {
	bench_name {
		// Define the components, assumed to be linear.
		let x in 0 .. MAX_X;
		let y in 0 .. MAX_Y;
		// Setup code
		let z = x + y;
		let caller = whitelisted_caller();
	}: extrinsic_name(z, other_arguments...) // The extrinsic call.
	verify {
		// Post condition verification
		assert_eq!(MyPallet::<T>::my_var() == z);
	}
	// More benchmarks here
}

A better way of writing them would be closer to how tests are written in rust:

#[pallet::benchmark]
fn bench_name(x: LinearComponent<0, MAX_X>, y: LinearComponent<0, MAX_Y>) {
	// Setup code
	let z = x + y;
	let caller = whitelisted_caller();
	// The extrinsic call.
	extrinsic_name(z, other_arguments...);
	// Post condition verification
	assert_eq!(MyPallet::<T>::my_var(), == z);
}

[bkchr]

For the record, i created this issue by "half accident". Nevertheless, it still needs to be done. So, someone who will work on this, should update the description properly.

[ggwpez]

@shawntabrizi I added some context to the issue.
Feel free to use that as a basis for a concept and modify it, were just my first thoughts.

I think if we use something like LinearComponent we can later on introduce QuadraticComponent et al to upgrade our weight regression logic.

[sam0x17]

So I've been working through the existing benchmarking macro code and I really like the syntax you propose @bkchr

That said, I'm curious what your thoughts are on detecting/annotating the components setup, the extrinsic call, and the post conditions verification code. My understanding based on how the underlying traits and structs work is we do need to be able to separate these out in our macro, but maybe I'm wrong about that. If I'm wrong, then we could totally do your suggested syntax for this without modification, and I'd be totally for that. I'm going to play around a bit more with cargo expand today to get a definitive answer.

If I am right and we do need some way of identifying these separate regions of the code, I wonder if we could just annotate the extrinsic call and use this as a bisection/divider between the components setup and the post-condition verification code, something like this:

#[pallet::benchmark]
fn bench_name(x: LinearComponent<0, MAX_X>, y: LinearComponent<0, MAX_Y>) {
	// Setup code
	let z = x + y;
	let caller = whitelisted_caller();
	// The extrinsic call.
        #[extrinsic_call]
	extrinsic_name(z, other_arguments...);
	// Post condition verification
	assert_eq!(MyPallet::<T>::my_var(), == z);
}

Then we can eat that #[extrinsic_call] attribute during parsing and use its location to get the spans for the setup code, the extrinsic call, and the post conditions / verification and feed these into the benchmarking codegen properly.

Thoughts on this? Is this totally unnecessary?

[ggwpez]

Hey @sam0x17, the suggested code was actually from me 😄
Currently we separate the setup, call and verify code since we only want to measure the time of the actual call.
So we definitely need some way of separating them; I think the separator approach could work. In this case it could throw an error message if there is no #[extrinsic_call] found.
For the call AFAIK it is wrapped in a bypass call filter but that should be easy to extend if you can parse it so far.

High level we want to get closer to the standard Rust syntax as from what we currently have - entirely custom syntax.
But some things still need to happen somehow: Should work with instantiatable pallets, should support where clauses.

The idea about the LinearComponent was just the first I could think of - feel free to come up with something better.
But using something like this would give us the possibility to extend it to QuadraticComponent etc. Only downside is that it is still compile-time only. Currently that is a requirement of the benchmarking backend.

@shawntabrizi probably has some more ideas about it.

[sam0x17]

@ggwpez looks like my reply I had typed out the other day didn't post

That is good to hear. I have been working on parsing code for the above syntax with my #[extrinsic_call] annotation as laid out above. Planning to try to generate the original inner structs and trait impls from there so we can make this minimally invasive.

[sam0x17]

@ggwpez what is the deal with the whole LinearComponent<X, Y> range syntax? Would there be potentially other (non-linear) ones as well, or would we be good just defining a single generic type for this. Why not use a Rust range? That said, I did find the docs note about the existing syntax being inclusive-inclusive and rust ranges being inclusive-exclusive. Anyway just wanted your thoughts!

[ggwpez]

Hi @sam0x17,
currently we have the implicit assumption that every component is linear. As a developer this is probably not obvious (unless you read the docs). I thought by putting it in the type we make it more obvious. Anyway, in the faaar future it would be nice to have other types as well, but for now its just linear.
For the beginning we can keep it simple and use std::ops::{Range, RangeInclusive} instead, if you want. Should be a bit easier to understand what is going on for new devs.

[sam0x17]

So I opted for making a trait called ParamRange and then having Linear implement that trait. This way we can later add more. I ended up moving away from the std::range stuff because I think it adds more confusion and complicates the implementation a bit