Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
24cf315
Create a macro which automates creation of benchmark test suites.
coriolinus Feb 11, 2021
0209e4d
bump impl_version
coriolinus Feb 11, 2021
df14bba
allow unused on test_bench_by_name
coriolinus Feb 11, 2021
dacbe67
use proper doctest ignore attribute
coriolinus Feb 11, 2021
47c795f
Explicitly hand the Module to the test suite
coriolinus Feb 12, 2021
4b3be35
explicitly import what we need into `mod tests`
coriolinus Feb 12, 2021
9577ee7
bench_module is `ident` not `tt`
coriolinus Feb 12, 2021
a6cf5d4
allow end users to specify arguments for new_test_ext
coriolinus Feb 12, 2021
ebf64f0
enable explicitly specifying the path to the benchmarks invocation
coriolinus Feb 12, 2021
e0444d8
Revert "bump impl_version"
coriolinus Feb 12, 2021
cb6a33d
list failing benchmark tests and the errors which caused the failure
coriolinus Feb 12, 2021
fd71312
harden benchmark tests against internal panics
coriolinus Feb 12, 2021
7169e83
suppress warning about ignored profiles
coriolinus Feb 15, 2021
c382fb2
impl_benchmark_test_suite for assets
coriolinus Feb 15, 2021
bd39a02
impl_benchmark_test_suite for balances
coriolinus Feb 15, 2021
ebcf6d8
impl_benchmark_test_suite for bounties
coriolinus Feb 15, 2021
e0839c2
impl_benchmark_test_suite for Collective
coriolinus Feb 15, 2021
5e45e40
impl_benchmark_test_suite for Contracts
coriolinus Feb 15, 2021
5c9f586
impl_benchmark_test_suite for Democracy
coriolinus Feb 15, 2021
2730e07
don't impl_benchmark_test_suite for Elections-Phragmen
coriolinus Feb 15, 2021
4206191
impl_benchmark_test_suite for Identity
coriolinus Feb 15, 2021
9889fc9
impl_benchmark_test_suite for ImOnline
coriolinus Feb 15, 2021
10079b5
impl_benchmark_test_suite for indices
coriolinus Feb 15, 2021
ffa7693
impl_benchmark_test_suite for lottery
coriolinus Feb 15, 2021
01964c6
impl_benchmark_test_suite for merkle-mountain-range
coriolinus Feb 15, 2021
3ef76f3
impl_benchmark_test_suite for Multisig
coriolinus Feb 15, 2021
deb140f
impl_benchmark_test_suite for offences
coriolinus Feb 15, 2021
c154611
impl_benchmark_test_suite for Proxy
coriolinus Feb 15, 2021
f4cdd67
impl_benchmark_test_suite for scheduler
coriolinus Feb 15, 2021
a350c64
impl_benchmark_test_suite for session
coriolinus Feb 15, 2021
6efac34
impl_benchmark_test_suite for staking
coriolinus Feb 15, 2021
3e14cd2
impl_benchmark_test_suite for system
coriolinus Feb 15, 2021
681deaf
impl_benchmark_test_suite for timestamp
coriolinus Feb 15, 2021
4e03f34
impl_benchmark_test_suite for tips
coriolinus Feb 15, 2021
82f2a0d
impl_benchmark_test_suite for treasury
coriolinus Feb 15, 2021
7fcb460
impl_benchmark_test_suite for utility
coriolinus Feb 15, 2021
db62d9f
impl_benchmark_test_suite for vesting
coriolinus Feb 15, 2021
5341906
fix wrong module name in impl_benchmark_test_suite in Offences
coriolinus Feb 15, 2021
1732671
address line length nits
coriolinus Feb 15, 2021
f1859ee
enable optional keyword argument: exec_name
coriolinus Feb 15, 2021
f3d2f8f
get rid of dead code
coriolinus Feb 15, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Create a macro which automates creation of benchmark test suites.
  • Loading branch information
coriolinus committed Feb 11, 2021
commit 24cf315123fb7dc8c6b01918a926738044e6c9a6
103 changes: 101 additions & 2 deletions frame/benchmarking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,26 @@ 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.
#[cfg(test)]
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."),
}
}
};
}

Expand Down Expand Up @@ -903,6 +923,85 @@ macro_rules! impl_benchmark_test {
};
}

