Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Benchmarks 'how-to guide' uses outdated syntax #1355

Closed
Wiezzel opened this issue Aug 3, 2022 · 3 comments · Fixed by #1378
Closed

Benchmarks 'how-to guide' uses outdated syntax #1355

Wiezzel opened this issue Aug 3, 2022 · 3 comments · Fixed by #1378

Comments

@Wiezzel
Copy link

Wiezzel commented Aug 3, 2022

In am referring to this guide. In section 2. Add benchmarking to your runtime, point 3. says:

Then, in addition to your normal runtime configuration, you also need to update the benchmarking section of your runtime. To add our new benchmarks, we simply add a new line with the add_benchmark! macro

As far as I know, this syntax (add_benchmark!) has been deprecated in favour of a more convenient and easier define_benchmarks! in this PR. I'd like to request updating this section of the guide to use the new syntax.

@lisa-parity
Copy link
Contributor

@ggwpez , Can you provide some guidance on what to change?
It seems like the add_benchmark! macro is still valid based on this:
https://paritytech.github.io/substrate/master/frame_benchmarking/index.html
and
Adding Benchmarks
The benchmarks included with each pallet are not automatically added to your node. To actually execute these benchmarks, you need to implement the frame_benchmarking::Benchmark trait. You can see an example of how to do this in the included Substrate node.

Assuming there are already some benchmarks set up on your node, you just need to add another instance of the add_benchmark! macro:

/// configuration for running benchmarks
/// | name of your pallet's crate (as imported)
/// v v
add_benchmark!(params, batches, pallet_balances, Balances);
/// ^ ^
/// where all benchmark results are saved |
/// the struct created for your pallet by construct_runtime!

I wouldn't be at all surprised that this doc is out-of-date, but the How-to guide is pretty thin on details so I'm not sure what needs to change (well, actually, it probably needs to be completely rewritten at some point ;-D).

@ggwpez
Copy link
Contributor

ggwpez commented Aug 8, 2022

Thanks for spotting it!
The old syntax was in the form of:

add_benchmark!(params, batches, frame_benchmarking, BaselineBench::<Runtime>);
add_benchmark!(params, batches, pallet_assets, Assets);
add_benchmark!(params, batches, pallet_babe, Babe);

and then in another function, one had to write the same thing again which is error prone:

list_benchmark!(list, extra, frame_benchmarking, BaselineBench::<Runtime>);
list_benchmark!(list, extra, pallet_assets, Assets);
list_benchmark!(list, extra, pallet_babe, Babe);

these two got unified into define_benchmarks! which allows a developer to only write it once.

define_benchmarks!(
	[frame_benchmarking, BaselineBench::<Runtime>]
	[pallet_assets, Assets]
	[pallet_babe, Babe]);

This can be seen here (and following lines). The old syntax is still valid and it is possible to mix the old and new syntax, but I would suggest to only mention the new one.
The developer now only has to add the new benchmark to the define_benchmarks! macro invocation.
In case an alias is needed, they still need to be defined twice eg. here and here.

Looks like this article also mentions the old syntax.

@lisa-parity
Copy link
Contributor

Thanks, @ggwpez ! I took a stab at updating the guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants