-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Create a macro which automates creation of benchmark test suites. #8104
Changes from 6 commits
24cf315
0209e4d
df14bba
dacbe67
47c795f
4b3be35
9577ee7
a6cf5d4
ebf64f0
e0444d8
cb6a33d
fd71312
7169e83
c382fb2
bd39a02
ebcf6d8
e0839c2
5e45e40
5c9f586
2730e07
4206191
9889fc9
10079b5
ffa7693
01964c6
3ef76f3
deb140f
c154611
f4cdd67
a350c64
6efac34
3e14cd2
681deaf
4e03f34
82f2a0d
7fcb460
db62d9f
5341906
1732671
f1859ee
f3d2f8f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -830,6 +830,31 @@ macro_rules! impl_benchmark { | |||||
| return Ok(results); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| /// Test a particular benchmark by name. | ||||||
| /// | ||||||
| /// This isn't called `test_benchmark_by_name` just in case some end-user eventually | ||||||
| /// writes a benchmark, itself called `by_name`; the function would be shadowed in | ||||||
| /// that case. | ||||||
| /// | ||||||
| /// This is generally intended to be used by child test modules such as those created | ||||||
| /// by the `impl_benchmark_test_suite` macro. However, it is not an error if a pallet | ||||||
| /// author chooses not to implement benchmarks. | ||||||
| #[cfg(test)] | ||||||
| #[allow(unused)] | ||||||
| fn test_bench_by_name<T>(name: &[u8]) -> Result<(), &'static str> | ||||||
| where | ||||||
| T: Config + frame_system::Config, $( $where_clause )* | ||||||
| { | ||||||
| let name = sp_std::str::from_utf8(name) | ||||||
| .map_err(|_| "`name` is not a valid utf8 string!")?; | ||||||
| match name { | ||||||
| $( stringify!($name) => { | ||||||
| $crate::paste::paste! { [< test_benchmark_ $name >]::<T>() } | ||||||
| } )* | ||||||
| _ => Err("Could not find test for requested benchmark."), | ||||||
| } | ||||||
| } | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -903,6 +928,87 @@ macro_rules! impl_benchmark_test { | |||||
| }; | ||||||
| } | ||||||
|
|
||||||
| /// This creates a test suite which runs the module's benchmarks. | ||||||
| /// | ||||||
| /// When called in [`pallet_example`] as | ||||||
| /// | ||||||
| /// ```rust,ignore | ||||||
| /// impl_benchmark_test_suite!(Module, crate::tests::new_test_ext, crate::tests::Test); | ||||||
| /// ``` | ||||||
| /// | ||||||
| /// It expands to the equivalent of: | ||||||
| /// | ||||||
| /// ```rust,ignore | ||||||
| /// #[cfg(test)] | ||||||
| /// mod tests { | ||||||
| /// use super::*; | ||||||
| /// use crate::tests::{new_test_ext, Test}; | ||||||
| /// use frame_support::assert_ok; | ||||||
| /// | ||||||
| /// #[test] | ||||||
| /// fn test_benchmarks() { | ||||||
| /// new_test_ext().execute_with(|| { | ||||||
| /// assert_ok!(test_benchmark_accumulate_dummy::<Test>()); | ||||||
| /// assert_ok!(test_benchmark_set_dummy::<Test>()); | ||||||
| /// assert_ok!(test_benchmark_another_set_dummy::<Test>()); | ||||||
| /// assert_ok!(test_benchmark_sort_vector::<Test>()); | ||||||
| /// }); | ||||||
| /// } | ||||||
| /// } | ||||||
| /// ``` | ||||||
| /// | ||||||
| /// ## Arguments | ||||||
| /// | ||||||
| /// The first argument, `module`, must be the path to this crate's module. | ||||||
| /// | ||||||
| /// The second argument, `new_test_ext`, must be the path to a function which takes no arguments | ||||||
| /// and returns either a `sp_io::TestExternalities`, or some other type with an identical interface. | ||||||
| /// | ||||||
| /// The third argument, `test`, must be the path to the runtime. The item to which this must refer | ||||||
| /// will generally take the form: | ||||||
| /// | ||||||
| /// ```rust,ignore | ||||||
| /// frame_support::construct_runtime!( | ||||||
| /// pub enum Test where ... | ||||||
| /// { ... } | ||||||
| /// ); | ||||||
| /// ``` | ||||||
| /// | ||||||
| // ## Notes (not for rustdoc) | ||||||
| // | ||||||
| // The biggest challenge for this macro is communicating the actual test functions to be run. We | ||||||
| // can't just build an array of function pointers to each test function and iterate over it, because | ||||||
| // the test functions are parameterized by the `Test` type. That's incompatible with | ||||||
| // monomorphization: if it were legal, then even if the compiler detected and monomorphized the | ||||||
| // functions into only the types of the callers, which implementation would the function pointer | ||||||
| // point to? There would need to be some kind of syntax for selecting the destination of the pointer | ||||||
| // according to a generic argument, and in general it would be a huge mess and not worth it. | ||||||
| // | ||||||
| // Instead, we're going to steal a trick from `fn run_benchmark`: generate a function which is | ||||||
| // itself parametrized by `Test`, which accepts a `&[u8]` parameter containing the name of the | ||||||
| // benchmark, and dispatches based on that to the appropriate real test implementation. Then, we can | ||||||
| // just iterate over the `Benchmarking::benchmarks` list to run the actual implementations. | ||||||
| #[macro_export] | ||||||
| macro_rules! impl_benchmark_test_suite { | ||||||
| ($bench_module:tt, $new_test_ext:path, $test:path) => { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Maybe we can use ident instead of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's precisely why we can't use
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not moving to procedural ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My main objection to procedural macros is that they're a heavy hammer: they take much longer to develop (at my level of experience) than macros-by-example. There is also some weirdness about requiring their own specially-configured crate. It doesn't feel worth it for a macro as small as However, as I wrote in #8104 (comment), the only way I can see to get a single test case per benchmark is to merge the functionality into the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My 2 cents is that once you get the initial scaffolding there, proc-marcos are well worth the extra time in the long run. For this use case, I'd admit that I would personally go with them, but can totally agree with that it is simpler to start with a simple |
||||||
| #[cfg(test)] | ||||||
| mod tests { | ||||||
| use super::{test_bench_by_name, $bench_module}; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, I'm not sure that we want to make this explicit: the function is ultimately generated by the What do you think of this: we could add an optional parameter
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. another idea would be to move
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ebf64f0 has what I was talking about. I think that's more usable than requiring the end-user to specify a generated function. |
||||||
| use $crate::frame_support::assert_ok; | ||||||
|
|
||||||
| #[test] | ||||||
| fn test_benchmarks() { | ||||||
| $new_test_ext().execute_with(|| { | ||||||
| use $crate::Benchmarking; | ||||||
| for benchmark_name in $bench_module ::<$test>::benchmarks(true) { | ||||||
| assert_ok!(test_bench_by_name::<$test>(benchmark_name)); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am concerned with this approach if one of the benchmarks fails, it will be hard to identify which benchmark is the issue. I would prefer we actually generate N tests in this macro where each benchmark gets its own entire function, and thus when it fails, it is easy to identify which test failed.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is critical for debugging during benchmark development.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd prefer that too. Unfortunately, it's hard: we don't have a simple way to iterate over the names of the benchmarks within the macro scope which generates the test module, which is why we introduce the I'll think some more about how to accomplish this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. moving the assert to inside test_bench_by_name should be equivalent to what was before no ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No: a failing assertion within A short-term hack for identifying failing benchmarks would be to simply print the current benchmark to stdout: you can identify the failing benchmark because its name will be the last printed. On success, However, doing this properly involves building the functionality of If I have to substantially modify the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before I start on a huge project rewriting the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it is hard for me to tell without a UI test, but I can be fine with it
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm actually a lot of code in benchmark would simply do assert and unwrap instead of returning error.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. fd71312 |
||||||
| } | ||||||
| }); | ||||||
| } | ||||||
| } | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| /// show error message and debugging info for the case of an error happening | ||||||
| /// during a benchmark | ||||||
| pub fn show_benchmark_debug_info( | ||||||
|
|
@@ -1031,7 +1137,7 @@ macro_rules! add_benchmark { | |||||
| *repeat, | ||||||
| whitelist, | ||||||
| *verify, | ||||||
| ).map_err(|e| { | ||||||
| ).map_err(|e| { | ||||||
| $crate::show_benchmark_debug_info( | ||||||
| instance_string, | ||||||
| benchmark, | ||||||
|
|
@@ -1058,7 +1164,7 @@ macro_rules! add_benchmark { | |||||
| *repeat, | ||||||
| whitelist, | ||||||
| *verify, | ||||||
| ).map_err(|e| { | ||||||
| ).map_err(|e| { | ||||||
| $crate::show_benchmark_debug_info( | ||||||
| instance_string, | ||||||
| benchmark, | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.