Custom Benchmark Errors and Override (#9517)

* 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 <bkchr@users.noreply.github.com>

* doesnt need encode/decode

* fix benchmark return

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/frame/benchmarking/src/analysis.rs

@@ -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;
@@ -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
 		}
@@ -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)
 		}
@@ -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)
 		}
@@ -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);
 
@@ -402,8 +402,8 @@ mod tests {
 		storage_root_time: u128,
 		reads: u32,
 		writes: u32,
-	) -> BenchmarkResults {
-		BenchmarkResults {
+	) -> BenchmarkResult {
+		BenchmarkResult {
 			components,
 			extrinsic_time,
 			storage_root_time,

substrate/frame/benchmarking/src/lib.rs

@@ -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()
@@ -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;
@@ -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 => <
@@ -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> {
@@ -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
@@ -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) {
@@ -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,
@@ -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()),
 				}
 			}
 		}
@@ -925,7 +925,7 @@ 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, _>
@@ -933,7 +933,7 @@ macro_rules! impl_benchmark_test {
 
 					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, _>
@@ -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(_)) => (),
 						}
@@ -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,
 			});
 		}
 	)

substrate/frame/benchmarking/src/tests.rs

@@ -28,48 +28,45 @@ use sp_runtime::{
 };
 use sp_std::prelude::*;
 
