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

Commit

Permalink
Custom Benchmark Errors and Override (#9517)
Browse files Browse the repository at this point in the history 
* initial idea

* update benchmark test to frame v2

* fix some errors

* fixes for elec phrag

* fix tests

* update extrinsic time and docs

* fix import

* undo extra changes

* helper function

* wrong way

* Update frame/benchmarking/src/utils.rs

Co-authored-by: Bastian Köcher <[email protected]>

* doesnt need encode/decode

* fix benchmark return

Co-authored-by: Bastian Köcher <[email protected]>
  • Loading branch information
@shawntabrizi @bkchr
shawntabrizi and bkchr authored Aug 19, 2021
1 parent 44ee839 commit 643818d
Show file tree
Hide file tree
Showing 13 changed files with 252 additions and 134 deletions.
14 changes: 7 additions & 7 deletions frame/benchmarking/src/analysis.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

//! Tools for analyzing the benchmark results.
use crate::BenchmarkResults;
use crate::BenchmarkResult;
use core::convert::TryFrom;
use linregress::{FormulaRegressionBuilder, RegressionDataBuilder};
use std::collections::BTreeMap;
Expand Down Expand Up @@ -76,7 +76,7 @@ impl TryFrom<Option<String>> for AnalysisChoice {
impl Analysis {
// Useful for when there are no components, and we just need an median value of the benchmark
// results. Note: We choose the median value because it is more robust to outliers.
fn median_value(r: &Vec<BenchmarkResults>, selector: BenchmarkSelector) -> Option<Self> {
fn median_value(r: &Vec<BenchmarkResult>, selector: BenchmarkSelector) -> Option<Self> {
if r.is_empty() {
return None
}
Expand Down Expand Up @@ -104,7 +104,7 @@ impl Analysis {
})
}

pub fn median_slopes(r: &Vec<BenchmarkResults>, selector: BenchmarkSelector) -> Option<Self> {
pub fn median_slopes(r: &Vec<BenchmarkResult>, selector: BenchmarkSelector) -> Option<Self> {
if r[0].components.is_empty() {
return Self::median_value(r, selector)
}
Expand Down Expand Up @@ -199,7 +199,7 @@ impl Analysis {
})
}

pub fn min_squares_iqr(r: &Vec<BenchmarkResults>, selector: BenchmarkSelector) -> Option<Self> {
pub fn min_squares_iqr(r: &Vec<BenchmarkResult>, selector: BenchmarkSelector) -> Option<Self> {
if r[0].components.is_empty() {
return Self::median_value(r, selector)
}
Expand Down Expand Up @@ -279,7 +279,7 @@ impl Analysis {
})
}

pub fn max(r: &Vec<BenchmarkResults>, selector: BenchmarkSelector) -> Option<Self> {
pub fn max(r: &Vec<BenchmarkResult>, selector: BenchmarkSelector) -> Option<Self> {
let median_slopes = Self::median_slopes(r, selector);
let min_squares = Self::min_squares_iqr(r, selector);

Expand Down Expand Up @@ -402,8 +402,8 @@ mod tests {
storage_root_time: u128,
reads: u32,
writes: u32,
) -> BenchmarkResults {
BenchmarkResults {
) -> BenchmarkResult {
BenchmarkResult {
components,
extrinsic_time,
storage_root_time,
Expand Down
80 changes: 51 additions & 29 deletions frame/benchmarking/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ macro_rules! benchmark_backend {
&self,
components: &[($crate::BenchmarkParameter, u32)],
verify: bool
) -> Result<$crate::Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str> {
) -> Result<$crate::Box<dyn FnOnce() -> Result<(), $crate::BenchmarkError>>, &'static str> {
$(
// Prepare instance
let $param = components.iter()
Expand All @@ -658,7 +658,7 @@ macro_rules! benchmark_backend {
$( $param_instancer ; )*
$( $post )*

Ok($crate::Box::new(move || -> Result<(), &'static str> {
Ok($crate::Box::new(move || -> Result<(), $crate::BenchmarkError> {
$eval;
if verify {
$postcode;
Expand Down Expand Up @@ -717,7 +717,7 @@ macro_rules! selected_benchmark {
&self,
components: &[($crate::BenchmarkParameter, u32)],
verify: bool
) -> Result<$crate::Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str> {
) -> Result<$crate::Box<dyn FnOnce() -> Result<(), $crate::BenchmarkError>>, &'static str> {
match self {
$(
Self::$bench => <
Expand All @@ -741,7 +741,7 @@ macro_rules! impl_benchmark {
( $( $name_skip_meta:ident ),* )
) => {
impl<T: Config $(<$instance>, $instance: $instance_bound )? >
$crate::Benchmarking<$crate::BenchmarkResults> for Pallet<T $(, $instance)? >
$crate::Benchmarking for Pallet<T $(, $instance)? >
where T: frame_system::Config, $( $where_clause )*
{
fn benchmarks(extra: bool) -> $crate::Vec<$crate::BenchmarkMetadata> {
Expand Down Expand Up @@ -772,13 +772,13 @@ macro_rules! impl_benchmark {
whitelist: &[$crate::TrackedStorageKey],
verify: bool,
internal_repeats: u32,
) -> Result<$crate::Vec<$crate::BenchmarkResults>, &'static str> {
) -> Result<$crate::Vec<$crate::BenchmarkResult>, $crate::BenchmarkError> {
// Map the input to the selected benchmark.
let extrinsic = $crate::sp_std::str::from_utf8(extrinsic)
.map_err(|_| "`extrinsic` is not a valid utf8 string!")?;
let selected_benchmark = match extrinsic {
$( stringify!($name) => SelectedBenchmark::$name, )*
_ => return Err("Could not find extrinsic."),
_ => return Err("Could not find extrinsic.".into()),
};

// Add whitelist to DB including whitelisted caller
Expand All @@ -790,7 +790,7 @@ macro_rules! impl_benchmark {
whitelist.push(whitelisted_caller_key.into());
$crate::benchmarking::set_whitelist(whitelist);

let mut results: $crate::Vec<$crate::BenchmarkResults> = $crate::Vec::new();
let mut results: $crate::Vec<$crate::BenchmarkResult> = $crate::Vec::new();

// Always do at least one internal repeat...
for _ in 0 .. internal_repeats.max(1) {
Expand Down Expand Up @@ -852,13 +852,13 @@ macro_rules! impl_benchmark {
let elapsed_storage_root = finish_storage_root - start_storage_root;

let skip_meta = [ $( stringify!($name_skip_meta).as_ref() ),* ];
let read_and_written_keys = if (&skip_meta).contains(&extrinsic) {
let read_and_written_keys = if skip_meta.contains(&extrinsic) {
$crate::vec![(b"Skipped Metadata".to_vec(), 0, 0, false)]
} else {
$crate::benchmarking::get_read_and_written_keys()
};

results.push($crate::BenchmarkResults {
results.push($crate::BenchmarkResult {
components: c.to_vec(),
extrinsic_time: elapsed_extrinsic,
storage_root_time: elapsed_storage_root,
Expand Down Expand Up @@ -893,14 +893,14 @@ macro_rules! impl_benchmark {
/// by the `impl_benchmark_test_suite` macro. However, it is not an error if a pallet
/// author chooses not to implement benchmarks.
#[allow(unused)]
fn test_bench_by_name(name: &[u8]) -> Result<(), &'static str> {
fn test_bench_by_name(name: &[u8]) -> Result<(), $crate::BenchmarkError> {
let name = $crate::sp_std::str::from_utf8(name)
.map_err(|_| "`name` is not a valid utf8 string!")?;
.map_err(|_| -> $crate::BenchmarkError { "`name` is not a valid utf8 string!".into() })?;
match name {
$( stringify!($name) => {
$crate::paste::paste! { Self::[< test_benchmark_ $name >]() }
} )*
_ => Err("Could not find test for requested benchmark."),
_ => Err("Could not find test for requested benchmark.".into()),
}
}
}
Expand All @@ -925,15 +925,15 @@ macro_rules! impl_benchmark_test {
where T: frame_system::Config, $( $where_clause )*
{
#[allow(unused)]
fn [<test_benchmark_ $name>] () -> Result<(), &'static str> {
fn [<test_benchmark_ $name>] () -> Result<(), $crate::BenchmarkError> {
let selected_benchmark = SelectedBenchmark::$name;
let components = <
SelectedBenchmark as $crate::BenchmarkingSetup<T, _>
>::components(&selected_benchmark);

let execute_benchmark = |
c: $crate::Vec<($crate::BenchmarkParameter, u32)>
| -> Result<(), &'static str> {
| -> Result<(), $crate::BenchmarkError> {
// Set up the benchmark, return execution + verification function.
let closure_to_verify = <
SelectedBenchmark as $crate::BenchmarkingSetup<T, _>
Expand Down Expand Up @@ -1213,8 +1213,15 @@ macro_rules! impl_benchmark_test_suite {
anything_failed = true;
},
Ok(Err(err)) => {
println!("{}: {}", String::from_utf8_lossy(benchmark_name), err);
anything_failed = true;
match err {
$crate::BenchmarkError::Stop(err) => {
println!("{}: {:?}", String::from_utf8_lossy(benchmark_name), err);
anything_failed = true;
},
$crate::BenchmarkError::Override(_) => {
// This is still considered a success condition.
}
}
},
Ok(Ok(_)) => (),
}
Expand Down Expand Up @@ -1328,25 +1335,40 @@ macro_rules! add_benchmark {
internal_repeats,
} = config;
if &pallet[..] == &name_string[..] {
$batches.push($crate::BenchmarkBatch {
pallet: name_string.to_vec(),
instance: instance_string.to_vec(),
benchmark: benchmark.clone(),
results: $( $location )*::run_benchmark(
&benchmark[..],
&selected_components[..],
whitelist,
*verify,
*internal_repeats,
).map_err(|e| {
let benchmark_result = $( $location )*::run_benchmark(
&benchmark[..],
&selected_components[..],
whitelist,
*verify,
*internal_repeats,
);

let final_results = match benchmark_result {
Ok(results) => results,
Err($crate::BenchmarkError::Override(mut result)) => {
// Insert override warning as the first storage key.
result.keys.insert(0,
(b"Benchmark Override".to_vec(), 0, 0, false)
);
$crate::vec![result]
},
Err($crate::BenchmarkError::Stop(e)) => {
$crate::show_benchmark_debug_info(
instance_string,
benchmark,
selected_components,
verify,
e,
)
})?
);
return Err(e.into());
},
};

$batches.push($crate::BenchmarkBatch {
pallet: name_string.to_vec(),
instance: instance_string.to_vec(),
benchmark: benchmark.clone(),
results: final_results,
});
}
)
Expand Down
Loading

0 comments on commit 643818d

Please sign in to comment.