From ac2546b5c2050bee2c669e97505026d37c42d2e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bastian=20K=C3=B6cher?= <git@kchr.de>
Date: Wed, 13 Nov 2024 10:05:37 +0000
Subject: [PATCH] frame-benchmarking: Use correct components for pallet
instances (#6435)
When using multiple instances of the same pallet, each instance was
executed with the components of all instances. While actually each
instance should only be executed with the components generated for the
particular instance. The problem here was that in the runtime only the
pallet-name was used to determine if a certain pallet should be
benchmarked. When using instances, the pallet name is the same for both
of these instances. The solution is to also take the instance name into
account.
The fix requires to change the `Benchmark` runtime api to also take the
`instance`. The node side is written in a backwards compatible way to
also support runtimes which do not yet support the `instance` parameter.
---------
Co-authored-by: GitHub Action <action@github.com>
Co-authored-by: clangenb <37865735+clangenb@users.noreply.github.com>
Co-authored-by: Adrian Catangiu <adrian@parity.io>
---
Cargo.lock | 3 +
prdoc/pr_6435.prdoc | 16 ++++
substrate/client/db/src/lib.rs | 4 +-
substrate/frame/benchmarking/Cargo.toml | 6 ++
.../frame/benchmarking/src/tests_instance.rs | 61 +++++++++++-
substrate/frame/benchmarking/src/utils.rs | 3 +
substrate/frame/benchmarking/src/v1.rs | 3 +-
substrate/frame/referenda/Cargo.toml | 1 -
.../benchmarking-cli/src/pallet/command.rs | 92 +++++++++++++------
9 files changed, 155 insertions(+), 34 deletions(-)
create mode 100644 prdoc/pr_6435.prdoc
diff --git a/Cargo.lock b/Cargo.lock
index e36f252ecb3..f0a133227b3 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -6923,15 +6923,18 @@ dependencies = [
"parity-scale-codec",
"paste",
"rusty-fork",
+ "sc-client-db",
"scale-info",
"serde",
"sp-api 26.0.0",
"sp-application-crypto 30.0.0",
"sp-core 28.0.0",
+ "sp-externalities 0.25.0",
"sp-io 30.0.0",
"sp-keystore 0.34.0",
"sp-runtime 31.0.1",
"sp-runtime-interface 24.0.0",
+ "sp-state-machine 0.35.0",
"sp-storage 19.0.0",
"static_assertions",
]
diff --git a/prdoc/pr_6435.prdoc b/prdoc/pr_6435.prdoc
new file mode 100644
index 00000000000..025c666d911
--- /dev/null
+++ b/prdoc/pr_6435.prdoc
@@ -0,0 +1,16 @@
+title: 'frame-benchmarking: Use correct components for pallet instances'
+doc:
+- audience: Runtime Dev
+ description: |-
+ When benchmarking multiple instances of the same pallet, each instance was executed with the components of all instances. While actually each instance should only be executed with the components generated for the particular instance. The problem here was that in the runtime only the pallet-name was used to determine if a certain pallet should be benchmarked. When using instances, the pallet name is the same for both of these instances. The solution is to also take the instance name into account.
+
+ The fix requires to change the `Benchmark` runtime api to also take the `instance`. The node side is written in a backwards compatible way to also support runtimes which do not yet support the `instance` parameter.
+crates:
+- name: frame-benchmarking
+ bump: major
+- name: frame-benchmarking-cli
+ bump: major
+- name: sc-client-db
+ bump: none
+- name: pallet-referenda
+ bump: none
diff --git a/substrate/client/db/src/lib.rs b/substrate/client/db/src/lib.rs
index aaa1398a13b..cec981c0560 100644
--- a/substrate/client/db/src/lib.rs
+++ b/substrate/client/db/src/lib.rs
@@ -1180,7 +1180,7 @@ impl<Block: BlockT> Backend<Block> {
/// The second argument is the Column that stores the State.
///
/// Should only be needed for benchmarking.
- #[cfg(any(feature = "runtime-benchmarks"))]
+ #[cfg(feature = "runtime-benchmarks")]
pub fn expose_db(&self) -> (Arc<dyn sp_database::Database<DbHash>>, sp_database::ColumnId) {
(self.storage.db.clone(), columns::STATE)
}
@@ -1188,7 +1188,7 @@ impl<Block: BlockT> Backend<Block> {
/// Expose the Storage that is used by this backend.
///
/// Should only be needed for benchmarking.
- #[cfg(any(feature = "runtime-benchmarks"))]
+ #[cfg(feature = "runtime-benchmarks")]
pub fn expose_storage(&self) -> Arc<dyn sp_state_machine::Storage<HashingFor<Block>>> {
self.storage.clone()
}
diff --git a/substrate/frame/benchmarking/Cargo.toml b/substrate/frame/benchmarking/Cargo.toml
index 9ea350a1d29..0c74d94b33b 100644
--- a/substrate/frame/benchmarking/Cargo.toml
+++ b/substrate/frame/benchmarking/Cargo.toml
@@ -38,6 +38,9 @@ static_assertions = { workspace = true, default-features = true }
array-bytes = { workspace = true, default-features = true }
rusty-fork = { workspace = true }
sp-keystore = { workspace = true, default-features = true }
+sc-client-db = { workspace = true }
+sp-state-machine = { workspace = true }
+sp-externalities = { workspace = true }
[features]
default = ["std"]
@@ -53,14 +56,17 @@ std = [
"sp-api/std",
"sp-application-crypto/std",
"sp-core/std",
+ "sp-externalities/std",
"sp-io/std",
"sp-keystore/std",
"sp-runtime-interface/std",
"sp-runtime/std",
+ "sp-state-machine/std",
"sp-storage/std",
]
runtime-benchmarks = [
"frame-support/runtime-benchmarks",
"frame-system/runtime-benchmarks",
+ "sc-client-db/runtime-benchmarks",
"sp-runtime/runtime-benchmarks",
]
diff --git a/substrate/frame/benchmarking/src/tests_instance.rs b/substrate/frame/benchmarking/src/tests_instance.rs
index ecffbd1a018..428f29e2bc1 100644
--- a/substrate/frame/benchmarking/src/tests_instance.rs
+++ b/substrate/frame/benchmarking/src/tests_instance.rs
@@ -61,6 +61,7 @@ mod pallet_test {
#[pallet::weight({0})]
pub fn set_value(origin: OriginFor<T>, n: u32) -> DispatchResult {
let _sender = ensure_signed(origin)?;
+ assert!(n >= T::LowerBound::get());
Value::<T, I>::put(n);
Ok(())
}
@@ -81,6 +82,7 @@ frame_support::construct_runtime!(
{
System: frame_system,
TestPallet: pallet_test,
+ TestPallet2: pallet_test::<Instance2>,
}
);
@@ -117,6 +119,12 @@ impl pallet_test::Config for Test {
type UpperBound = ConstU32<100>;
}
+impl pallet_test::Config<pallet_test::Instance2> for Test {
+ type RuntimeEvent = RuntimeEvent;
+ type LowerBound = ConstU32<50>;
+ type UpperBound = ConstU32<100>;
+}
+
impl pallet_test::OtherConfig for Test {
type OtherEvent = RuntimeEvent;
}
@@ -130,6 +138,7 @@ mod benchmarks {
use crate::account;
use frame_support::ensure;
use frame_system::RawOrigin;
+ use sp_core::Get;
// Additional used internally by the benchmark macro.
use super::pallet_test::{Call, Config, Pallet};
@@ -143,7 +152,7 @@ mod benchmarks {
}
set_value {
- let b in 1 .. 1000;
+ let b in ( <T as Config<I>>::LowerBound::get() ) .. ( <T as Config<I>>::UpperBound::get() );
let caller = account::<T::AccountId>("caller", 0, 0);
}: _ (RawOrigin::Signed(caller), b.into())
verify {
@@ -173,3 +182,53 @@ mod benchmarks {
)
}
}
+
+#[test]
+fn ensure_correct_instance_is_selected() {
+ use crate::utils::Benchmarking;
+
+ crate::define_benchmarks!(
+ [pallet_test, TestPallet]
+ [pallet_test, TestPallet2]
+ );
+
+ let whitelist = vec![];
+
+ let mut batches = Vec::<crate::BenchmarkBatch>::new();
+ let config = crate::BenchmarkConfig {
+ pallet: "pallet_test".bytes().collect::<Vec<_>>(),
+ // We only want that this `instance` is used.
+ // Otherwise the wrong components are used.
+ instance: "TestPallet".bytes().collect::<Vec<_>>(),
+ benchmark: "set_value".bytes().collect::<Vec<_>>(),
+ selected_components: TestPallet::benchmarks(false)
+ .into_iter()
+ .find_map(|b| {
+ if b.name == "set_value".as_bytes() {
+ Some(b.components.into_iter().map(|c| (c.0, c.1)).collect::<Vec<_>>())
+ } else {
+ None
+ }
+ })
+ .unwrap(),
+ verify: false,
+ internal_repeats: 1,
+ };
+ let params = (&config, &whitelist);
+
+ let state = sc_client_db::BenchmarkingState::<sp_runtime::traits::BlakeTwo256>::new(
+ Default::default(),
+ None,
+ false,
+ false,
+ )
+ .unwrap();
+
+ let mut overlay = Default::default();
+ let mut ext = sp_state_machine::Ext::new(&mut overlay, &state, None);
+ sp_externalities::set_and_run_with_externalities(&mut ext, || {
+ add_benchmarks!(params, batches);
+ Ok::<_, crate::BenchmarkError>(())
+ })
+ .unwrap();
+}
diff --git a/substrate/frame/benchmarking/src/utils.rs b/substrate/frame/benchmarking/src/utils.rs
index fb55cee99e8..3a10e43d83b 100644
--- a/substrate/frame/benchmarking/src/utils.rs
+++ b/substrate/frame/benchmarking/src/utils.rs
@@ -200,6 +200,8 @@ impl From<DispatchError> for BenchmarkError {
pub struct BenchmarkConfig {
/// The encoded name of the pallet to benchmark.
pub pallet: Vec<u8>,
+ /// The encoded name of the pallet instance to benchmark.
+ pub instance: Vec<u8>,
/// The encoded name of the benchmark/extrinsic to run.
pub benchmark: Vec<u8>,
/// The selected component values to use when running the benchmark.
@@ -229,6 +231,7 @@ pub struct BenchmarkMetadata {
sp_api::decl_runtime_apis! {
/// Runtime api for benchmarking a FRAME runtime.
+ #[api_version(2)]
pub trait Benchmark {
/// Get the benchmark metadata available for this runtime.
///
diff --git a/substrate/frame/benchmarking/src/v1.rs b/substrate/frame/benchmarking/src/v1.rs
index e73ed1f4382..64f93b22cf1 100644
--- a/substrate/frame/benchmarking/src/v1.rs
+++ b/substrate/frame/benchmarking/src/v1.rs
@@ -1821,12 +1821,13 @@ macro_rules! add_benchmark {
let (config, whitelist) = $params;
let $crate::BenchmarkConfig {
pallet,
+ instance,
benchmark,
selected_components,
verify,
internal_repeats,
} = config;
- if &pallet[..] == &name_string[..] {
+ if &pallet[..] == &name_string[..] && &instance[..] == &instance_string[..] {
let benchmark_result = <$location>::run_benchmark(
&benchmark[..],
&selected_components[..],
diff --git a/substrate/frame/referenda/Cargo.toml b/substrate/frame/referenda/Cargo.toml
index 32dba343659..e9b4eb04ed5 100644
--- a/substrate/frame/referenda/Cargo.toml
+++ b/substrate/frame/referenda/Cargo.toml
@@ -57,7 +57,6 @@ std = [
]
runtime-benchmarks = [
"assert_matches",
- "frame-benchmarking",
"frame-benchmarking/runtime-benchmarks",
"frame-support/runtime-benchmarks",
"frame-system/runtime-benchmarks",
diff --git a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs
index 6f7e79f1638..0c068fc585b 100644
--- a/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs
+++ b/substrate/utils/frame/benchmarking-cli/src/pallet/command.rs
@@ -96,6 +96,7 @@ pub(crate) type PovModesMap =
#[derive(Debug, Clone)]
struct SelectedBenchmark {
pallet: String,
+ instance: String,
extrinsic: String,
components: Vec<(BenchmarkParameter, u32, u32)>,
pov_modes: Vec<(String, String)>,
@@ -152,7 +153,7 @@ fn combine_batches(
}
/// Explains possible reasons why the metadata for the benchmarking could not be found.
-const ERROR_METADATA_NOT_FOUND: &'static str = "Did not find the benchmarking metadata. \
+const ERROR_API_NOT_FOUND: &'static str = "Did not find the benchmarking runtime api. \
This could mean that you either did not build the node correctly with the \
`--features runtime-benchmarks` flag, or the chain spec that you are using was \
not created by a node that was compiled with the flag";
@@ -306,6 +307,33 @@ impl PalletCmd {
.with_runtime_cache_size(2)
.build();
+ let runtime_version: sp_version::RuntimeVersion = Self::exec_state_machine(
+ StateMachine::new(
+ state,
+ &mut Default::default(),
+ &executor,
+ "Core_version",
+ &[],
+ &mut Self::build_extensions(executor.clone(), state.recorder()),
+ &runtime_code,
+ CallContext::Offchain,
+ ),
+ "Could not find `Core::version` runtime api.",
+ )?;
+
+ let benchmark_api_version = runtime_version
+ .api_version(
+ &<dyn frame_benchmarking::Benchmark<
+ // We need to use any kind of `Block` type to make the compiler happy, not
+ // relevant for the `ID`.
+ sp_runtime::generic::Block<
+ sp_runtime::generic::Header<u32, Hasher>,
+ sp_runtime::generic::UncheckedExtrinsic<(), (), (), ()>,
+ >,
+ > as sp_api::RuntimeApiInfo>::ID,
+ )
+ .ok_or_else(|| ERROR_API_NOT_FOUND)?;
+
let (list, storage_info): (Vec<BenchmarkList>, Vec<StorageInfo>) =
Self::exec_state_machine(
StateMachine::new(
@@ -318,7 +346,7 @@ impl PalletCmd {
&runtime_code,
CallContext::Offchain,
),
- ERROR_METADATA_NOT_FOUND,
+ ERROR_API_NOT_FOUND,
)?;
// Use the benchmark list and the user input to determine the set of benchmarks to run.
@@ -338,7 +366,7 @@ impl PalletCmd {
let pov_modes = Self::parse_pov_modes(&benchmarks_to_run)?;
let mut failed = Vec::<(String, String)>::new();
- 'outer: for (i, SelectedBenchmark { pallet, extrinsic, components, .. }) in
+ 'outer: for (i, SelectedBenchmark { pallet, instance, extrinsic, components, .. }) in
benchmarks_to_run.clone().into_iter().enumerate()
{
log::info!(
@@ -392,7 +420,31 @@ impl PalletCmd {
}
all_components
};
+
for (s, selected_components) in all_components.iter().enumerate() {
+ let params = |verify: bool, repeats: u32| -> Vec<u8> {
+ if benchmark_api_version >= 2 {
+ (
+ pallet.as_bytes(),
+ instance.as_bytes(),
+ extrinsic.as_bytes(),
+ &selected_components.clone(),
+ verify,
+ repeats,
+ )
+ .encode()
+ } else {
+ (
+ pallet.as_bytes(),
+ extrinsic.as_bytes(),
+ &selected_components.clone(),
+ verify,
+ repeats,
+ )
+ .encode()
+ }
+ };
+
// First we run a verification
if !self.no_verify {
let state = &state_without_tracking;
@@ -407,14 +459,7 @@ impl PalletCmd {
&mut Default::default(),
&executor,
"Benchmark_dispatch_benchmark",
- &(
- pallet.as_bytes(),
- extrinsic.as_bytes(),
- &selected_components.clone(),
- true, // run verification code
- 1, // no need to do internal repeats
- )
- .encode(),
+ ¶ms(true, 1),
&mut Self::build_extensions(executor.clone(), state.recorder()),
&runtime_code,
CallContext::Offchain,
@@ -447,14 +492,7 @@ impl PalletCmd {
&mut Default::default(),
&executor,
"Benchmark_dispatch_benchmark",
- &(
- pallet.as_bytes(),
- extrinsic.as_bytes(),
- &selected_components.clone(),
- false, // don't run verification code for final values
- self.repeat,
- )
- .encode(),
+ ¶ms(false, self.repeat),
&mut Self::build_extensions(executor.clone(), state.recorder()),
&runtime_code,
CallContext::Offchain,
@@ -489,14 +527,7 @@ impl PalletCmd {
&mut Default::default(),
&executor,
"Benchmark_dispatch_benchmark",
- &(
- pallet.as_bytes(),
- extrinsic.as_bytes(),
- &selected_components.clone(),
- false, // don't run verification code for final values
- self.repeat,
- )
- .encode(),
+ ¶ms(false, self.repeat),
&mut Self::build_extensions(executor.clone(), state.recorder()),
&runtime_code,
CallContext::Offchain,
@@ -571,6 +602,7 @@ impl PalletCmd {
{
benchmarks_to_run.push((
item.pallet.clone(),
+ item.instance.clone(),
benchmark.name.clone(),
benchmark.components.clone(),
benchmark.pov_modes.clone(),
@@ -581,13 +613,15 @@ impl PalletCmd {
// Convert `Vec<u8>` to `String` for better readability.
let benchmarks_to_run: Vec<_> = benchmarks_to_run
.into_iter()
- .map(|(pallet, extrinsic, components, pov_modes)| {
- let pallet = String::from_utf8(pallet.clone()).expect("Encoded from String; qed");
+ .map(|(pallet, instance, extrinsic, components, pov_modes)| {
+ let pallet = String::from_utf8(pallet).expect("Encoded from String; qed");
+ let instance = String::from_utf8(instance).expect("Encoded from String; qed");
let extrinsic =
String::from_utf8(extrinsic.clone()).expect("Encoded from String; qed");
SelectedBenchmark {
pallet,
+ instance,
extrinsic,
components,
pov_modes: pov_modes
--
GitLab