Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Enable verification logic when executing benchmarks #6929

Merged
merged 12 commits into from
Aug 24, 2020
Merged

Enable verification logic when executing benchmarks #6929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d5adfdc
Add `--verify` flag to benchmark execution
shawntabrizi Aug 21, 2020
09f6026
make it so `--verify` can be used for getting the actual benchmarks
shawntabrizi Aug 21, 2020
c4627a0
undo manual testing
shawntabrizi Aug 21, 2020
1f22e84
oops
shawntabrizi Aug 21, 2020
a7c22c1
use benchmark config struct
shawntabrizi Aug 21, 2020
1d693eb
verify is default on, docs update
shawntabrizi Aug 21, 2020
d6bc32a
remove clone
shawntabrizi Aug 21, 2020
f662a4c
improve formatting
shawntabrizi Aug 21, 2020
4dc13c8
fix test
shawntabrizi Aug 21, 2020
137d39e
Merge branch 'master' into shawntabrizi-run-benchmark-verify
shawntabrizi Aug 21, 2020
1c63e33
bump impl for ci
shawntabrizi Aug 21, 2020
42affc3
Merge branch 'master' into shawntabrizi-run-benchmark-verify
shawntabrizi Aug 24, 2020
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
10 changes: 2 additions & 8 deletions bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1134,13 +1134,7 @@ impl_runtime_apis! {
#[cfg(feature = "runtime-benchmarks")]
impl frame_benchmarking::Benchmark<Block> for Runtime {
fn dispatch_benchmark(
pallet: Vec<u8>,
benchmark: Vec<u8>,
lowest_range_values: Vec<u32>,
highest_range_values: Vec<u32>,
steps: Vec<u32>,
repeat: u32,
extra: bool,
config: frame_benchmarking::BenchmarkConfig
) -> Result<Vec<frame_benchmarking::BenchmarkBatch>, sp_runtime::RuntimeString> {
use frame_benchmarking::{Benchmarking, BenchmarkBatch, add_benchmark, TrackedStorageKey};
// Trying to add benchmarks directly to the Session Pallet caused cyclic dependency issues.
Expand Down Expand Up @@ -1170,7 +1164,7 @@ impl_runtime_apis! {
];

let mut batches = Vec::<BenchmarkBatch>::new();
let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist, extra);
let params = (&config, &whitelist);

add_benchmark!(params, batches, pallet_babe, Babe);
add_benchmark!(params, batches, pallet_balances, Balances);
Expand Down
179 changes: 92 additions & 87 deletions frame/benchmarking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,30 +648,11 @@ macro_rules! benchmark_backend {
]
}

fn instance(&self, components: &[($crate::BenchmarkParameter, u32)])
-> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>
{
$(
let $common = $common_from;
)*
$(
// Prepare instance
let $param = components.iter()
.find(|&c| c.0 == $crate::BenchmarkParameter::$param)
.unwrap().1;
)*
$(
let $pre_id : $pre_ty = $pre_ex;
)*
$( $param_instancer ; )*
$( $post )*

Ok(Box::new(move || -> Result<(), &'static str> { $eval; Ok(()) }))
}

fn verify(&self, components: &[($crate::BenchmarkParameter, u32)])
-> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>
{
fn instance(
&self,
components: &[($crate::BenchmarkParameter, u32)],
verify: bool
) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str> {
$(
let $common = $common_from;
)*
Expand All @@ -687,7 +668,13 @@ macro_rules! benchmark_backend {
$( $param_instancer ; )*
$( $post )*

Ok(Box::new(move || -> Result<(), &'static str> { $eval; $postcode; Ok(()) }))
Ok(Box::new(move || -> Result<(), &'static str> {
$eval;
if verify {
$postcode;
}
Ok(())
}))
}
}
};
Expand Down Expand Up @@ -736,26 +723,16 @@ macro_rules! selected_benchmark {
}
}

fn instance(&self, components: &[($crate::BenchmarkParameter, u32)])
-> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>
{
match self {
$(
Self::$bench => <
$bench as $crate::BenchmarkingSetup<T $(, $bench_inst)? >
>::instance(&$bench, components),
)*
}
}

fn verify(&self, components: &[($crate::BenchmarkParameter, u32)])
-> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>
{
fn instance(
&self,
components: &[($crate::BenchmarkParameter, u32)],
verify: bool
) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str> {
match self {
$(
Self::$bench => <
$bench as $crate::BenchmarkingSetup<T $(, $bench_inst)? >
>::verify(&$bench, components),
>::instance(&$bench, components, verify),
)*
}
}
Expand Down Expand Up @@ -791,7 +768,8 @@ macro_rules! impl_benchmark {
highest_range_values: &[u32],
steps: &[u32],
repeat: u32,
whitelist: &[$crate::TrackedStorageKey]
whitelist: &[$crate::TrackedStorageKey],
verify: bool,
) -> Result<Vec<$crate::BenchmarkResults>, &'static str> {
// Map the input to the selected benchmark.
let extrinsic = sp_std::str::from_utf8(extrinsic)
Expand Down Expand Up @@ -826,14 +804,15 @@ macro_rules! impl_benchmark {
repeat: u32,
c: Vec<($crate::BenchmarkParameter, u32)>,
results: &mut Vec<$crate::BenchmarkResults>,
verify: bool,
| -> Result<(), &'static str> {
// Run the benchmark `repeat` times.
for _ in 0..repeat {
// Set up the externalities environment for the setup we want to
// benchmark.
let closure_to_benchmark = <
SelectedBenchmark as $crate::BenchmarkingSetup<T $(, $instance)?>
>::instance(&selected_benchmark, &c)?;
>::instance(&selected_benchmark, &c, verify)?;

// Set the block number to at least 1 so events are deposited.
if $crate::Zero::is_zero(&frame_system::Module::<T>::block_number()) {
Expand All @@ -847,43 +826,49 @@ macro_rules! impl_benchmark {
// Reset the read/write counter so we don't count operations in the setup process.
$crate::benchmarking::reset_read_write_count();

// Time the extrinsic logic.
frame_support::debug::trace!(
target: "benchmark",
"Start Benchmark: {:?}", c
);
if verify {
closure_to_benchmark()?;
} else {
// Time the extrinsic logic.
frame_support::debug::trace!(
target: "benchmark",
"Start Benchmark: {:?}", c
);

let start_extrinsic = $crate::benchmarking::current_time();
closure_to_benchmark()?;
let finish_extrinsic = $crate::benchmarking::current_time();
let elapsed_extrinsic = finish_extrinsic - start_extrinsic;
// Commit the changes to get proper write count
$crate::benchmarking::commit_db();
frame_support::debug::trace!(
target: "benchmark",
"End Benchmark: {} ns", elapsed_extrinsic
);
let read_write_count = $crate::benchmarking::read_write_count();
frame_support::debug::trace!(
target: "benchmark",
"Read/Write Count {:?}", read_write_count
);
let start_extrinsic = $crate::benchmarking::current_time();

// Time the storage root recalculation.
let start_storage_root = $crate::benchmarking::current_time();
$crate::storage_root();
let finish_storage_root = $crate::benchmarking::current_time();
let elapsed_storage_root = finish_storage_root - start_storage_root;
closure_to_benchmark()?;

results.push($crate::BenchmarkResults {
components: c.clone(),
extrinsic_time: elapsed_extrinsic,
storage_root_time: elapsed_storage_root,
reads: read_write_count.0,
repeat_reads: read_write_count.1,
writes: read_write_count.2,
repeat_writes: read_write_count.3,
});
let finish_extrinsic = $crate::benchmarking::current_time();
let elapsed_extrinsic = finish_extrinsic - start_extrinsic;
// Commit the changes to get proper write count
$crate::benchmarking::commit_db();
frame_support::debug::trace!(
target: "benchmark",
"End Benchmark: {} ns", elapsed_extrinsic
);
let read_write_count = $crate::benchmarking::read_write_count();
frame_support::debug::trace!(
target: "benchmark",
"Read/Write Count {:?}", read_write_count
);

// Time the storage root recalculation.
let start_storage_root = $crate::benchmarking::current_time();
$crate::storage_root();
let finish_storage_root = $crate::benchmarking::current_time();
let elapsed_storage_root = finish_storage_root - start_storage_root;

results.push($crate::BenchmarkResults {
components: c.clone(),
extrinsic_time: elapsed_extrinsic,
storage_root_time: elapsed_storage_root,
reads: read_write_count.0,
repeat_reads: read_write_count.1,
writes: read_write_count.2,
repeat_writes: read_write_count.3,
});
}

// Wipe the DB back to the genesis state.
$crate::benchmarking::wipe_db();
Expand All @@ -893,7 +878,11 @@ macro_rules! impl_benchmark {
};

if components.is_empty() {
repeat_benchmark(repeat, Default::default(), &mut results)?;
if verify {
// If `--verify` is used, run the benchmark once to verify it would complete.
repeat_benchmark(1, Default::default(), &mut Vec::new(), true)?;
}
repeat_benchmark(repeat, Default::default(), &mut results, false)?;
} else {
// Select the component we will be benchmarking. Each component will be benchmarked.
for (idx, (name, low, high)) in components.iter().enumerate() {
Expand Down Expand Up @@ -929,7 +918,11 @@ macro_rules! impl_benchmark {
)
.collect();

repeat_benchmark(repeat, c, &mut results)?;
if verify {
// If `--verify` is used, run the benchmark once to verify it would complete.
repeat_benchmark(1, Default::default(), &mut Vec::new(), true)?;
}
repeat_benchmark(repeat, c, &mut results, false)?;
}
}
}
Expand Down Expand Up @@ -962,17 +955,17 @@ macro_rules! impl_benchmark_test {
let execute_benchmark = |
c: Vec<($crate::BenchmarkParameter, u32)>
| -> Result<(), &'static str> {
// Set up the verification state
// Set up the benchmark, return execution + verification function.
let closure_to_verify = <
SelectedBenchmark as $crate::BenchmarkingSetup<T, _>
>::verify(&selected_benchmark, &c)?;
>::instance(&selected_benchmark, &c, true)?;

// Set the block number to at least 1 so events are deposited.
if $crate::Zero::is_zero(&frame_system::Module::<T>::block_number()) {
frame_system::Module::<T>::set_block_number(1.into());
}

// Run verification
// Run execution + verification
closure_to_verify()?;

// Reset the state
Expand Down Expand Up @@ -1015,7 +1008,7 @@ macro_rules! impl_benchmark_test {
/// First create an object that holds in the input parameters for the benchmark:
///
/// ```ignore
/// let params = (&pallet, &benchmark, &lowest_range_values, &highest_range_values, &steps, repeat, &whitelist);
/// let params = (&config, &whitelist);
/// ```
///
/// The `whitelist` is a parameter you pass to control the DB read/write tracking.
Expand Down Expand Up @@ -1059,18 +1052,29 @@ macro_rules! impl_benchmark_test {
macro_rules! add_benchmark {
( $params:ident, $batches:ident, $name:ident, $( $location:tt )* ) => (
let name_string = stringify!($name).as_bytes();
let (pallet, benchmark, lowest_range_values, highest_range_values, steps, repeat, whitelist, extra) = $params;
let (config, whitelist) = $params;
let $crate::BenchmarkConfig {
pallet,
benchmark,
lowest_range_values,
highest_range_values,
steps,
repeat,
verify,
extra,
} = config;
if &pallet[..] == &name_string[..] || &pallet[..] == &b"*"[..] {
if &pallet[..] == &b"*"[..] || &benchmark[..] == &b"*"[..] {
for benchmark in $( $location )*::benchmarks(extra).into_iter() {
for benchmark in $( $location )*::benchmarks(*extra).into_iter() {
$batches.push($crate::BenchmarkBatch {
results: $( $location )*::run_benchmark(
benchmark,
&lowest_range_values[..],
&highest_range_values[..],
&steps[..],
repeat,
*repeat,
whitelist,
*verify,
)?,
pallet: name_string.to_vec(),
benchmark: benchmark.to_vec(),
Expand All @@ -1083,8 +1087,9 @@ macro_rules! add_benchmark {
&lowest_range_values[..],
&highest_range_values[..],
&steps[..],
repeat,
*repeat,
whitelist,
*verify,
)?,
pallet: name_string.to_vec(),
benchmark: benchmark.clone(),
Expand Down
23 changes: 20 additions & 3 deletions frame/benchmarking/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,10 +176,11 @@ fn benchmarks_macro_works() {
let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
&selected,
&[(BenchmarkParameter::b, 1)],
true,
).expect("failed to create closure");

new_test_ext().execute_with(|| {
assert_eq!(closure(), Ok(()));
assert_ok!(closure());
});
}

Expand All @@ -193,6 +194,7 @@ fn benchmarks_macro_rename_works() {
let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
&selected,
&[(BenchmarkParameter::b, 1)],
true,
).expect("failed to create closure");

new_test_ext().execute_with(|| {
Expand All @@ -210,24 +212,39 @@ fn benchmarks_macro_works_for_non_dispatchable() {
let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
&selected,
&[(BenchmarkParameter::x, 1)],
true,
).expect("failed to create closure");

assert_eq!(closure(), Ok(()));
assert_ok!(closure());
}

#[test]
fn benchmarks_macro_verify_works() {
// Check postcondition for benchmark `set_value` is valid.
let selected = SelectedBenchmark::set_value;

let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::verify(
let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
&selected,
&[(BenchmarkParameter::b, 1)],
true,
).expect("failed to create closure");

new_test_ext().execute_with(|| {
assert_ok!(closure());
});

// Check postcondition for benchmark `bad_verify` is invalid.
let selected = SelectedBenchmark::bad_verify;

let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
&selected,
&[(BenchmarkParameter::x, 10000)],
true,
).expect("failed to create closure");

new_test_ext().execute_with(|| {
assert_err!(closure(), "You forgot to sort!");
});
}

#[test]
Expand Down
Loading