Update benchmarking macros

prdoc/pr_3934.prdoc

@@ -0,0 +1,12 @@
+title: Update benchmarking macros
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      Current benchmarking macro return a closure with the captured benchmarked code.
+      This could cause compilation issues when the benchmarked code has complex lifetime requirements.
+      This PR updates the existing macro by injecting the recoding parameter and invoking the start / stop method around the benchmarked block instead of returning a closure 
+
+crates:
+  - name: frame-benchmarking
+

substrate/frame/benchmarking/src/tests.rs

@@ -262,15 +262,11 @@ mod benchmarks {
 		let components = <SelectedBenchmark as BenchmarkingSetup<Test>>::components(&selected);
 		assert_eq!(components, vec![(BenchmarkParameter::b, 1, 1000)]);
 
-		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());
+			assert_ok!(<SelectedBenchmark as BenchmarkingSetup<Test>>::test_instance(
+				&selected,
+				&[(BenchmarkParameter::b, 1)],
+			));
 		});
 	}
 
@@ -281,15 +277,11 @@ mod benchmarks {
 		let components = <SelectedBenchmark as BenchmarkingSetup<Test>>::components(&selected);
 		assert_eq!(components, vec![(BenchmarkParameter::b, 1, 1000)]);
 
-		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());
+			assert_ok!(<SelectedBenchmark as BenchmarkingSetup<Test>>::test_instance(
+				&selected,
+				&[(BenchmarkParameter::b, 1)],
+			));
 		});
 	}
 
@@ -300,60 +292,49 @@ mod benchmarks {
 		let components = <SelectedBenchmark as BenchmarkingSetup<Test>>::components(&selected);
 		assert_eq!(components, vec![(BenchmarkParameter::x, 1, 10000)]);
 
-		let closure = <SelectedBenchmark as BenchmarkingSetup<Test>>::instance(
-			&selected,
-			&[(BenchmarkParameter::x, 1)],
-			true,
-		)
-		.expect("failed to create closure");
-
-		assert_ok!(closure());
+		new_test_ext().execute_with(|| {
+			assert_ok!(<SelectedBenchmark as BenchmarkingSetup<Test>>::test_instance(
+				&selected,
+				&[(BenchmarkParameter::x, 1)],
+			));
+		});
 	}
 
 	#[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>>::instance(
-			&selected,
-			&[(BenchmarkParameter::b, 1)],
-			true,
-		)
-		.expect("failed to create closure");
-
 		new_test_ext().execute_with(|| {
-			assert_ok!(closure());
+			assert_ok!(<SelectedBenchmark as BenchmarkingSetup<Test>>::test_instance(
+				&selected,
+				&[(BenchmarkParameter::b, 1)],
+			));
 		});
 
 		// 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!");
+			assert_err!(
+				<SelectedBenchmark as BenchmarkingSetup<Test>>::test_instance(
+					&selected,
+					&[(BenchmarkParameter::x, 10000)],
+				),
+				"You forgot to sort!"
+			);
 		});
 	}
 
 	#[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();
+			let result = <SelectedBenchmark as BenchmarkingSetup<Test>>::test_instance(
+				&selected,
+				&[(BenchmarkParameter::b, 1)],
+			);
 			assert!(matches!(result, Err(BenchmarkError::Override(_))));
 		});
 	}

substrate/frame/benchmarking/src/utils.rs

@@ -23,7 +23,7 @@ use scale_info::TypeInfo;
 use serde::{Deserialize, Serialize};
 use sp_io::hashing::blake2_256;
 use sp_runtime::{traits::TrailingZeroInput, DispatchError};
-use sp_std::{prelude::Box, vec::Vec};
+use sp_std::vec::Vec;
 use sp_storage::TrackedStorageKey;
 
 /// An alphabet of possible parameters to use for benchmarking.
@@ -342,6 +342,61 @@ pub trait Benchmarking {
 	) -> Result<Vec<BenchmarkResult>, BenchmarkError>;
 }
 
