Improve usability of add+list_benchmark!

[ggwpez]

Problem

The usage of the add_benchmark and list_benchmark macros is error prone since they require code duplication.
Related paritytech/polkadot-sdk#397

Current Syntax

impl Benchmark<Block> for Runtime {
	fn benchmark_metadata(…)  {
		add_benchmark!(list, extra, path1, location1);
		...
		add_benchmark!(list, extra, pathN, locationN);
	}

	fn dispatch_benchmark(…)  {
		list_benchmark!(params, batches, path1, location1);
		// error prone repetition
		list_benchmark!(params, batches, pathN, locationN);
	}
}

Possible Solution

This MR contains a non-destructive way of extending the syntax to eliminate the repetition.
Added:

• define_benchmarks!: takes a list of bench configs that was formerly passed to add_+list_benchmark!
• list_benchmarks!: lists all benchmarks defined in define_benchmarks!
• add_benchmarks!: adds all benchmarks defined in define_benchmarks!

New Syntax

define_benchmarks!(
	[path1, location1]
	...
	[pathN, locationN]
);

impl_runtime_apis! {
	impl Benchmark<Block> for Runtime {
		fn benchmark_metadata(…)  {
			add_benchmarks!(list, extra);
		}

		fn dispatch_benchmark(…)  {
			list_benchmarks!(params, batches);
		}
	}
}

@ reviewers

• I tried to label the MR, D-* label is currently missing.
• There is a bit of boilerplate code before the define_benchmarks macro call, which could be moved into it but I am not sure if this is good style.
• Please take a look at the syntax and naming. I was not sure if the [path, location] … is the best syntax.
• I checked that the benchmarks to run are the same as on master via: cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --pallet='*' --extrinsic='*' --list and the same for the node-template.

[shawntabrizi]

This looks good in general, but I would prefer a backwards compatible PR, rather than forcing everyone into this way of things. That probably means just keeping the existing macro as is, and changing the name of the new macro you created to something like auto_list_benchmark!. Otherwise, seems good to me, and something I would def use in Substrate / Polkadot.

[ggwpez]

This looks good in general, but I would prefer a backwards compatible PR, rather than forcing everyone into this way of things.

It is backwards-compatible 😄 but the naming is a bit sneaky.
add_benchmark vs add_benchmarks: note the suffix s. Should I still rename it to auto_list_benchmark?

Also what do you think about the boilerplate code, should I put it inside the macro or not? This specifically:

#[macro_use]
extern crate frame_benchmarking;

#[cfg(feature = "runtime-benchmarks")]
mod benches {
}

@shawntabrizi

[kianenigma]

I thought this syntax (extern crate) was not needed anymore?

[ggwpez]

I could do this, but then we would have to import the "hidden" cb_add_benchmarks and cb_list_benchmarks macros, which is more confusing I think since the user does not directly need them.

[kianenigma]

LGTM, just have a few small questions.

[kianenigma]

This looks good in general, but I would prefer a backwards compatible PR, rather than forcing everyone into this way of things.

It is backwards-compatible 😄 but the naming is a bit sneaky. add_benchmark vs add_benchmarks: note the suffix s. Should I still rename it to auto_list_benchmark?

Also what do you think about the boilerplate code, should I put it inside the macro or not? This specifically:

#[macro_use]
extern crate frame_benchmarking;

#[cfg(feature = "runtime-benchmarks")]
mod benches {
}

@shawntabrizi

More a fan of having it inside the impl_runtime_api macro to keep all the benchmark related stuff under one roof.

[ggwpez]

More a fan of having it inside the impl_runtime_api macro to keep all the benchmark related stuff under one roof.

Yes I tried this. The impl_runtime_apis! only accept impl ... syntax.
So either I also change the impl_runtime_apis macro or it wont work.

This MR is only a temporary solution until we have automatic bench-case extraction from the pallet metadata, so I think it is ok.

[kianenigma]

Okay, still LGTM!

[ggwpez]

bot merge

[shawntabrizi]

@ggwpez please do the same in Polkadot and Cumulus?