generation of real benchmark functions for benchmarking v2

[sam0x17]

TLDR: the purpose of this is to get some useful compiler errors and warnings if people try to do bad things inside of benchmark function defs, and we accomplish this by actually generating real functions from the defs that they write. These functions aren't actually used for anything, though the programmer is free to call them if they wish. The underlying benchmarking impls take code that is essentially copy pasted from these function defs to fill in a number of impls i.e. the setup, call, and verification sections end up inside several closures in trait impls. See #13277 and the related issues below to understand why real functions are desirable.

This has been re-opened because several other issues, including #13277 could be resolved by generating benchmark function definitions.

Right now, we use function definitions like this to define benchmarks in the benchmarking v2 syntax:

#[benchmark]
fn force_unreserve() {
	let user: T::AccountId = account("user", 0, SEED);
	let user_lookup = T::Lookup::unlookup(user.clone());

	// Give some multiple of the existential deposit
	let existential_deposit = T::ExistentialDeposit::get();
	let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
	let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance);

	// Reserve the balance
	<Balances<T, I> as ReservableCurrency<_>>::reserve(&user, balance)?;
	assert_eq!(Balances::<T, I>::reserved_balance(&user), balance);
	assert!(Balances::<T, I>::free_balance(&user).is_zero());

	#[extrinsic_call]
	_(RawOrigin::Root, user_lookup, balance);

	assert!(Balances::<T, I>::reserved_balance(&user).is_zero());
	assert_eq!(Balances::<T, I>::free_balance(&user), balance);
}

Currently this compiles (even though there is a ? operator but our function doesn't return Result) because the return types on the benchmark function defs are ignored by the benchmarking macros, because there isn't actually a real function that gets generated, instead the setup, extrinsic call, and verification sections of this code get put into trait impls that allow this to compile. In fact right now you can put any return type and it will compile as long as it is parsable, even if you use non-existent types, as noted by #13278.

In #13277 we attempted to alleviate this by requiring certain return types in certain scenarios and manually adding a compile error if the ? operator is used with an incompatible return-type, but this had edge cases that were difficult to handle (i.e. people returning Err manually, etc) that frankly should be handled by the compiler.

Another issue that arose because of this is right now we do not get warnings if a benchmark parameter is not used in the function body, but we definitely want these warnings (#13303)

The solution for all of these issues is to actually generate real function definitions, which is what this PR sets out to do.

Progress

• Basic generation of benchmark function defs
• Generation of working params (x, y, etc)
• Addition of verify param to generated function signature
• Ability to use generic BenchmarkResult<T> when we want to return a result and/or use ? operator
• Support blank () returns so existing benchmarks work unmodified
• Handling of where clause
• Confirm existing UI tests still work
• Additional UI tests
• retain attrs and doc comments
• Support paths leading to BenchmarkResult and BenchmarkError instead of just BenchmarkResult and BenchmarkError
• more UI tests for the above item
• Update docs
• Remove v2::BenchmarkResult and just stick with Result<T, BenchmarkError> because this name otherwise collides with the existing BenchmarkResult used by the internal benchmarking code, causing confusion.
• Fix lingering CI failure / https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2394138
• Remove Result<T, BenchmarkError> syntax and just stick with Result<(), BenchmarkError> based on feedback and potential WASM boundary issues

Related Issues

• part of [FRAME core] ☂ Benchmark syntax V2 follow-ups polkadot-sdk#380
• fixes benchmarking v2 syntax is confusingly lax about return types, leading to confusion #13278
• fixes benchmarking v2 syntax does not issue warnings on unused benchmark params #13303
• fixes Allow /// comments on benchmarks. #13383

[sam0x17]

Re-opening because of #13277 (comment)

[sam0x17]

Interesting edge case @ggwpez:

Let's consider the benchmark function def from the balances pallet mentioned above. I think we would expect to be able to write it like this:

#[benchmark]
fn force_unreserve() -> BenchmarkResult {
	let user: T::AccountId = account("user", 0, SEED);
	let user_lookup = T::Lookup::unlookup(user.clone());

	// Give some multiple of the existential deposit
	let existential_deposit = T::ExistentialDeposit::get();
	let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
	let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance);

	// Reserve the balance
	<Balances<T, I> as ReservableCurrency<_>>::reserve(&user, balance)?;
	assert_eq!(Balances::<T, I>::reserved_balance(&user), balance);
	assert!(Balances::<T, I>::free_balance(&user).is_zero());

	#[extrinsic_call]
	_(RawOrigin::Root, user_lookup, balance);

	assert!(Balances::<T, I>::reserved_balance(&user).is_zero());
	assert_eq!(Balances::<T, I>::free_balance(&user), balance);

        Ok(())
}

The problem is we actually can't do the Ok(()) because the trait impl expansion for the verify statements looks like this:

Ok(#krate::Box::new(move || -> Result<(), #krate::BenchmarkError> {
	#post_call
	if verify {
		#(
			#verify_stmts
		)*
	}
	Ok(())
}))

