[FRAME core] ☂ Benchmark syntax V2 follow-ups

[ggwpez]

After paritytech/substrate#12924 is done, some things to consider (cc @sam0x17):

• Remove code duplication with old macro expansion code.
• migrate new benchmarking syntax from frame_support::benchmarking to frame_benchmarking::v2 substrate#13235
• #13277
• benchmarking v2 syntax is confusingly lax about return types, leading to confusion substrate#13278
• benchmarking v2 syntax does not issue warnings on unused benchmark params substrate#13303
• generation of real benchmark functions for benchmarking v2 substrate#13224
• add _(force_origin) support to benchmarking v2 substrate#13721
• FRAME: Avoid boilerplate weight macros #205
• bench syntax v2: run test in single functions #376
• Testing
  • Migrate more pallets so we can be sure that we cover everything with the new syntax.
  • Check for helpful error messages, maybe add help/tutorial links.
  • Pallet UI tests, especially if we add new V2-only syntax.
• In-code docs
  • Allow /// comments on benchmarks. substrate#13383
  • Carrying over benchmark comments into the metadata and output them into the WeightInfo trait.
  • Expose benchmark functions to rust doc. I don think there is a good way of doing this though, since we want them private… maybe having a benchmarks::doc crate which only contains the docs could work.
• Syntax extensions
  • Try if the #[must_use] can work by using Clippy #[must_use] for benchmarking components #285 or manually.
  • Modern version of impl_benchmark_test_suite!.
  • Consider multi-char component names
  • Add #[pov_mode] attribute from Add Proof Size to Weight Output substrate#11637
• External docs
  • Either extend pallet-basic-examples or create a new examples pallet to show off all the V2 syntax as reference.
  • Check all README docs, substrate docs and docs.rs stuff
  • Update Tutorials
• Announce to Community (should do this once we are sure not to have breaking syntax changes)

[sam0x17]

@ggwpez as part of this, do we also want callable benchmark functions based on the existing function defs? I was contemplating just taking the function defs, replacing the Linear components with things like x: u32, and adding a verify boolean to the end of the signature

So this:

#[benchmark]
fn my_benchmark(x: Linear<1, 100>) {
    let caller = whitelisted_caller();
    for i in 0..x {
        do_something();
    }

    #[extrinsic_call]
    _(RawOrigin::Signed(caller.clone()), something, something);

    assert_eq!(something, something);
}

would expand to something like this (in addition to all the traits and structs it already expands to):

#[benchmark]
fn my_benchmark(x: u32, verify: bool) {
    let caller = whitelisted_caller();
    for i in 0..x {
        do_something();
    }

    my_benchmark(RawOrigin::Signed(caller.clone()), something, something);

    if verify {
        assert_eq!(something, something);
    }
}

Then the functions would be acessible directly from the enclosing module like:

benchmarks::my_benchmark(33, true);

I don't know if this would work in all cases. One of the trait impls already basically does this, but I figured people are going to be confused if they realize these function defs don't actually become function defs, so this would fix that

We could also preserve doc comments on these function defs (in addition to putting them on weightinfo, etc)

[ggwpez]

I don't know if this would work in all cases. One of the trait impls already basically does this, but I figured people are going to be confused if they realize these function defs don't actually become function defs, so this would fix that

Not sure how useful this would be, since nobody should call these functions manually anyway. Or in which case would you want to call them directly?

We could also preserve doc comments on these function defs (in addition to putting them on weightinfo, etc)

Yea we should somehow expose the docs i think, but its rather low priority. We could also do some additional doc crate for that since the benches are private.

[sam0x17]

Ok this makes sense. Will focus on the other items first then 👍🏻

[jasl]

Although

#[extrinsic_call]
_(RawOrigin::Signed(caller.clone()), something, something);

is valid syntax, but it looks very rare, and Clion warns #[extrinsic_call] has syntax error (expects '(', ';', , '[' or '}'，but got '_', I shall submit a ticket for them)

parhaps use macro here is more readdable and IDE-friendly?

[sam0x17]

Is that just for the _() version?

…