+#[frame_support::pallet]
 mod pallet_test {
-	use frame_support::pallet_prelude::Get;
+	use frame_support::pallet_prelude::*;
+	use frame_system::pallet_prelude::*;
 
-	frame_support::decl_storage! {
-		trait Store for Module<T: Config> as Test where
-			<T as OtherConfig>::OtherEvent: Into<<T as Config>::Event>
-		{
-			pub Value get(fn value): Option<u32>;
-		}
-	}
+	#[pallet::pallet]
+	#[pallet::generate_store(pub(super) trait Store)]
+	pub struct Pallet<T>(_);
 
-	frame_support::decl_module! {
-		pub struct Module<T: Config> for enum Call where
-			origin: T::Origin, <T as OtherConfig>::OtherEvent: Into<<T as Config>::Event>
-		{
-			#[weight = 0]
-			fn set_value(origin, n: u32) -> frame_support::dispatch::DispatchResult {
-				let _sender = frame_system::ensure_signed(origin)?;
-				Value::put(n);
-				Ok(())
-			}
-
-			#[weight = 0]
-			fn dummy(origin, _n: u32) -> frame_support::dispatch::DispatchResult {
-				let _sender = frame_system::ensure_none(origin)?;
-				Ok(())
-			}
-		}
-	}
-
-	pub trait OtherConfig {
-		type OtherEvent;
-	}
-
-	pub trait Config: frame_system::Config + OtherConfig
-	where
-		Self::OtherEvent: Into<<Self as Config>::Event>,
-	{
-		type Event;
+	#[pallet::config]
+	pub trait Config: frame_system::Config {
 		type LowerBound: Get<u32>;
 		type UpperBound: Get<u32>;
 	}
+
+	#[pallet::storage]
+	#[pallet::getter(fn heartbeat_after)]
+	pub(crate) type Value<T: Config> = StorageValue<_, u32, OptionQuery>;
+
+	#[pallet::call]
+	impl<T: Config> Pallet<T> {
+		#[pallet::weight(0)]
+		pub fn set_value(origin: OriginFor<T>, n: u32) -> DispatchResult {
+			let _sender = frame_system::ensure_signed(origin)?;
+			Value::<T>::put(n);
+			Ok(())
+		}
+
+		#[pallet::weight(0)]
+		pub fn dummy(origin: OriginFor<T>, _n: u32) -> DispatchResult {
+			let _sender = frame_system::ensure_none(origin)?;
+			Ok(())
+		}
+
+		#[pallet::weight(0)]
+		pub fn always_error(_origin: OriginFor<T>) -> DispatchResult {
+			return Err("I always fail".into())
+		}
+	}
 }
 
 type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
@@ -118,27 +115,18 @@ parameter_types! {
 }
 
 impl pallet_test::Config for Test {
-	type Event = Event;
 	type LowerBound = LowerBound;
 	type UpperBound = UpperBound;
 }
 
-impl pallet_test::OtherConfig for Test {
-	type OtherEvent = Event;
-}
-
 fn new_test_ext() -> sp_io::TestExternalities {
 	GenesisConfig::default().build_storage().unwrap().into()
 }
 
 mod benchmarks {
-	use super::{
-		new_test_ext,
-		pallet_test::{self, Value},
-		Test,
-	};
-	use crate::{account, BenchmarkParameter, BenchmarkingSetup};
-	use frame_support::{assert_err, assert_ok, ensure, traits::Get, StorageValue};
+	use super::{new_test_ext, pallet_test::Value, Test};
+	use crate::{account, BenchmarkError, BenchmarkParameter, BenchmarkResult, BenchmarkingSetup};
+	use frame_support::{assert_err, assert_ok, ensure, traits::Get};
 	use frame_system::RawOrigin;
 	use sp_std::prelude::*;
 
@@ -148,8 +136,7 @@ mod benchmarks {
 	crate::benchmarks! {
 		where_clause {
 			where
-				<T as pallet_test::OtherConfig>::OtherEvent: Into<<T as pallet_test::Config>::Event> + Clone,
-				<T as pallet_test::Config>::Event: Clone,
+				crate::tests::Origin: From<RawOrigin<<T as frame_system::Config>::AccountId>>,
 		}
 
 		set_value {
@@ -157,7 +144,7 @@ mod benchmarks {
 			let caller = account::<T::AccountId>("caller", 0, 0);
 		}: _ (RawOrigin::Signed(caller), b.into())
 		verify {
-			assert_eq!(Value::get(), Some(b));
+			assert_eq!(Value::<T>::get(), Some(b));
 		}
 
 		other_name {
@@ -206,7 +193,7 @@ mod benchmarks {
 			let caller = account::<T::AccountId>("caller", 0, 0);
 		}: set_value(RawOrigin::Signed(caller), b.into())
 		verify {
-			assert_eq!(Value::get(), Some(b));
+			assert_eq!(Value::<T>::get(), Some(b));
 		}
 
 		#[skip_meta]
@@ -215,7 +202,21 @@ mod benchmarks {
 			let caller = account::<T::AccountId>("caller", 0, 0);
 		}: set_value(RawOrigin::Signed(caller), b.into())
 		verify {
-			assert_eq!(Value::get(), Some(b));
+			assert_eq!(Value::<T>::get(), Some(b));
+		}
+
+		override_benchmark {
+			let b in 1 .. 1000;
+			let caller = account::<T::AccountId>("caller", 0, 0);
+		}: {
+			Err(BenchmarkError::Override(
+				BenchmarkResult {
+					extrinsic_time: 1_234_567_890,
+					reads: 1337,
+					writes: 420,
+					..Default::default()
+				}
+			))?;
 		}
 	}
 
@@ -306,6 +307,23 @@ mod benchmarks {
 		});
 	}
 
+	#[test]
+	fn benchmark_override_works() {
+		let selected = SelectedBenchmark::override_benchmark;
+
+		let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
+			&selected,
+			&[(BenchmarkParameter::b, 1)],
+			true,
+		)
+		.expect("failed to create closure");
+
+		new_test_ext().execute_with(|| {
+			let result = closure();
+			assert!(matches!(result, Err(BenchmarkError::Override(_))));
+		});
+	}
+
 	#[test]
 	fn benchmarks_generate_unit_tests() {
 		new_test_ext().execute_with(|| {

substrate/frame/benchmarking/src/utils.rs

@@ -18,7 +18,11 @@
 //! Interfaces, types and utils for benchmarking a FRAME runtime.
 
 use codec::{Decode, Encode};
-use frame_support::traits::StorageInfo;
+use frame_support::{
+	dispatch::{DispatchError, DispatchErrorWithPostInfo},
+	pallet_prelude::*,
+	traits::StorageInfo,
+};
 use sp_io::hashing::blake2_256;
 use sp_std::{prelude::Box, vec::Vec};
 use sp_storage::TrackedStorageKey;
@@ -73,7 +77,7 @@ pub struct BenchmarkBatch {
 	/// The extrinsic (or benchmark name) of this benchmark.
 	pub benchmark: Vec<u8>,
 	/// The results from this benchmark.
-	pub results: Vec<BenchmarkResults>,
+	pub results: Vec<BenchmarkResult>,
 }
 
 // TODO: could probably make API cleaner here.
@@ -87,16 +91,16 @@ pub struct BenchmarkBatchSplitResults {
 	/// The extrinsic (or benchmark name) of this benchmark.
 	pub benchmark: Vec<u8>,
 	/// The extrinsic timing results from this benchmark.
-	pub time_results: Vec<BenchmarkResults>,
+	pub time_results: Vec<BenchmarkResult>,
 	/// The db tracking results from this benchmark.
-	pub db_results: Vec<BenchmarkResults>,
+	pub db_results: Vec<BenchmarkResult>,
 }
 
-/// Results from running benchmarks on a FRAME pallet.
+/// Result from running benchmarks on a FRAME pallet.
 /// Contains duration of the function call in nanoseconds along with the benchmark parameters
 /// used for that benchmark result.
 #[derive(Encode, Decode, Default, Clone, PartialEq, Debug)]
-pub struct BenchmarkResults {
+pub struct BenchmarkResult {
 	pub components: Vec<(BenchmarkParameter, u32)>,
 	pub extrinsic_time: u128,
 	pub storage_root_time: u128,
@@ -108,6 +112,50 @@ pub struct BenchmarkResults {
 	pub keys: Vec<(Vec<u8>, u32, u32, bool)>,
 }
 
+impl BenchmarkResult {
+	pub fn from_weight(w: Weight) -> Self {
+		Self { extrinsic_time: (w as u128) / 1_000, ..Default::default() }
+	}
+}
+
+/// 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 {
+	Stop(&'static str),
+	Override(BenchmarkResult),
+}
+
+impl From<BenchmarkError> for &'static str {
+	fn from(e: BenchmarkError) -> Self {
+		match e {
+			BenchmarkError::Stop(s) => s,
+			BenchmarkError::Override(_) => "benchmark override",
+		}
+	}
+}
+
+impl From<&'static str> for BenchmarkError {
+	fn from(s: &'static str) -> Self {
+		Self::Stop(s)
+	}
+}
+
+impl From<DispatchErrorWithPostInfo> for BenchmarkError {
+	fn from(e: DispatchErrorWithPostInfo) -> Self {
+		Self::Stop(e.into())
+	}
+}
+
+impl From<DispatchError> for BenchmarkError {
+	fn from(e: DispatchError) -> Self {
+		Self::Stop(e.into())
+	}
+}
+
 /// Configuration used to setup and run runtime benchmarks.
 #[derive(Encode, Decode, Default, Clone, PartialEq, Debug)]
 pub struct BenchmarkConfig {
@@ -235,7 +283,7 @@ pub trait Benchmarking {
 }
 
 /// The pallet benchmarking trait.
-pub trait Benchmarking<T> {
+pub trait Benchmarking {
 	/// Get the benchmarks available for this pallet. Generally there is one benchmark per
 	/// extrinsic, so these are sometimes just called "extrinsics".
 	///
@@ -251,7 +299,7 @@ pub trait Benchmarking<T> {
 		whitelist: &[TrackedStorageKey],
 		verify: bool,
 		internal_repeats: u32,
-	) -> Result<Vec<T>, &'static str>;
+	) -> Result<Vec<BenchmarkResult>, BenchmarkError>;
 }
 
 /// The required setup for creating a benchmark.
@@ -267,7 +315,7 @@ pub trait BenchmarkingSetup<T, I = ()> {
 		&self,
 		components: &[(BenchmarkParameter, u32)],
 		verify: bool,
-	) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str>;
+	) -> Result<Box<dyn FnOnce() -> Result<(), BenchmarkError>>, &'static str>;
 }
 
 /// Grab an account, seeded by a name and index.