Improve Benchmark Writer: Remove Unused Components, Remove Multiply b…

…y Zero, Files Split by Pallet (paritytech#6785)

* initial improvements

* better file management, ignore unused components

* Output warning when components unused

* update comment

* Write even when base weight is zero

* remove unwrap where possible

* Dont sort components to dedup

* undo delete

* improve clarity of unused components

* remove unused dep

* Update Process.json

Process.json

@@ -21,4 +21,9 @@
 	"project_name": "Smart Contracts",
 	"owner": "pepyakin",
 	"matrix_room_id": "!yBKstWVBkwzUkPslsp:matrix.parity.io"
+},
+{
+	"project_name": "Benchmarking and Weights",
+	"owner": "shawntabrizi",
+	"matrix_room_id": "!pZPWqCRLVtORZTEsEf:matrix.parity.io"
 }]

frame/benchmarking/src/utils.rs

@@ -30,6 +30,13 @@ pub enum BenchmarkParameter {
 	a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, q, r, s, t, u, v, w, x, y, z,
 }
 
+#[cfg(feature = "std")]
+impl std::fmt::Display for BenchmarkParameter {
+	fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+		write!(f, "{:?}", self)
+	}
+}
+
 /// The results of a single of benchmark.
 #[derive(Encode, Decode, Clone, PartialEq, Debug)]
 pub struct BenchmarkBatch {

utils/frame/benchmarking-cli/Cargo.toml

@@ -12,7 +12,6 @@ description = "CLI for benchmarking FRAME"
 targets = ["x86_64-unknown-linux-gnu"]
 
 [dependencies]
-Inflector = "0.11.4"
 frame-benchmarking = { version = "2.0.0-rc5", path = "../../../frame/benchmarking" }
 sp-core = { version = "2.0.0-rc5", path = "../../../primitives/core" }
 sc-service = { version = "0.8.0-rc5", default-features = false, path = "../../../client/service" }

utils/frame/benchmarking-cli/src/command.rs

@@ -95,8 +95,7 @@ impl BenchmarkCmd {
 						let mut file = crate::writer::open_file("traits.rs")?;
 						crate::writer::write_trait(&mut file, batches.clone())?;
 					} else {
-						let mut file = crate::writer::open_file("benchmarks.rs")?;
-						crate::writer::write_results(&mut file, batches.clone())?;
+						crate::writer::write_results(&batches)?;
 					}
 				}
 

utils/frame/benchmarking-cli/src/writer.rs

@@ -20,13 +20,15 @@
 use std::fs::{File, OpenOptions};
 use std::io::prelude::*;
 use frame_benchmarking::{BenchmarkBatch, BenchmarkSelector, Analysis};
-use inflector::Inflector;
+use sp_runtime::traits::Zero;
+
+const VERSION: &'static str = env!("CARGO_PKG_VERSION");
 
 pub fn open_file(path: &str) -> Result<File, std::io::Error> {
 	OpenOptions::new()
 		.create(true)
 		.write(true)
-		.append(true)
+		.truncate(true)
 		.open(path)
 }
 
@@ -47,81 +49,49 @@ pub fn write_trait(file: &mut File, batches: Vec<BenchmarkBatch>) -> Result<(),
 		if batch.pallet != current_pallet {
 			if !current_pallet.is_empty() {
 				// close trait
-				write!(file, "}}\n").unwrap();
+				write!(file, "}}\n")?;
 			}
 
 			// trait wrapper
-			write!(file, "// {}\n", pallet_string).unwrap();
-			write!(file, "pub trait WeightInfo {{\n").unwrap();
-
-			current_pallet = batch.pallet.clone()
-		}
-
-		// function name
-		write!(file, "\tfn {}(", benchmark_string).unwrap();
-
-		// params
-		let components = &batch.results[0].components;
-		for component in components {
-			write!(file, "{:?}: u32, ", component.0).unwrap();
-		}
-		// return value
-		write!(file, ") -> Weight;\n").unwrap();
-	}
-
-	// final close trait
-	write!(file, "}}\n").unwrap();
-
-	// Reset
-	current_pallet = Vec::<u8>::new();
-
-	for batch in &batches {
-		if batch.results.is_empty() { continue }
-
-		let benchmark_string = String::from_utf8(batch.benchmark.clone()).unwrap();
-
-		// only create new trait definitions when we go to a new pallet
-		if batch.pallet != current_pallet {
-			if !current_pallet.is_empty() {
-				// close trait
-				write!(file, "}}\n").unwrap();
-			}
-
-			// impl trait
-			write!(file, "\n").unwrap();
-			write!(file, "impl WeightInfo for () {{\n").unwrap();
+			write!(file, "// {}\n", pallet_string)?;
+			write!(file, "pub trait WeightInfo {{\n")?;
 
 			current_pallet = batch.pallet.clone()
 		}
 
 		// function name
-		write!(file, "\tfn {}(", benchmark_string).unwrap();
+		write!(file, "\tfn {}(", benchmark_string)?;
 
 		// params
 		let components = &batch.results[0].components;
 		for component in components {
-			write!(file, "_{:?}: u32, ", component.0).unwrap();
+			write!(file, "{:?}: u32, ", component.0)?;
 		}
 		// return value
-		write!(file, ") -> Weight {{ 1_000_000_000 }}\n").unwrap();
+		write!(file, ") -> Weight;\n")?;
 	}
 
 	// final close trait