+/// The recording trait used to record the time and proof size of a benchmark iteration.
+pub trait Recording {
+	/// Start the recording.
+	fn start(&mut self) {}
+
+	// Stop the recording.
+	fn stop(&mut self) {}
+}
+
+/// A no-op recording, used when instantiating the benchmark test instance.
+pub struct NoopRecording;
+impl Recording for NoopRecording {}
+
+/// Records the time and proof size of a single benchmark iteration.
+#[derive(Default)]
+pub struct BenchmarkRecording {
+	start_extrinsic: u128,
+	finish_extrinsic: u128,
+	start_pov: Option<u32>,
+	end_pov: Option<u32>,
+}
+
+impl Recording for BenchmarkRecording {
+	fn start(&mut self) {
+		self.start_pov = crate::benchmarking::proof_size();
+		self.start_extrinsic = crate::benchmarking::current_time();
+	}
+
+	fn stop(&mut self) {
+		self.finish_extrinsic = crate::benchmarking::current_time();
+		self.end_pov = crate::benchmarking::proof_size();
+	}
+}
+
+impl BenchmarkRecording {
+	pub fn elapsed_extrinsic(&self) -> u128 {
+		self.finish_extrinsic.saturating_sub(self.start_extrinsic)
+	}
+
+	pub fn start_pov(&self) -> Option<u32> {
+		self.start_pov
+	}
+
+	pub fn end_pov(&self) -> Option<u32> {
+		self.end_pov
+	}
+
+	pub fn diff_pov(&self) -> u32 {
+		match (self.start_pov, self.end_pov) {
+			(Some(start), Some(end)) => end.saturating_sub(start),
+			_ => Default::default(),
+		}
+	}
+}
+
 /// The required setup for creating a benchmark.
 ///
 /// Instance generic parameter is optional and can be used in order to capture unused generics for
@@ -353,9 +408,18 @@ pub trait BenchmarkingSetup<T, I = ()> {
 	/// Set up the storage, and prepare a closure to run the benchmark.
 	fn instance(
 		&self,
+		recording: &mut impl Recording,
 		components: &[(BenchmarkParameter, u32)],
 		verify: bool,
-	) -> Result<Box<dyn FnOnce() -> Result<(), BenchmarkError>>, BenchmarkError>;
+	) -> Result<(), BenchmarkError>;
+
+	/// Same as `instance` but passing a no-op recording.
+	fn test_instance(
+		&self,
+		components: &[(BenchmarkParameter, u32)],
+	) -> Result<(), BenchmarkError> {
+		return self.instance(&mut NoopRecording {}, components, true);
+	}
 }
 
 /// Grab an account, seeded by a name and index.

substrate/frame/benchmarking/src/v1.rs