/// This creates a test suite which runs the module's benchmarks.
///
/// When called in [`pallet_example`] as
///
/// ```rust,ignored
/// impl_benchmark_test_suite!(crate::tests::new_test_ext, crate::tests::Test);
/// ```
///
/// It expands to the equivalent of:
///
/// ```rust,ignored
/// #[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, `new_test_ext`, must be the path to a function which takes no arguments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking no arguments is a bad assumption for this function unfortunately.

Take a look at here:

https://github.com/paritytech/substrate/blob/master/frame/babe/src/benchmarking.rs#L76

One thing that will help you figure out if this PR satisfies the needs we have is to replace all of the benchmark test instances with your new macro and confirm things compile. You will have to be careful of such customization instances (although any really heavy customization we can just say is not supported by the macro and should be implemented manually)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a6cf5d4 should resolve this.

/// and returns either a `sp_io::TestExternalities`, or some other type with an identical interface.
///
/// The second argument, `test`, must be the path to the runtime. The item to which this must refer
/// will generally take the form:
///
/// ```rust,ignored
/// 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 {
($new_test_ext:path, $test:path) => {
#[cfg(test)]
mod tests {
use super::*;
use $crate::frame_support::assert_ok;

#[test]
fn test_benchmarks() {
$new_test_ext().execute_with(|| {
use $crate::Benchmarking;
for benchmark_name in Module::<$test>::benchmarks(true) {
assert_ok!(test_bench_by_name::<$test>(benchmark_name));
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is critical for debugging during benchmark development.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer we actually generate N tests in this macro where each benchmark gets its own entire function

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 test_bench_by_name function at all.

I'll think some more about how to accomplish this.

Copy link
Contributor

Choose a reason for hiding this comment

The 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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No: a failing assertion within test_bench_by_name still fails the single test without testing anything else, or propagating information about which test failed.

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, cargo test will just swallow stdout.

However, doing this properly involves building the functionality of impl_benchmark_test_suite directly into the benchmarks macro; there just isn't another way that I can see to generate individual test cases without requiring the user to explicitly enumerate the generated test functions. Obviously, explicitly enumerating those is worse than the status quo: it retains the boilerplate, but makes it less clear what is being accomplished.

If I have to substantially modify the benchmarks macro, I'd like to rewrite it as a procedural macro, because that's just much easier to understand than some 900 lines of interlinked recursive example macros as currently exist. However, that feels like kind of an enormous job. I can say that it'll take me a fair while to complete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I start on a huge project rewriting the benchmarks macro, what do you think of cb6a33d? It fails at the goal of generating a single test case per benchmark test, but it succeeds at the goals of running all the tests, and providing a list of failing tests, and what the failures were.

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
For them maybe we can print the name of the test before its exeuction

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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(
Expand Down Expand Up @@ -1031,7 +1130,7 @@ macro_rules! add_benchmark {
*repeat,
whitelist,
*verify,
).map_err(|e| {
).map_err(|e| {
$crate::show_benchmark_debug_info(
instance_string,
benchmark,
Expand All @@ -1058,7 +1157,7 @@ macro_rules! add_benchmark {
*repeat,
whitelist,
*verify,
).map_err(|e| {
).map_err(|e| {
$crate::show_benchmark_debug_info(
instance_string,
benchmark,
Expand Down
21 changes: 3 additions & 18 deletions frame/example/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ impl<T: Config> Module<T> {
//
// Note that a signed extension can also indicate that a particular data must be present in the
// _signing payload_ of a transaction by providing an implementation for the `additional_signed`
// method. This example will not cover this type of extension. See `CheckSpecVersion` in
// method. This example will not cover this type of extension. See `CheckSpecVersion` in
// [FRAME System](https://github.com/paritytech/substrate/tree/master/frame/system#signed-extensions)
// for an example.
//
Expand Down Expand Up @@ -652,7 +652,7 @@ where
#[cfg(feature = "runtime-benchmarks")]
mod benchmarking {
use super::*;
use frame_benchmarking::{benchmarks, account};
use frame_benchmarking::{benchmarks, account, impl_benchmark_test_suite};
use frame_system::RawOrigin;

benchmarks!{
Expand Down Expand Up @@ -684,22 +684,7 @@ mod benchmarking {
}
}

#[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>());
});
}
}
impl_benchmark_test_suite!(crate::tests::new_test_ext, crate::tests::Test);
}

#[cfg(test)]
Expand Down