So if the last line of verify_stmts is Ok(()), we'll get an error because we have two Ok(())s in a row.

There are several workarounds we could do:

1. we could require a BenchmarkResult compatible return type on all benchmark function definitions and insert the Ok(()) at the end of the benchmark function def as part of the expansion (after the verify if statement), like we already do with the verify part of the impl. Then the programmer would not need to write Ok(()) at the end of every benchmark function def, and every benchmark function def would be -> BenchmarkResult or something compatible with that. This option might be confusing though since it would appear that you aren't returning Ok(()) to the programmer.
2. We could require a BenchmarkResult compatible return type on all benchmark function definitions, expect the user to end every benchmark function def with Ok(()), and remove the Ok(()) that the macro expansion code inserts at the end of the verify statements in the impl. We would also change the verify expansion code above to do } else { Ok(()) } so both paths return an Ok(()) regardless of whether verify is true.
3. We could allow blank return type or a custom return type and remove the macro-inserted Ok(()) line from the verify section as with option (2). Any custom return type provided by the programmer would have to be compatible with Result<T, BenchmarkError> so for this option I would just opt for BenchmarkResult<T>. For this to work we would have to extract the last line of the benchmark function def and treat this as the return value and insert it after the if verify if there is a non-blank return-type on the function def. When there is a blank return type, we would still insert the Ok(()) automatically in the verify impl.

Option 3 is the hardest to implement and has the most flexibility and best dev UX imo. Option 2 is more awkward and less flexible but would still be a good option. Option 1 is great other than the strange fact that the programmer doesn't need to (and in fact can't) add the Ok(()) to the end of their benchmark function def.

Ultimately I like option 3 the most because it opens up the possibility for the programmer to return a custom type and actually do something with the benchmark function def, but also allows the programmer to be lazy and just have a blank return for simple benchmarks, so this is what I would recommend.

Option 3 would look like this for force_unreserved:

#[benchmark]
fn force_unreserve() -> BenchmarkResult<()> {
	let user: T::AccountId = account("user", 0, SEED);
	let user_lookup = T::Lookup::unlookup(user.clone());

	// Give some multiple of the existential deposit
	let existential_deposit = T::ExistentialDeposit::get();
	let balance = existential_deposit.saturating_mul(ED_MULTIPLIER.into());
	let _ = <Balances<T, I> as Currency<_>>::make_free_balance_be(&user, balance);

	// Reserve the balance
	<Balances<T, I> as ReservableCurrency<_>>::reserve(&user, balance)?;
	assert_eq!(Balances::<T, I>::reserved_balance(&user), balance);
	assert!(Balances::<T, I>::free_balance(&user).is_zero());

	#[extrinsic_call]
	_(RawOrigin::Root, user_lookup, balance);

	assert!(Balances::<T, I>::reserved_balance(&user).is_zero());
	assert_eq!(Balances::<T, I>::free_balance(&user), balance);

        Ok(())
}

Other benchmark function defs would be able to remain the way they are since blank return would be supported.

[ggwpez]

Yea I think having blank return or possible Result is nice as UX so you are not forced to write Ok(()).

PS: Dont know how. My example code does not work.

[sam0x17]

Ooh that’s a good idea I will try a closure

…

On Mon, Feb 6, 2023 at 11:00 AM Oliver Tale-Yazdi ***@***.***> wrote: Yea I think having blank return or possible Result is nice as UX so you are not forced to write Ok(()). Can you not wrap the verify block in a closure and use ? on it like: #post_callif verify { || { #( #verify_stmts )* }()?;}Ok(()) or however the exact syntax would look like. — Reply to this email directly, view it on GitHub <#13224 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAOE4LJ5NAOHIA3DNHUKB6DWWENZDANCNFSM6AAAAAAUEPDBMI> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>