@@ -786,9 +786,10 @@ macro_rules! benchmark_backend {
 
 			fn instance(
 				&self,
+				recording: &mut impl $crate::Recording,
 				components: &[($crate::BenchmarkParameter, u32)],
 				verify: bool
-			) -> Result<$crate::__private::Box<dyn FnOnce() -> Result<(), $crate::BenchmarkError>>, $crate::BenchmarkError> {
+			) -> Result<(), $crate::BenchmarkError> {
 				$(
 					// Prepare instance
 					let $param = components.iter()
@@ -802,13 +803,14 @@ macro_rules! benchmark_backend {
 				$( $param_instancer ; )*
 				$( $post )*
 
-				Ok($crate::__private::Box::new(move || -> Result<(), $crate::BenchmarkError> {
-					$eval;
-					if verify {
-						$postcode;
-					}
-					Ok(())
-				}))
+				recording.start();
+				$eval;
+				recording.stop();
+
+				if verify {
+					$postcode;
+				}
+				Ok(())
 			}
 		}
 	};
@@ -960,14 +962,15 @@ macro_rules! selected_benchmark {
 
 			fn instance(
 				&self,
+				recording: &mut impl $crate::Recording,
 				components: &[($crate::BenchmarkParameter, u32)],
 				verify: bool
-			) -> Result<$crate::__private::Box<dyn FnOnce() -> Result<(), $crate::BenchmarkError>>, $crate::BenchmarkError> {
+			) -> Result<(), $crate::BenchmarkError> {
 				match self {
 					$(
 						Self::$bench => <
 							$bench as $crate::BenchmarkingSetup<T $(, $bench_inst)? >
-						>::instance(&$bench, components, verify),
+						>::instance(&$bench, recording, components, verify),
 					)*
 				}
 			}
@@ -1075,12 +1078,6 @@ macro_rules! impl_benchmark {
 					// Always reset the state after the benchmark.
 					$crate::__private::defer!($crate::benchmarking::wipe_db());
 
-					// 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, verify)?;
-
 					// Set the block number to at least 1 so events are deposited.
 					if $crate::__private::Zero::is_zero(&frame_system::Pallet::<T>::block_number()) {
 						frame_system::Pallet::<T>::set_block_number(1u32.into());
@@ -1108,20 +1105,14 @@ macro_rules! impl_benchmark {
 						verify
 					);
 
-					let start_pov = $crate::benchmarking::proof_size();
-					let start_extrinsic = $crate::benchmarking::current_time();
-
-					closure_to_benchmark()?;
-
-					let finish_extrinsic = $crate::benchmarking::current_time();
-					let end_pov = $crate::benchmarking::proof_size();
+					// Set up the externalities environment for the setup we want to
+					// benchmark.
+					let mut recording = $crate::BenchmarkRecording::default();
+					<SelectedBenchmark as $crate::BenchmarkingSetup<T $(, $instance)?>>::instance(&selected_benchmark, &mut recording, c, verify)?;
 
 					// Calculate the diff caused by the benchmark.
-					let elapsed_extrinsic = finish_extrinsic.saturating_sub(start_extrinsic);
-					let diff_pov = match (start_pov, end_pov) {
-						(Some(start), Some(end)) => end.saturating_sub(start),
-						_ => Default::default(),
-					};
+					let elapsed_extrinsic = recording.elapsed_extrinsic();
+					let diff_pov = recording.diff_pov();
 
 					// Commit the changes to get proper write count
 					$crate::benchmarking::commit_db();
@@ -1136,7 +1127,7 @@ macro_rules! impl_benchmark {
 					);
 					$crate::__private::log::trace!(
 						target: "benchmark",
-						"Proof sizes: before {:?} after {:?} diff {}", &start_pov, &end_pov, &diff_pov
+						"Proof sizes: before {:?} after {:?} diff {}", recording.start_pov(), recording.end_pov(), &diff_pov
 					);
 
 					// Time the storage root recalculation.
@@ -1230,18 +1221,13 @@ macro_rules! impl_benchmark_test {
 						// Always reset the state after the benchmark.
 						$crate::__private::defer!($crate::benchmarking::wipe_db());
 
-						// Set up the benchmark, return execution + verification function.
-						let closure_to_verify = <
-							SelectedBenchmark as $crate::BenchmarkingSetup<T, _>
-						>::instance(&selected_benchmark, &c, true)?;
-
 						// Set the block number to at least 1 so events are deposited.
 						if $crate::__private::Zero::is_zero(&frame_system::Pallet::<T>::block_number()) {
 							frame_system::Pallet::<T>::set_block_number(1u32.into());
 						}
 
 						// Run execution + verification
-						closure_to_verify()
+						<SelectedBenchmark as $crate::BenchmarkingSetup<T, _>>::test_instance(&selected_benchmark, &c)
 					};
 
 					if components.is_empty() {

substrate/frame/staking/src/benchmarking.rs

@@ -1077,15 +1077,13 @@ mod tests {
 				(frame_benchmarking::BenchmarkParameter::v, v),
 				(frame_benchmarking::BenchmarkParameter::n, n),
 			];
-			let closure_to_benchmark =
-				<SelectedBenchmark as frame_benchmarking::BenchmarkingSetup<Test>>::instance(
+
+			assert_ok!(
+				<SelectedBenchmark as frame_benchmarking::BenchmarkingSetup<Test>>::test_instance(
 					&selected_benchmark,
 					&c,
-					true,
 				)
-				.unwrap();
-
-			assert_ok!(closure_to_benchmark());
+			);
 		});
 	}
 }