From ad7436874b1d54945ee18cffbe7cdb3c6dd02e22 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 3 Dec 2020 22:46:34 -0800 Subject: [PATCH 1/6] Support multiple instances with benchmarks --- Cargo.lock | 1 + frame/benchmarking/src/lib.rs | 11 +++++--- frame/benchmarking/src/utils.rs | 2 ++ utils/frame/benchmarking-cli/Cargo.toml | 4 ++- utils/frame/benchmarking-cli/src/writer.rs | 29 +++++++++++++++------- 5 files changed, 33 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index eb98e96a8894d..c4c27f2fcc176 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1546,6 +1546,7 @@ dependencies = [ name = "frame-benchmarking-cli" version = "2.0.0" dependencies = [ + "Inflector", "chrono", "frame-benchmarking", "handlebars", diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index fdfe857e4be3b..8bea2600e1d52 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1056,6 +1056,7 @@ macro_rules! impl_benchmark_test { macro_rules! add_benchmark { ( $params:ident, $batches:ident, $name:ident, $( $location:tt )* ) => ( let name_string = stringify!($name).as_bytes(); + let instance_string = stringify!($( $location )* ).as_bytes(); let (config, whitelist) = $params; let $crate::BenchmarkConfig { pallet, @@ -1071,6 +1072,9 @@ macro_rules! add_benchmark { if &pallet[..] == &b"*"[..] || &benchmark[..] == &b"*"[..] { for benchmark in $( $location )*::benchmarks(*extra).into_iter() { $batches.push($crate::BenchmarkBatch { + pallet: name_string.to_vec(), + instance: instance_string.to_vec(), + benchmark: benchmark.to_vec(), results: $( $location )*::run_benchmark( benchmark, &lowest_range_values[..], @@ -1080,12 +1084,13 @@ macro_rules! add_benchmark { whitelist, *verify, )?, - pallet: name_string.to_vec(), - benchmark: benchmark.to_vec(), }); } } else { $batches.push($crate::BenchmarkBatch { + pallet: name_string.to_vec(), + instance: instance_string.to_vec(), + benchmark: benchmark.clone(), results: $( $location )*::run_benchmark( &benchmark[..], &lowest_range_values[..], @@ -1095,8 +1100,6 @@ macro_rules! add_benchmark { whitelist, *verify, )?, - pallet: name_string.to_vec(), - benchmark: benchmark.clone(), }); } } diff --git a/frame/benchmarking/src/utils.rs b/frame/benchmarking/src/utils.rs index 042f4b707aef4..2c2aee910e364 100644 --- a/frame/benchmarking/src/utils.rs +++ b/frame/benchmarking/src/utils.rs @@ -43,6 +43,8 @@ impl std::fmt::Display for BenchmarkParameter { pub struct BenchmarkBatch { /// The pallet containing this benchmark. pub pallet: Vec, + /// The instance of this pallet being benchmarked. + pub instance: Vec, /// The extrinsic (or benchmark name) of this benchmark. pub benchmark: Vec, /// The results from this benchmark. diff --git a/utils/frame/benchmarking-cli/Cargo.toml b/utils/frame/benchmarking-cli/Cargo.toml index 4ee2454e708e9..2413d3d18d76c 100644 --- a/utils/frame/benchmarking-cli/Cargo.toml +++ b/utils/frame/benchmarking-cli/Cargo.toml @@ -23,11 +23,13 @@ sp-externalities = { version = "0.8.0", path = "../../../primitives/externalitie sp-keystore = { version = "0.8.0", path = "../../../primitives/keystore" } sp-runtime = { version = "2.0.0", path = "../../../primitives/runtime" } sp-state-machine = { version = "0.8.0", path = "../../../primitives/state-machine" } -structopt = "0.3.8" codec = { version = "1.3.1", package = "parity-scale-codec" } +structopt = "0.3.8" chrono = "0.4" serde = "1.0.116" handlebars = "3.5.0" +Inflector = "0.11.4" + [features] default = ["db"] diff --git a/utils/frame/benchmarking-cli/src/writer.rs b/utils/frame/benchmarking-cli/src/writer.rs index 61423000231d0..3000db21b36bf 100644 --- a/utils/frame/benchmarking-cli/src/writer.rs +++ b/utils/frame/benchmarking-cli/src/writer.rs @@ -22,6 +22,7 @@ use std::fs; use std::path::PathBuf; use serde::Serialize; +use inflector::Inflector; use crate::BenchmarkCmd; use frame_benchmarking::{BenchmarkBatch, BenchmarkSelector, Analysis}; @@ -37,6 +38,7 @@ struct TemplateData { date: String, version: String, pallet: String, + instance: String, header: String, cmd: CmdData, benchmarks: Vec, @@ -100,7 +102,7 @@ fn io_error(s: &str) -> std::io::Error { // p1 -> [b1, b2, b3] // p2 -> [b1, b2] // ``` -fn map_results(batches: &[BenchmarkBatch]) -> Result>, std::io::Error> { +fn map_results(batches: &[BenchmarkBatch]) -> Result>, std::io::Error> { // Skip if batches is empty. if batches.is_empty() { return Err(io_error("empty batches")) } @@ -113,6 +115,7 @@ fn map_results(batches: &[BenchmarkBatch]) -> Result Result Date: Thu, 3 Dec 2020 23:18:26 -0800 Subject: [PATCH 2/6] fix tests --- utils/frame/benchmarking-cli/src/writer.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/utils/frame/benchmarking-cli/src/writer.rs b/utils/frame/benchmarking-cli/src/writer.rs index 3000db21b36bf..b7c44ae1fb652 100644 --- a/utils/frame/benchmarking-cli/src/writer.rs +++ b/utils/frame/benchmarking-cli/src/writer.rs @@ -373,6 +373,7 @@ mod test { return BenchmarkBatch { pallet: [pallet.to_vec(), b"_pallet".to_vec()].concat(), + instance: b"instance".to_vec(), benchmark: [benchmark.to_vec(), b"_benchmark".to_vec()].concat(), results, } @@ -413,15 +414,21 @@ mod test { test_data(b"second", b"first", BenchmarkParameter::c, 3, 4), ]).unwrap(); - let first_benchmark = &mapped_results.get("first_pallet").unwrap()[0]; + let first_benchmark = &mapped_results.get( + &("first_pallet".to_string(), "instance".to_string()) + ).unwrap()[0]; assert_eq!(first_benchmark.name, "first_benchmark"); check_data(first_benchmark, "a", 10, 3); - let second_benchmark = &mapped_results.get("first_pallet").unwrap()[1]; + let second_benchmark = &mapped_results.get( + &("first_pallet".to_string(), "instance".to_string()) + ).unwrap()[1]; assert_eq!(second_benchmark.name, "second_benchmark"); check_data(second_benchmark, "b", 9, 2); - let second_pallet_benchmark = &mapped_results.get("second_pallet").unwrap()[0]; + let second_pallet_benchmark = &mapped_results.get( + &("second_pallet".to_string(), "instance".to_string()) + ).unwrap()[0]; assert_eq!(second_pallet_benchmark.name, "first_benchmark"); check_data(second_pallet_benchmark, "c", 3, 4); } From 8f59f465b06dac2f833a7314994e5a30abe5b6df Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 4 Dec 2020 13:28:56 -0800 Subject: [PATCH 3/6] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- frame/benchmarking/src/lib.rs | 2 +- utils/frame/benchmarking-cli/Cargo.toml | 1 - utils/frame/benchmarking-cli/src/writer.rs | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 8bea2600e1d52..f0e665bffaeaf 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1056,7 +1056,7 @@ macro_rules! impl_benchmark_test { macro_rules! add_benchmark { ( $params:ident, $batches:ident, $name:ident, $( $location:tt )* ) => ( let name_string = stringify!($name).as_bytes(); - let instance_string = stringify!($( $location )* ).as_bytes(); + let instance_string = stringify!( $( $location )* ).as_bytes(); let (config, whitelist) = $params; let $crate::BenchmarkConfig { pallet, diff --git a/utils/frame/benchmarking-cli/Cargo.toml b/utils/frame/benchmarking-cli/Cargo.toml index 2413d3d18d76c..83f93799691d3 100644 --- a/utils/frame/benchmarking-cli/Cargo.toml +++ b/utils/frame/benchmarking-cli/Cargo.toml @@ -30,7 +30,6 @@ serde = "1.0.116" handlebars = "3.5.0" Inflector = "0.11.4" - [features] default = ["db"] db = ["sc-client-db/with-kvdb-rocksdb", "sc-client-db/with-parity-db"] diff --git a/utils/frame/benchmarking-cli/src/writer.rs b/utils/frame/benchmarking-cli/src/writer.rs index b7c44ae1fb652..234c41ef6b0c7 100644 --- a/utils/frame/benchmarking-cli/src/writer.rs +++ b/utils/frame/benchmarking-cli/src/writer.rs @@ -250,7 +250,7 @@ pub fn write_results( // If a user only specified a directory... if file_path.is_dir() { // Check if there might be multiple instances benchmarked. - if all_results.keys().find(|(p, i)| p == pallet && i != instance).is_some() { + if all_results.keys().any(|(p, i)| p == pallet && i != instance) { // Create new file: "path/to/pallet_name_instance_name.rs". file_path.push(pallet.clone() + "_" + &instance.to_snake_case()); } else { From 0b5ffefe712231f3ee5ab16de6cd4188ef07dee2 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 4 Dec 2020 14:07:03 -0800 Subject: [PATCH 4/6] docs --- frame/benchmarking/src/lib.rs | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index f0e665bffaeaf..3b71711af292f 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1052,6 +1052,24 @@ macro_rules! impl_benchmark_test { /// ``` /// /// At the end of `dispatch_benchmark`, you should return this batches object. +/// +/// In the case where you have multiple instances of a pallet that you need to separately benchmark, +/// the name of your module struct will be used as a suffix to your outputted weight file. For +/// example: +/// +/// ``` +/// add_benchmark!(params, batches, pallet_balances, Balances); // pallet_balances.rs +/// add_benchmark!(params, batches, pallet_collective, Council); // pallet_collective_council.rs +/// add_benchmark!(params, batches, pallet_collective, TechnicalCommittee); // pallet_collective_technical_committee.rs +/// ``` +/// +/// You can manipulate this suffixed string by using a type alias if needed. For example: +/// +/// ``` +/// type Council2 = TechnicalCommittee; +/// add_benchmark!(params, batches, pallet_collective, Council2); // pallet_collective_council2.rs +/// ``` + #[macro_export] macro_rules! add_benchmark { ( $params:ident, $batches:ident, $name:ident, $( $location:tt )* ) => ( From 1d283804df5842133001b09f169f9ff788ca0a95 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 4 Dec 2020 14:15:48 -0800 Subject: [PATCH 5/6] fix output --- frame/benchmarking/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 3b71711af292f..2d0aae6c79884 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1067,7 +1067,7 @@ macro_rules! impl_benchmark_test { /// /// ``` /// type Council2 = TechnicalCommittee; -/// add_benchmark!(params, batches, pallet_collective, Council2); // pallet_collective_council2.rs +/// add_benchmark!(params, batches, pallet_collective, Council2); // pallet_collective_council_2.rs /// ``` #[macro_export] From e12594e83c24c2eecc2e38dd846d6d0209e4ad8b Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 4 Dec 2020 14:46:18 -0800 Subject: [PATCH 6/6] Update lib.rs --- frame/benchmarking/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 2d0aae6c79884..27994eeca434f 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -1057,7 +1057,7 @@ macro_rules! impl_benchmark_test { /// the name of your module struct will be used as a suffix to your outputted weight file. For /// example: /// -/// ``` +/// ```ignore /// add_benchmark!(params, batches, pallet_balances, Balances); // pallet_balances.rs /// add_benchmark!(params, batches, pallet_collective, Council); // pallet_collective_council.rs /// add_benchmark!(params, batches, pallet_collective, TechnicalCommittee); // pallet_collective_technical_committee.rs @@ -1065,7 +1065,7 @@ macro_rules! impl_benchmark_test { /// /// You can manipulate this suffixed string by using a type alias if needed. For example: /// -/// ``` +/// ```ignore /// type Council2 = TechnicalCommittee; /// add_benchmark!(params, batches, pallet_collective, Council2); // pallet_collective_council_2.rs /// ```