From f9a591cfa527668e89101a16cbbc6e8a6d6dc894 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 5 Sep 2021 19:16:40 -0400 Subject: [PATCH 1/4] introduce benchmark skip --- frame/benchmarking/src/lib.rs | 28 ++++++++++++++++++++-------- frame/benchmarking/src/utils.rs | 11 +++++++---- 2 files changed, 27 insertions(+), 12 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index fb602f0732b7d..7447e9fed8734 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1220,6 +1220,11 @@ macro_rules! impl_benchmark_test_suite { }, $crate::BenchmarkError::Override(_) => { // This is still considered a success condition. + $crate::log::error!("WARNING: benchmark error overrided"); + }, + $crate::BenchmarkError::Skip => { + // This is considered a success condition. + $crate::log::error!("WARNING: benchmark error skipped") } } }, @@ -1344,13 +1349,14 @@ macro_rules! add_benchmark { ); let final_results = match benchmark_result { - Ok(results) => results, + Ok(results) => Some(results), Err($crate::BenchmarkError::Override(mut result)) => { // Insert override warning as the first storage key. + $crate::log::error!("WARNING: benchmark error overrided"); result.keys.insert(0, (b"Benchmark Override".to_vec(), 0, 0, false) ); - $crate::vec![result] + Some($crate::vec![result]) }, Err($crate::BenchmarkError::Stop(e)) => { $crate::show_benchmark_debug_info( @@ -1362,14 +1368,20 @@ macro_rules! add_benchmark { ); return Err(e.into()); }, + Err($crate::BenchmarkError::Skip) => { + $crate::log::error!("WARNING: benchmark error skipped"); + None + } }; - $batches.push($crate::BenchmarkBatch { - pallet: name_string.to_vec(), - instance: instance_string.to_vec(), - benchmark: benchmark.clone(), - results: final_results, - }); + if let Some(final_results) = final_results { + $batches.push($crate::BenchmarkBatch { + pallet: name_string.to_vec(), + instance: instance_string.to_vec(), + benchmark: benchmark.clone(), + results: final_results, + }); + } } ) } diff --git a/frame/benchmarking/src/utils.rs b/frame/benchmarking/src/utils.rs index 64eb611a187b0..d54e32f0ce9d8 100644 --- a/frame/benchmarking/src/utils.rs +++ b/frame/benchmarking/src/utils.rs @@ -119,14 +119,16 @@ impl BenchmarkResult { } /// Possible errors returned from the benchmarking pipeline. -/// -/// * Stop: The benchmarking pipeline should stop and return the inner string. -/// * WeightOverride: The benchmarking pipeline is allowed to fail here, and we should use the -/// included weight instead. #[derive(Clone, PartialEq, Debug)] pub enum BenchmarkError { + /// The benchmarking pipeline should stop and return the inner string. Stop(&'static str), + /// The benchmarking pipeline is allowed to fail here, and we should use the + /// included weight instead. Override(BenchmarkResult), + /// The benchmarking pipeline is allowed to fail here, and we should simply + /// skip processing these results. + Skip, } impl From for &'static str { @@ -134,6 +136,7 @@ impl From for &'static str { match e { BenchmarkError::Stop(s) => s, BenchmarkError::Override(_) => "benchmark override", + BenchmarkError::Skip => "benchmark skip", } } } From 6a0a047e43b2eefa139d0fb68d321eb61258c7c8 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 5 Sep 2021 20:53:58 -0400 Subject: [PATCH 2/4] fmt --- client/utils/src/metrics.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/client/utils/src/metrics.rs b/client/utils/src/metrics.rs index 8df8e65962474..85ccce626bc25 100644 --- a/client/utils/src/metrics.rs +++ b/client/utils/src/metrics.rs @@ -16,7 +16,6 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . - //! Metering primitives and globals use lazy_static::lazy_static; From b78ca0837e973feb9ad2ddd71dc02027319537c1 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 6 Sep 2021 15:53:57 -0400 Subject: [PATCH 3/4] Update lib.rs --- frame/benchmarking/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 7447e9fed8734..3245a4a35636b 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1220,11 +1220,11 @@ macro_rules! impl_benchmark_test_suite { }, $crate::BenchmarkError::Override(_) => { // This is still considered a success condition. - $crate::log::error!("WARNING: benchmark error overrided"); + $crate::log::error!("WARNING: benchmark error overrided - {}", String::from_utf8_lossy(benchmark_name)); }, $crate::BenchmarkError::Skip => { // This is considered a success condition. - $crate::log::error!("WARNING: benchmark error skipped") + $crate::log::error!("WARNING: benchmark error skipped - {}", String::from_utf8_lossy(benchmark_name)); } } }, @@ -1352,7 +1352,7 @@ macro_rules! add_benchmark { Ok(results) => Some(results), Err($crate::BenchmarkError::Override(mut result)) => { // Insert override warning as the first storage key. - $crate::log::error!("WARNING: benchmark error overrided"); + $crate::log::error!("WARNING: benchmark error overrided - {}", String::from_utf8_lossy(benchmark)); result.keys.insert(0, (b"Benchmark Override".to_vec(), 0, 0, false) ); @@ -1369,7 +1369,7 @@ macro_rules! add_benchmark { return Err(e.into()); }, Err($crate::BenchmarkError::Skip) => { - $crate::log::error!("WARNING: benchmark error skipped"); + $crate::log::error!("WARNING: benchmark error skipped - {}", String::from_utf8_lossy(benchmark)); None } }; From 355c532661f18674b495a0c1d357993d4c346b84 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 6 Sep 2021 16:14:09 -0400 Subject: [PATCH 4/4] fix up --- frame/benchmarking/src/lib.rs | 44 ++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 3245a4a35636b..42eeac483fa97 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -40,7 +40,7 @@ pub use sp_io::storage::root as storage_root; #[doc(hidden)] pub use sp_runtime::traits::Zero; #[doc(hidden)] -pub use sp_std::{self, boxed::Box, prelude::Vec, vec}; +pub use sp_std::{self, boxed::Box, prelude::Vec, str, vec}; #[doc(hidden)] pub use sp_storage::TrackedStorageKey; pub use utils::*; @@ -774,7 +774,7 @@ macro_rules! impl_benchmark { internal_repeats: u32, ) -> Result<$crate::Vec<$crate::BenchmarkResult>, $crate::BenchmarkError> { // Map the input to the selected benchmark. - let extrinsic = $crate::sp_std::str::from_utf8(extrinsic) + let extrinsic = $crate::str::from_utf8(extrinsic) .map_err(|_| "`extrinsic` is not a valid utf8 string!")?; let selected_benchmark = match extrinsic { $( stringify!($name) => SelectedBenchmark::$name, )* @@ -894,7 +894,7 @@ macro_rules! impl_benchmark { /// author chooses not to implement benchmarks. #[allow(unused)] fn test_bench_by_name(name: &[u8]) -> Result<(), $crate::BenchmarkError> { - let name = $crate::sp_std::str::from_utf8(name) + let name = $crate::str::from_utf8(name) .map_err(|_| -> $crate::BenchmarkError { "`name` is not a valid utf8 string!".into() })?; match name { $( stringify!($name) => { @@ -1209,22 +1209,40 @@ macro_rules! impl_benchmark_test_suite { $bench_module::<$test>::test_bench_by_name(benchmark_name) }) { Err(err) => { - println!("{}: {:?}", String::from_utf8_lossy(benchmark_name), err); + println!( + "{}: {:?}", + $crate::str::from_utf8(benchmark_name) + .expect("benchmark name is always a valid string!"), + err, + ); anything_failed = true; }, Ok(Err(err)) => { match err { $crate::BenchmarkError::Stop(err) => { - println!("{}: {:?}", String::from_utf8_lossy(benchmark_name), err); + println!( + "{}: {:?}", + $crate::str::from_utf8(benchmark_name) + .expect("benchmark name is always a valid string!"), + err, + ); anything_failed = true; }, $crate::BenchmarkError::Override(_) => { // This is still considered a success condition. - $crate::log::error!("WARNING: benchmark error overrided - {}", String::from_utf8_lossy(benchmark_name)); + $crate::log::error!( + "WARNING: benchmark error overrided - {}", + $crate::str::from_utf8(benchmark_name) + .expect("benchmark name is always a valid string!"), + ); }, $crate::BenchmarkError::Skip => { // This is considered a success condition. - $crate::log::error!("WARNING: benchmark error skipped - {}", String::from_utf8_lossy(benchmark_name)); + $crate::log::error!( + "WARNING: benchmark error skipped - {}", + $crate::str::from_utf8(benchmark_name) + .expect("benchmark name is always a valid string!"), + ); } } }, @@ -1352,7 +1370,11 @@ macro_rules! add_benchmark { Ok(results) => Some(results), Err($crate::BenchmarkError::Override(mut result)) => { // Insert override warning as the first storage key. - $crate::log::error!("WARNING: benchmark error overrided - {}", String::from_utf8_lossy(benchmark)); + $crate::log::error!( + "WARNING: benchmark error overrided - {}", + $crate::str::from_utf8(benchmark) + .expect("benchmark name is always a valid string!") + ); result.keys.insert(0, (b"Benchmark Override".to_vec(), 0, 0, false) ); @@ -1369,7 +1391,11 @@ macro_rules! add_benchmark { return Err(e.into()); }, Err($crate::BenchmarkError::Skip) => { - $crate::log::error!("WARNING: benchmark error skipped - {}", String::from_utf8_lossy(benchmark)); + $crate::log::error!( + "WARNING: benchmark error skipped - {}", + $crate::str::from_utf8(benchmark) + .expect("benchmark name is always a valid string!") + ); None } };