new proc-macro-based benchmarking syntax (#12924)

* add stub for new benchmark macro

* benchmark syntax

* add #[extrinsic call] separator

* parse #[benchmark] item as a function

* proper emission of error when #[extrinsic_call] annotation is missing

* clean up

* enclosing module via benchmarks! { } working

* use an attribute macro on the module instead of benchmarks! { }

* cargo fmt

* working component implementation

* WIP

* working

* add syntax for Linear<A, B>

* parsing of param ranges (still need to build tuple though)

* params parsing WIP

* clean up (don't need extrinsic call name)

* use proper Result syntax for BenchmarkDef parsing

* proper parsing of Linear<0, 1> style args

* successfully parse and make use of linear component ranges 💥

* rename support variable => home because eventually will be moved

* compile-time check that param range types implement ParamRange

* switch to using balances as example, failing on instance pallet

* successfully set up __origin and __call with balances 💥

* clean up

* use a module

* don't need a variable for transfer

* rename benchmark_transfer -> transfer because no longer conflicts

* clean up

* working with transfer_increasing_users as well 💥

* re-add BareBlock

* add comments for undocumented structs+functions+traits

* refactor in preparation for removing module requirements

* switch to a block instead of a module

* use the outer macro pattern to to enable #[benchmarks] aggregation

* successfully generate SelectedBenchmark 💥

* implement components for SelectedBenchmark

* implement instance for SelectedBenchmark

* properly track #[extra]

* working impl for fn benchmarks()

* run_benchmarks WIP

* finish run_benchmark! impl 💥

* import balances transfer_best_case benchmark

* import transfer_keep_alive balances pallet benchmark

* import set_balance_creating balances pallet benchmark

* import set_balance_killing balances pallet benchmark

* import force_transfer balances pallet benchmark

* add #[extra] annotation and docs to transfer_increasing_users

* import transfer_all balances pallet benchmark

* import force_unreserve balances pallet benchmark

* prepare to implement impl_benchmark_test_suite!

* ensure tests cover #[extra] before and after #[benchmark] tag

* refactor

* clean up

* fix

* move to outer

* switch to benchmarks/instance_benchmarks

* test impl almost done, strange compiler error

* benchmark test suites working 💥

* clean up

* add stub and basic parsing for where_clause

* working except where clause and extrinsic calls containing method chains

* assume option (2) for now wrt https://github.com/paritytech/substrate/pull/12924#issuecomment-1372938718

* clean up

* switch to attribute-style

* properly handle where clauses

* fix subtle missing where clause, now just MessageQueue issues

* fix block formatting in message-queue pallet

* switch to block vs non-block parsing of extrinsic call

* working now but some benchmark tests failing

* message-queue tests working (run order issue fixed) 🎉

* add comments and internal docs for fame_support_procedural::benchmark

* fix license years

* docs for lib.rs

* add docs to new support procedural macros

* don't allow #[benchmark] outside of benchmarking module

* add docs

* use benchmark(extra, skip_meta) style args

* update docs accordingly

* appease clippy

* bump ci

* add notes about `extra` and `skip_meta`

* fix doc tests

* re-run CI

* use `ignore` instead of `no_run` on doc examples

* bump CI

* replace some if-lets with if-elses

* more refactoring of if-let statements

* fix remaining if-lets in BenchmarkDef::from()

* fix if-lets in benchmarks()

* fix remaining if-lets, use nested find_map for extrinsic call

* switch to use #[extrinsic_call] or #[block] situationally

* refactor ExtrinsicCallDef => BenchmarkCallDef

* update docs with info about #[block]

* add macro stub for #[extrinsic_call]

* fix docs and add stub for #[block] as well

* remove unused extern crate line

* fix clippy nits

* Use V2 bench syntax in pallet-example-basic

Just testing the dev-ex...

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* carry over comment

* use curly-brace style for impl_benchmark_test_suite!

* remove unneeded parenthesis

* proper handling of _() extrinsic call style

* add docs for _() syntax

* fix crate access

* simplify keyword access

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* simplify module content destructuring

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* fix crate access "frame_benchmarking" => "frame-benchmarking", compiles

* use _() extrinsic call syntax where possible in balances

* simplify attr.path.segments.last()

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* fix compile error being suppressed

* simplify extrinsic call keyword parsing

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* use ? operator instead of return None

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* rename generics => type_use_generics
rename full_generics => type_impl_generics

* simplify extrinsic call extraction with transpose

* bump CI

* nit

* proper handling of too many + too few block/extrinsic call annotations

* change to B >= A

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* remove unneeded ignore

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* remove another ignore

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* add ui tests

* use _() style extrinsic call on accumulate_dummy

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* add range check to ParamRange

* ui test for bad param ranges

* fix failing example

* add ignore back to other failing example

* tweak expr_call span

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* fix typo

* eliminate a match

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* change pub fn benchmarks to return Result<TokenStream>

* fix origin error span

* more informative error for invalid benchmark parameter name

* fix spans on a few benchmark errors

* remove unneeded clone

* refactor inner loop of benchmark function parsing

* preserve mod attributes

* refactor outer loop of benchmark def parsing code, greatly simplified

* simplify to use a ? operator when parsing benchmark attr path

* fix another ? operator

* further simplify benchmark function attr parsing with more ? ops

* refactor extrinsic call handling to use if let rather than match

* replace is_ok => is_err

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* re-use name during expansion of benchmark def

* remove unneeded clone

* fix span for origin missing error

* fix missing semi

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Co-authored-by: parity-processbot <>

substrate/frame/message-queue/src/benchmarking.rs

@@ -1,6 +1,6 @@
 // This file is part of Substrate.
 
-// Copyright (C) 2022 Parity Technologies (UK) Ltd.
+// Copyright (C) 2023 Parity Technologies (UK) Ltd.
 // SPDX-License-Identifier: Apache-2.0
 
 // Licensed under the Apache License, Version 2.0 (the "License");
@@ -22,68 +22,85 @@
 
 use super::{mock_helpers::*, Pallet as MessageQueue, *};
 
-use frame_benchmarking::{benchmarks, whitelisted_caller};
-use frame_support::traits::Get;
+use frame_benchmarking::{impl_benchmark_test_suite, whitelisted_caller};
+use frame_support::{benchmarking::*, traits::Get};
 use frame_system::RawOrigin;
 use sp_std::prelude::*;
 
-benchmarks! {
-	where_clause {
-		where
-			// NOTE: We need to generate multiple origins, therefore Origin is `From<u32>`. The
-			// `PartialEq` is for asserting the outcome of the ring (un)knitting and *could* be
-			// removed if really necessary.
-			<<T as Config>::MessageProcessor as ProcessMessage>::Origin: From<u32> + PartialEq,
-			<T as Config>::Size: From<u32>,
-	}
+#[benchmarks(
+	where
+		<<T as Config>::MessageProcessor as ProcessMessage>::Origin: From<u32> + PartialEq,
+		<T as Config>::Size: From<u32>,
+		// NOTE: We need to generate multiple origins, therefore Origin is `From<u32>`. The
+		// `PartialEq` is for asserting the outcome of the ring (un)knitting and *could* be
+		// removed if really necessary.
+)]
+mod benchmarks {
+	use super::*;
 
 	// Worst case path of `ready_ring_knit`.
-	ready_ring_knit {
-		let mid: MessageOriginOf::<T> = 1.into();
+	#[benchmark]
+	fn ready_ring_knit() {
+		let mid: MessageOriginOf<T> = 1.into();
 		build_ring::<T>(&[0.into(), mid.clone(), 2.into()]);
 		unknit::<T>(&mid);
 		assert_ring::<T>(&[0.into(), 2.into()]);
 		let mut neighbours = None;
-	}: {
-		neighbours = MessageQueue::<T>::ready_ring_knit(&mid).ok();
-	} verify {
+
+		#[block]
+		{
+			neighbours = MessageQueue::<T>::ready_ring_knit(&mid).ok();
+		}
+
 		// The neighbours needs to be modified manually.
-		BookStateFor::<T>::mutate(&mid, |b| { b.ready_neighbours = neighbours });
+		BookStateFor::<T>::mutate(&mid, |b| b.ready_neighbours = neighbours);
 		assert_ring::<T>(&[0.into(), 2.into(), mid]);
 	}
 
 	// Worst case path of `ready_ring_unknit`.
-	ready_ring_unknit {
+	#[benchmark]
+	fn ready_ring_unknit() {
 		build_ring::<T>(&[0.into(), 1.into(), 2.into()]);
 		assert_ring::<T>(&[0.into(), 1.into(), 2.into()]);
-		let o: MessageOriginOf::<T> = 0.into();
+		let o: MessageOriginOf<T> = 0.into();
 		let neighbours = BookStateFor::<T>::get(&o).ready_neighbours.unwrap();
-	}: {
-		MessageQueue::<T>::ready_ring_unknit(&o, neighbours);
-	} verify {
+
+		#[block]
+		{
+			MessageQueue::<T>::ready_ring_unknit(&o, neighbours);
+		}
+
 		assert_ring::<T>(&[1.into(), 2.into()]);
 	}
 
 	// `service_queues` without any queue processing.
-	service_queue_base {
-	}: {
-		MessageQueue::<T>::service_queue(0.into(), &mut WeightMeter::max_limit(), Weight::MAX)
+	#[benchmark]
+	fn service_queue_base() {
+		#[block]
+		{
+			MessageQueue::<T>::service_queue(0.into(), &mut WeightMeter::max_limit(), Weight::MAX);
+		}
 	}
 
 	// `service_page` without any message processing but with page completion.
-	service_page_base_completion {
+	#[benchmark]
+	fn service_page_base_completion() {
 		let origin: MessageOriginOf<T> = 0.into();
 		let page = PageOf::<T>::default();
 		Pages::<T>::insert(&origin, 0, &page);
 		let mut book_state = single_page_book::<T>();
 		let mut meter = WeightMeter::max_limit();
 		let limit = Weight::MAX;
-	}: {
-		MessageQueue::<T>::service_page(&origin, &mut book_state, &mut meter, limit)
+
+		#[block]
+		{
+			MessageQueue::<T>::service_page(&origin, &mut book_state, &mut meter, limit);
+		}
 	}
 
 	// `service_page` without any message processing and without page completion.
-	service_page_base_no_completion {
+	#[benchmark]
+	fn service_page_base_no_completion() {
 		let origin: MessageOriginOf<T> = 0.into();
 		let mut page = PageOf::<T>::default();
 		// Mock the storage such that `is_complete` returns `false` but `peek_first` returns `None`.
@@ -93,49 +110,73 @@ benchmarks! {
 		let mut book_state = single_page_book::<T>();
 		let mut meter = WeightMeter::max_limit();
 		let limit = Weight::MAX;
-	}: {
-		MessageQueue::<T>::service_page(&origin, &mut book_state, &mut meter, limit)
+
+		#[block]
+		{
+			MessageQueue::<T>::service_page(&origin, &mut book_state, &mut meter, limit);
+		}
 	}
 
 	// Processing a single message from a page.
-	service_page_item {
+	#[benchmark]
+	fn service_page_item() {
 		let msg = vec![1u8; MaxMessageLenOf::<T>::get() as usize];
 		let mut page = page::<T>(&msg.clone());
 		let mut book = book_for::<T>(&page);
 		assert!(page.peek_first().is_some(), "There is one message");
 		let mut weight = WeightMeter::max_limit();
-	}: {
-		let status = MessageQueue::<T>::service_page_item(&0u32.into(), 0, &mut book, &mut page, &mut weight, Weight::MAX);
-		assert_eq!(status, ItemExecutionStatus::Executed(true));
-	} verify {
+
+		#[block]
+		{
+			let status = MessageQueue::<T>::service_page_item(
+				&0u32.into(),
+				0,
+				&mut book,
+				&mut page,
+				&mut weight,
+				Weight::MAX,
+			);
+			assert_eq!(status, ItemExecutionStatus::Executed(true));
+		}
+
 		// Check that it was processed.
-		assert_last_event::<T>(Event::Processed {
-			hash: T::Hashing::hash(&msg), origin: 0.into(),
-			weight_used: 1.into_weight(), success: true
-		}.into());
+		assert_last_event::<T>(
+			Event::Processed {
+				hash: T::Hashing::hash(&msg),
+				origin: 0.into(),
+				weight_used: 1.into_weight(),
+				success: true,
+			}
+			.into(),
+		);
 		let (_, processed, _) = page.peek_index(0).unwrap();
 		assert!(processed);
 		assert_eq!(book.message_count, 0);
 	}
 
 	// Worst case for calling `bump_service_head`.
-	bump_service_head {
+	#[benchmark]
+	fn bump_service_head() {
 		setup_bump_service_head::<T>(0.into(), 10.into());
 		let mut weight = WeightMeter::max_limit();
-	}: {
-		MessageQueue::<T>::bump_service_head(&mut weight);
-	} verify {
+
+		#[block]
+		{
+			MessageQueue::<T>::bump_service_head(&mut weight);
+		}
+
 		assert_eq!(ServiceHead::<T>::get().unwrap(), 10u32.into());
 		assert_eq!(weight.consumed, T::WeightInfo::bump_service_head());
 	}
 
-	reap_page {
+	#[benchmark]
+	fn reap_page() {
 		// Mock the storage to get a *cullable* but not *reapable* page.
 		let origin: MessageOriginOf<T> = 0.into();
 		let mut book = single_page_book::<T>();
 		let (page, msgs) = full_page::<T>();
 
-		for p in 0 .. T::MaxStale::get() * T::MaxStale::get() {
+		for p in 0..T::MaxStale::get() * T::MaxStale::get() {
 			if p == 0 {
 				Pages::<T>::insert(&origin, p, &page);
 			}
@@ -148,16 +189,19 @@ benchmarks! {
 		BookStateFor::<T>::insert(&origin, &book);
 		assert!(Pages::<T>::contains_key(&origin, 0));
 
-	}: _(RawOrigin::Signed(whitelisted_caller()), 0u32.into(), 0)
-	verify {
-		assert_last_event::<T>(Event::PageReaped{ origin: 0.into(), index: 0 }.into());
+		#[extrinsic_call]
+		_(RawOrigin::Signed(whitelisted_caller()), 0u32.into(), 0);
+
+		assert_last_event::<T>(Event::PageReaped { origin: 0.into(), index: 0 }.into());
 		assert!(!Pages::<T>::contains_key(&origin, 0));
 	}
 
 	// Worst case for `execute_overweight` where the page is removed as completed.
 	//
-	// The worst case occurs when executing the last message in a page of which all are skipped since it is using `peek_index` which has linear complexities.
-	execute_overweight_page_removed {
+	// The worst case occurs when executing the last message in a page of which all are skipped
+	// since it is using `peek_index` which has linear complexities.
+	#[benchmark]
+	fn execute_overweight_page_removed() {
 		let origin: MessageOriginOf<T> = 0.into();
 		let (mut page, msgs) = full_page::<T>();
 		// Skip all messages.
@@ -168,19 +212,34 @@ benchmarks! {
 		let book = book_for::<T>(&page);
 		Pages::<T>::insert(&origin, 0, &page);
 		BookStateFor::<T>::insert(&origin, &book);
-	}: {
-		MessageQueue::<T>::execute_overweight(RawOrigin::Signed(whitelisted_caller()).into(), 0u32.into(), 0u32, ((msgs - 1) as u32).into(), Weight::MAX).unwrap()
-	}
-	verify {
-		assert_last_event::<T>(Event::Processed {
-			hash: T::Hashing::hash(&((msgs - 1) as u32).encode()), origin: 0.into(),
-			weight_used: Weight::from_parts(1, 1), success: true
-		}.into());
+
+		#[block]
+		{
+			MessageQueue::<T>::execute_overweight(
+				RawOrigin::Signed(whitelisted_caller()).into(),
+				0u32.into(),
+				0u32,
+				((msgs - 1) as u32).into(),
+				Weight::MAX,
+			)
+			.unwrap();
+		}
+
+		assert_last_event::<T>(
+			Event::Processed {
+				hash: T::Hashing::hash(&((msgs - 1) as u32).encode()),
+				origin: 0.into(),
+				weight_used: Weight::from_parts(1, 1),
+				success: true,
+			}
+			.into(),
+		);
 		assert!(!Pages::<T>::contains_key(&origin, 0), "Page must be removed");
 	}
 
 	// Worst case for `execute_overweight` where the page is updated.
-	execute_overweight_page_updated {
+	#[benchmark]
+	fn execute_overweight_page_updated() {
 		let origin: MessageOriginOf<T> = 0.into();
 		let (mut page, msgs) = full_page::<T>();
 		// Skip all messages.
@@ -190,16 +249,34 @@ benchmarks! {
 		let book = book_for::<T>(&page);
 		Pages::<T>::insert(&origin, 0, &page);
 		BookStateFor::<T>::insert(&origin, &book);
-	}: {
-		MessageQueue::<T>::execute_overweight(RawOrigin::Signed(whitelisted_caller()).into(), 0u32.into(), 0u32, ((msgs - 1) as u32).into(), Weight::MAX).unwrap()
-	}
-	verify {
-		assert_last_event::<T>(Event::Processed {
-			hash: T::Hashing::hash(&((msgs - 1) as u32).encode()), origin: 0.into(),
-			weight_used: Weight::from_parts(1, 1), success: true
-		}.into());
+
+		#[block]
+		{
+			MessageQueue::<T>::execute_overweight(
+				RawOrigin::Signed(whitelisted_caller()).into(),
+				0u32.into(),
+				0u32,
+				((msgs - 1) as u32).into(),
+				Weight::MAX,
+			)
+			.unwrap();
+		}
+
+		assert_last_event::<T>(
+			Event::Processed {
+				hash: T::Hashing::hash(&((msgs - 1) as u32).encode()),
+				origin: 0.into(),
+				weight_used: Weight::from_parts(1, 1),
+				success: true,
+			}
+			.into(),
+		);
 		assert!(Pages::<T>::contains_key(&origin, 0), "Page must be updated");
 	}
 
-	impl_benchmark_test_suite!(MessageQueue, crate::mock::new_test_ext::<crate::integration_test::Test>(), crate::integration_test::Test);
+	impl_benchmark_test_suite! {
+		MessageQueue,
+		crate::mock::new_test_ext::<crate::integration_test::Test>(),
+		crate::integration_test::Test
+	}
 }