On Wed, Jan 25, 2023 at 5:49 AM Jun Jiang ***@***.***> wrote: Although #[extrinsic_call] _(RawOrigin::Signed(caller.clone()), something, something); is valid syntax, but it looks very rare, and Clion warns #[extrinsic_call] has syntax error (expects '(', ';', , '[' or '}'，but got '_') parhaps use macro here is more readdable and friendly for IDE? — Reply to this email directly, view it on GitHub <#380>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOE4LLIIE7FMHK5GIKTMK3WUEAKHANCNFSM6AAAAAATZKNEKM> . You are receiving this because you were assigned.Message ID: ***@***.***>

[jasl]

Is that just for the _() version?
…
On Wed, Jan 25, 2023 at 5:49 AM Jun Jiang @.> wrote: Although #[extrinsic_call] (RawOrigin::Signed(caller.clone()), something, something); is valid syntax, but it looks very rare, and Clion warns #[extrinsic_call] has syntax error (expects '(', ';', , '[' or '}'，but got '') parhaps use macro here is more readdable and friendly for IDE? — Reply to this email directly, view it on GitHub <#13132 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOE4LLIIE7FMHK5GIKTMK3WUEAKHANCNFSM6AAAAAATZKNEKM . You are receiving this because you were assigned.Message ID: @.>

yes, given full name will resovle the syntax error.

[sam0x17]

yes, given full name will resovle the syntax error.

@ggwpez what do you think of this? I'm not particularly attached to the _() shortcut so I'd be fine with removing it, but I know you like it.

On the flipside, there are a number of places in the codebase already where things like rust-analyzer break.

[jasl]

Another issue is blank benchmark will make compiling error

