Benchmark Improvements: Lingering Variables and _{ } syntax

[shawntabrizi]

Two small issues I am seeing while writing benchmarks. First is that the very first result from the benchmarks is always an outlier.

Here are some examples:

1,46000,30000
1,17000,17000
1,18000,16000

1,56000,36000
1,17000,17000
1,16000,16000

1,51000,31000
1,18000,16000
1,17000,17000

We need to run our benchmark once but record nothing, and then actually do the benchmarks. We should treat this warm up cycle as extra to the number of cycles total we want to run. It would also be good to identify where this initial overhead is coming from.

There is also an issue with the underlying macro which generates the benchmark.

Here is the benchmark code:

	_ {
		let r in 1 .. MAX_REGISTRARS => add_registrars::<T>(r)?;
		let s in 1 .. T::MaxSubAccounts::get() => {
			// Give them s many sub accounts
			let caller = account::<T>("caller", 0);
			let _ = add_sub_accounts::<T>(caller, s)?;
		};
		let x in 1 .. T::MaxAdditionalFields::get() => {
			// Create their main identity with x additional fields
			let info = create_identity_info::<T>(x);
			let caller = account::<T>("caller", 0);
			let caller_origin = <T as frame_system::Trait>::Origin::from(RawOrigin::Signed(caller));
			Identity::<T>::set_identity(caller_origin, info)?;
		};
	}

	add_registrar {
		let r in ...;
	}: _(RawOrigin::Root, account::<T>("registrar", r + 1))

And here is the expanded:

   #[allow(non_camel_case_types)]
    struct add_registrar;
    #[allow(unused_variables)]
    impl<T: Trait> ::frame_benchmarking::BenchmarkingSetup<T> for add_registrar {
        fn components(&self) -> Vec<(::frame_benchmarking::BenchmarkParameter, u32, u32)> {
            <[_]>::into_vec(box [(
                ::frame_benchmarking::BenchmarkParameter::r,
                {
                    let r = 1;
                    let s = 1;
                    let x = 1;
                    r
                },
                ({
                    let r = MAX_REGISTRARS;
                    let s = T::MaxSubAccounts::get();
                    let x = T::MaxAdditionalFields::get();
                    r
                }),
            )])
        }
        fn instance(
            &self,
            components: &[(::frame_benchmarking::BenchmarkParameter, u32)],
        ) -> Result<Box<dyn FnOnce() -> Result<(), &'static str>>, &'static str> {
            let r = 1;
            let s = 1;
            let x = 1;
            let r = components
                .iter()
                .find(|&c| c.0 == ::frame_benchmarking::BenchmarkParameter::r)
                .unwrap()
                .1;
            ({
                let r = || -> Result<(), &'static str> {
                    add_registrars::<T>(r)?;
                    Ok(())
                };
                let s = || -> Result<(), &'static str> {
                    {
                        let caller = account::<T>("caller", 0);
                        let _ = add_sub_accounts::<T>(caller, s)?;
                    };
                    Ok(())
                };
                let x = || -> Result<(), &'static str> {
                    {
                        let info = create_identity_info::<T>(x);
                        let caller = account::<T>("caller", 0);
                        let caller_origin =
                            <T as frame_system::Trait>::from(RawOrigin::Signed(caller));
                        Identity::<T>::set_identity(caller_origin, info)?;
                    };
                    Ok(())
                };
                r()?
            });
            Ok(Box::new(move || -> Result<(), &'static str> {
                {
                    <Call<T> as ::frame_benchmarking::Dispatchable>::dispatch(
                        Call::<T>::add_registrar(account::<T>("registrar", r + 1)),
                        RawOrigin::Root.into(),
                    )?;
                };
                Ok(())
            }))
        }
    }

Note there are some common parameters defined, but only r is used in the benchmark. However the expanded code includes all the other variables and even some code blocks which are not used.

One intention could be that we want all of these storage functions to run for every benchmark, whether or not we use them. If that is the case, then the default value should be the MAX value.

Otherwise, these unused variables should not appear (this is what I think we should do).

The problem here is that these variables are added completely hidden to the user, and the user may use some variable which is already defined, the Rust would otherwise hide the error.