[sam0x17]

Yea I think having blank return or possible Result is nice as UX so you are not forced to write Ok(()).

PS: Dont know how. My example code does not work.

Yeah I've tried a variety of things and this is actually quite hard to work around.

Generally speaking I think I want to optimize for maximum flexibility here because I'm certain someone is going to come out of the woodwork someday who actually wants to call these benchmark function defs and return something other than (), so we should be able to do BenchmarkResult<T>.

So something like the following:

Programmer would write this (using String as a custom return type for example purposes):

#[benchmark]
fn force_unreserve() -> BenchmarkResult<String> {
    something()?;
    let thing = "something";
    #[block]
    {}
    assert_eq!(2 + 2, 4);
    assert_eq!(4, 2 + 2);
    Ok(thing)
}

And we'd do this for the function def expansion:

let function_def = quote! {
	#vis #sig {
		#(
			#setup_stmts
		)*
		#extrinsic_call (or block)
		if verify {
			#(
				#verify_stmts
			)*
		}
		#last_item_of_function_def
	}
};

By convention the last statement of the function def would have to be the return value, though custom early returns via the return keyword would be possible as long as they return a compatible type. This last statement could be something like a match or whatever and it would still work.

Now if someone tried to create a value in the verify block and return it on the last line, this would (rightly) create a compile error telling them they are returning a potentially uninitialized value. So if they want to return some custom value, they should be setting it in the setup or extrinsic call / block section of the function def.

And this way I could still inject verify: bool as the last argument to the generated function def so people can decide when they call it whether they want the verify stuff to run or not without affecting the return value.

I think this should be the all-around best compromise UX-wise after going through all of the alternatives. Thoughts @ggwpez ?

[sam0x17]

What I lay out above is working on the balances pallet, and as a bonus I was able to get blank returns types () to still work so we get the best of both worlds in that you only need to specify -> BenchmarkResult<()> and add an Ok(()) at the end if you return Err or use ? somewhere in the benchmark. So existing benchmarks should compile as-is unless they use ?.

Still doing some tweaks to get where clauses to work and then need to update/check over the ui tests and add a few.

[ggwpez]

Yea could work by using the last line. I guess that would be a good solution - UX wise.
I dont really mind how we implement it, so just try out what you need to 😄

[sam0x17]

Yea could work by using the last line. I guess that would be a good solution - UX wise. I dont really mind how we implement it, so just try out what you need to 😄

Yeah will do. It seems to work well and since it's the last statement, we can again have more complex things there like a match etc if someone really needs to

[sam0x17]

note the current code isn't producing the extrinsic call / block properly in the function def working on that now

edit: should be good now

[sam0x17]

This is fully working but doing one more thing to support paths that might lead to BenchmarkResult instead of just verbatim BenchmarkResult. Same with BenchmarkError.

[ggwpez]

They are not really functions, so having any kind of non-benchmarking attribute is actually an error, or?
Since FRAME does not interpret them accordingly.

[sam0x17]

Well the function def we generate in the expansion is real, it's just the visibility only applies to it, not to any of the other stuff we generate, which I think is reasonable. The alternative would be ignoring it (which would be confusing to the programmer when they try to set it to be pub or not pub and the generated function doesn't track with that), or restricting it to be a certain thing (pub or non pub), which I think would be needlessly restrictive.

[ggwpez]

Hm my comment here was about attribute macros on the benchmarking function. Like should_panic or whatever would not make sense.

[sam0x17]

Oh right, sorry. I would make the same argument, just about attributes:

I would lean towards just letting all attributes through here. Sure in some cases it might not make sense, but I think it would be more confusing if the programmer tries to use an attribute and it is ignored or was specifically blocked.

Alternatively, we could issue a compiler error for any non-doc attributes.

Right now as far as I know we don't do any special blocking of non-pallet macros on pallet items, and I view this scenario as similar. We could open a new issue to fix this in a bunch of places if desirable

[ggwpez]

Alternatively, we could issue a compiler error for any non-doc attributes.

That is what i mean.

[ggwpez]

Thanks! We can discuss the open discussion afterwards.

[sam0x17]

bot merge