#[benchmarks]
mod benchmarks {
	use super::*;

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

should I open an issue for this?

[ggwpez]

parhaps use macro here is more readdable and IDE-friendly?

On the flipside, there are a number of places in the codebase already where things like rust-analyzer break.

Yea, I dont think we should try to work around a the bugs of downstream tooling. This would only hinder our progress, but in this case the syntax is not very good to begin with.

I'm not particularly attached to the _() shortcut so I'd be fine with removing it, but I know you like it.

TBH I dont like it that much either… any better idea? Since we have the chance to throw out the garbage we should try to get the best syntax possible.

#[extrinsic_call(x, y, z)]

could also work, but maybe confusing as to what it is doing since otherwise its used as attribute macro.
We could also try self instead of _, or can we leave the _ away and just have the (args...)?
(cc @shawntabrizi in case you are reading this)

[ggwpez]

Another issue is blank benchmark will make compiling error

#[benchmarks]
mod benchmarks {
	use super::*;

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

should I open an issue for this?

What should that code do? It cannot implement tests without a benchmark. Guess we could fix that, but I dont see how it would be useful.

[sam0x17]

Yeah regarding the empty benchmark thing, it will correctly issue no errors in the case that you just do:

#[benchmarks]
mod benchmarks {
}

Beyond that, of course impl_benchmark_test_suite! won't work if there are no benchmarks...

[sam0x17]

@ggwpez

I really like self from the options you presented above

[bkchr]

#[extrinsic_call(x, y, z)]

This is probably much longer than the actual name of the function we are benchmarking. What I want to say with this is that the _ syntax doesn't bring any advantage when we need this attribute macro.

In general I have to say that the current syntax is a little bit less intuitive than the previous syntax. Currently everything is written in one context, making it look like one big function. I think the previous approach of having different "stages" made it much more obvious. But I don't have any proposal for a better syntax at hand 🙈

[jasl]

Another issue is blank benchmark will make compiling error

#[benchmarks]
mod benchmarks {
	use super::*;

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

should I open an issue for this?

What should that code do? It cannot implement tests without a benchmark. Guess we could fix that, but I dont see how it would be useful.

the blank benchmarking sceanario is: say I'm working on a new pallet, its logic is WIP so I'm not start working on benchmarking, but I shall leave a skeleton at first

[jasl]

an off-topic warn: I'm test build Substrate with latest nightly rustc and got
warning: the following packages contain code that will be rejected by a future version of Rust: nalgebra v0.27.1
this dependency depends by linregress and linregress only depends by frame-benchmarking

[sam0x17]

This might be good to post as it’s own issue but good call out!

…

On Fri, Feb 3, 2023 at 6:13 PM Jun Jiang ***@***.***> wrote: an off-topic warn: I'm test build Substrate with latest nightly rustc and got warning: the following packages contain code that will be rejected by a future version of Rust: nalgebra v0.27.1 this dependency depends by linregress and linregress only depends by frame-benchmarking — Reply to this email directly, view it on GitHub <#380>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOE4LIX5BTWNLCUC2BJB7LWVWGKVANCNFSM6AAAAAATZKNEKM> . You are receiving this because you were assigned.Message ID: ***@***.***>

[ggwpez]

We can just update it i guess.

[sam0x17]

We can just update it i guess.

I'll make a PR

update: paritytech/substrate#13310

[bkchr]

#[extrinsic_call(x, y, z)]

This is probably much longer than the actual name of the function we are benchmarking. What I want to say with this is that the _ syntax doesn't bring any advantage when we need this attribute macro.

In general I have to say that the current syntax is a little bit less intuitive than the previous syntax. Currently everything is written in one context, making it look like one big function. I think the previous approach of having different "stages" made it much more obvious. But I don't have any proposal for a better syntax at hand see_no_evil

I thought about this again and came to the following syntax in my head:

#[benchmark]
fn my_call() {
     setup! {
          let a = something();
          let origin = Alice;
     };
     // Here we can support either `call!` when the name of the benchmark is equal to the call we want to execute or we would
    // also support something like custom! {} where you are totally free to put there in whatever logic you want as part of the benchmark.
     call!(origin, a);
     // This obviously being optional.
     verify! {}
}

This would give the entire function more structure and also clear intent on what is what etc. These macros obviously wouldn't be real macros, just something for us to parse. I also thought about creating a module and having each of these steps as a function in the module. The problem is the passing of arguments from the setup to the actual call. It would be even more "magic".

But yeah this here being just some proposal, maybe someone else comes up with something better based on my idea :)

[ggwpez]

#[benchmark]
fn my_call() {
     setup! {
          let a = something();
          let origin = Alice;
     };
     // Here we can support either `call!` when the name of the benchmark is equal to the call we want to execute or we would
    // also support something like custom! {} where you are totally free to put there in whatever logic you want as part of the benchmark.
     call!(origin, a);
     // This obviously being optional.
     verify! {}
}

Hm… In the beginning this could be useful, but for more advanced devs it will be annoying to explicitly mention these stages.
We could make an No-OP attribute macro:

#[benchmark]
fn my_call() {
     #[setup]
     {
          // setup logic
     }

     #[block]
     {
          // custom logic; this is already possible.
     ]

     #[verify]
     {
          // optional verification code...
     }
}

These attributes would just do nothing and be optional. Not sure if that improves the situation although it does add a bit more structure for new-comers.

[bkchr]

We could make an No-OP attribute macro:

Yeah they would also be no-op in my example above. I just wanted to raise, if you are happy with the current syntax, fine. I'm (who isn't that advanced in our benchmarking stuff) was quite confused by the old syntax and would be happy if we are being a little bit more expressive in the new one. I'm also not really convinced by my proposed syntax as it still doesn't "feel" right.

[jasl]

I like

#[benchmark]
fn my_call() {
     setup! {
          let a = something();
          let origin = Alice;
     };
     // Here we can support either `call!` when the name of the benchmark is equal to the call we want to execute or we would
    // also support something like custom! {} where you are totally free to put there in whatever logic you want as part of the benchmark.
     call!(origin, a);
     // This obviously being optional.
     verify! {}
}

this style because compared to attribute style, it's more compact, and _() style made CLion Rust unhappy, that's fine, but somewhat annoying...

I agree these macro should be optional, but using them could made complex benchmark more readable

[ggwpez]

The last two important issue to be feature complete have been done now. Going to close and otherwise re-open smaller issues.