-	write!(file, "}}\n").unwrap();
+	write!(file, "}}\n")?;
 
 	Ok(())
 }
 
-pub fn write_results(file: &mut File, batches: Vec<BenchmarkBatch>) -> Result<(), std::io::Error> {
+pub fn write_results(batches: &[BenchmarkBatch]) -> Result<(), std::io::Error> {
 	let mut current_pallet = Vec::<u8>::new();
 
 	// Skip writing if there are no batches
 	if batches.is_empty() { return Ok(()) }
 
-	// general imports
-	write!(file, "use frame_support::weights::{{Weight, constants::RocksDbWeight as DbWeight}};\n").unwrap();
+	let mut batches_iter = batches.iter().peekable();
 
-	for batch in &batches {
+	let first_pallet = String::from_utf8(
+		batches_iter.peek().expect("we checked that batches is not empty").pallet.clone()
+	).unwrap();
+	let mut file = open_file(&(first_pallet + ".rs"))?;
+
+
+	while let Some(batch) = batches_iter.next() {
 		// Skip writing if there are no results
 		if batch.results.is_empty() { continue }
 
@@ -130,69 +100,120 @@ pub fn write_results(file: &mut File, batches: Vec<BenchmarkBatch>) -> Result<()
 
 		// only create new trait definitions when we go to a new pallet
 		if batch.pallet != current_pallet {
-			if !current_pallet.is_empty() {
-				// close trait
-				write!(file, "}}\n").unwrap();
-			}
+			// auto-generation note
+			write!(
+				file,
+				"//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION {}\n\n",
+				 VERSION,
+			)?;
+
+			// general imports
+			write!(
+				file,
+				"use frame_support::weights::{{Weight, constants::RocksDbWeight as DbWeight}};\n\n"
+			)?;
 
 			// struct for weights
-			write!(file, "pub struct WeightFor{};\n",
-				pallet_string.to_pascal_case(),
-			).unwrap();
+			write!(file, "pub struct WeightInfo;\n")?;
 
 			// trait wrapper
-			write!(file, "impl {}::WeightInfo for WeightFor{} {{\n",
-				pallet_string,
-				pallet_string.to_pascal_case(),
-			).unwrap();
+			write!(file, "impl {}::WeightInfo for WeightInfo {{\n", pallet_string)?;
 
 			current_pallet = batch.pallet.clone()
 		}
 
-		// function name
-		write!(file, "\tfn {}(", benchmark_string).unwrap();
+		// Analysis results
+		let extrinsic_time = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::ExtrinsicTime).unwrap();
+		let reads = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::Reads).unwrap();
+		let writes = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::Writes).unwrap();
 
+		// Analysis data may include components that are not used, this filters out anything whose value is zero.
+		let mut used_components = Vec::new();
+		let mut used_extrinsic_time = Vec::new();
+		let mut used_reads = Vec::new();
+		let mut used_writes = Vec::new();
+		extrinsic_time.slopes.iter().zip(extrinsic_time.names.iter()).for_each(|(slope, name)| {
+			if !slope.is_zero() {
+				if !used_components.contains(&name) { used_components.push(name); }
+				used_extrinsic_time.push((slope, name));
+			}
+		});
+		reads.slopes.iter().zip(reads.names.iter()).for_each(|(slope, name)| {
+			if !slope.is_zero() {
+				if !used_components.contains(&name) { used_components.push(name); }
+				used_reads.push((slope, name));
+			}
+		});
+		writes.slopes.iter().zip(writes.names.iter()).for_each(|(slope, name)| {
+			if !slope.is_zero() {
+				if !used_components.contains(&name) { used_components.push(name); }
+				used_writes.push((slope, name));
+			}
+		});
+
+		let all_components = batch.results[0].components
+			.iter()
+			.map(|(name, _)| -> String { return name.to_string() })
+			.collect::<Vec<String>>();
+		if all_components.len() != used_components.len() {
+			let mut unused_components = all_components;
+			unused_components.retain(|x| !used_components.contains(&x));
+			write!(file, "\t// WARNING! Some components were not used: {:?}\n", unused_components)?;
+		}
+
+		// function name
+		write!(file, "\tfn {}(", benchmark_string)?;
 		// params
-		let components = &batch.results[0].components;
-		for component in components {
-			write!(file, "{:?}: u32, ", component.0).unwrap();
+		for component in used_components {
+			write!(file, "{}: u32, ", component)?;
 		}
 		// return value
-		write!(file, ") -> Weight {{\n").unwrap();
+		write!(file, ") -> Weight {{\n")?;
 
-		let extrinsic_time = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::ExtrinsicTime).unwrap();
-		write!(file, "\t\t({} as Weight)\n", extrinsic_time.base.saturating_mul(1000)).unwrap();
-		extrinsic_time.slopes.iter().zip(extrinsic_time.names.iter()).for_each(|(slope, name)| {
+		write!(file, "\t\t({} as Weight)\n", extrinsic_time.base.saturating_mul(1000))?;
+		used_extrinsic_time.iter().try_for_each(|(slope, name)| -> Result<(), std::io::Error> {
 			write!(file, "\t\t\t.saturating_add(({} as Weight).saturating_mul({} as Weight))\n",
 				slope.saturating_mul(1000),
 				name,
-			).unwrap();
-		});
+			)
+		})?;
 
-		let reads = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::Reads).unwrap();
-		write!(file, "\t\t\t.saturating_add(DbWeight::get().reads({} as Weight))\n", reads.base).unwrap();
-		reads.slopes.iter().zip(reads.names.iter()).for_each(|(slope, name)| {
+		if !reads.base.is_zero() {
+			write!(file, "\t\t\t.saturating_add(DbWeight::get().reads({} as Weight))\n", reads.base)?;
+		}
+		used_reads.iter().try_for_each(|(slope, name)| -> Result<(), std::io::Error> {
 			write!(file, "\t\t\t.saturating_add(DbWeight::get().reads(({} as Weight).saturating_mul({} as Weight)))\n",
 				slope,
 				name,
-			).unwrap();
-		});
+			)
+		})?;
 
-		let writes = Analysis::min_squares_iqr(&batch.results, BenchmarkSelector::Writes).unwrap();
-		write!(file, "\t\t\t.saturating_add(DbWeight::get().writes({} as Weight))\n", writes.base).unwrap();
-		writes.slopes.iter().zip(writes.names.iter()).for_each(|(slope, name)| {
+		if !writes.base.is_zero() {
+			write!(file, "\t\t\t.saturating_add(DbWeight::get().writes({} as Weight))\n", writes.base)?;
+		}
+		used_writes.iter().try_for_each(|(slope, name)| -> Result<(), std::io::Error> {
 			write!(file, "\t\t\t.saturating_add(DbWeight::get().writes(({} as Weight).saturating_mul({} as Weight)))\n",
 				slope,
 				name,
-			).unwrap();
-		});
+			)
+		})?;
 
 		// close function
-		write!(file, "\t}}\n").unwrap();
+		write!(file, "\t}}\n")?;
+
+		// Check if this is the end of the iterator
+		if let Some(next) = batches_iter.peek() {
+			// Next pallet is different than current pallet, so we close up the file and open a new one.
+			if next.pallet != current_pallet {
+				write!(file, "}}\n")?;
+				let next_pallet = String::from_utf8(next.pallet.clone()).unwrap();
+				file = open_file(&(next_pallet + ".rs"))?;
+			}
+		} else {
+			// This is the end of the iterator, so we close up the final file.
+			write!(file, "}}\n")?;
+		}
 	}
 
-	// final close trait
-	write!(file, "}}\n").unwrap();
-
 	Ok(())
 }