benchmarking f32 step calculation (#11890)

* benchmark steps with f32

* divide by self.steps - 1

* avoid steps <= 1

* Fix step size

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Fix 'steps' print and print number of args

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add Test

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* fix comments

* Remove unneeded comment

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: parity-processbot <>

substrate/bin/node/cli/tests/benchmark_pallet_works.rs

+// This file is part of Substrate.
+
+// Copyright (C) 2022 Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0
+
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License
+// along with this program. If not, see <https://www.gnu.org/licenses/>.
+
+use assert_cmd::cargo::cargo_bin;
+use std::process::Command;
+
+pub mod common;
+
+/// `benchmark pallet` works for the different combinations of `steps` and `repeat`.
+#[test]
+fn benchmark_pallet_works() {
+	// Some invalid combinations:
+	benchmark_pallet(0, 10, false);
+	benchmark_pallet(1, 10, false);
+	// ... and some valid:
+	benchmark_pallet(2, 1, true);
+	benchmark_pallet(50, 20, true);
+	benchmark_pallet(20, 50, true);
+}
+
+fn benchmark_pallet(steps: u32, repeat: u32, should_work: bool) {
+	let output = Command::new(cargo_bin("substrate"))
+		.args(["benchmark", "pallet", "--dev"])
+		// Use the `addition` benchmark since is the fastest.
+		.args(["--pallet", "frame-benchmarking", "--extrinsic", "addition"])
+		.args(["--steps", &format!("{}", steps), "--repeat", &format!("{}", repeat)])
+		.output()
+		.unwrap();
+
+	if output.status.success() != should_work {
+		let log = String::from_utf8_lossy(&output.stderr).to_string();
+		panic!("Test failed:\n{}", log);
+	}
+}

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

@@ -236,14 +236,22 @@ impl PalletCmd {
 					let lowest = self.lowest_range_values.get(idx).cloned().unwrap_or(*low);
 					let highest = self.highest_range_values.get(idx).cloned().unwrap_or(*high);
 
-					let diff = highest - lowest;
+					let diff =
+						highest.checked_sub(lowest).ok_or("`low` cannot be higher than `high`")?;
 
-					// Create up to `STEPS` steps for that component between high and low.
-					let step_size = (diff / self.steps).max(1);
-					let num_of_steps = diff / step_size + 1;
-					for s in 0..num_of_steps {
+					// The slope logic needs at least two points
+					// to compute a slope.
+					if self.steps < 2 {
+						return Err("`steps` must be at least 2.".into())
+					}
+
+					let step_size = (diff as f32 / (self.steps - 1) as f32).max(0.0);
+
+					for s in 0..self.steps {
 						// This is the value we will be testing for component `name`
-						let component_value = lowest + step_size * s;
+						let component_value = ((lowest as f32 + step_size * s as f32) as u32)
+							.min(highest)
+							.max(lowest);
 
 						// Select the max value for all the other components.
 						let c: Vec<(BenchmarkParameter, u32)> = components
@@ -364,13 +372,14 @@ impl PalletCmd {
 						if elapsed >= time::Duration::from_secs(5) {
 							timer = time::SystemTime::now();
 							log::info!(
-								"Running Benchmark: {}.{} {}/{} {}/{}",
+								"Running Benchmark: {}.{}({} args) {}/{} {}/{}",
 								String::from_utf8(pallet.clone())
 									.expect("Encoded from String; qed"),
 								String::from_utf8(extrinsic.clone())
 									.expect("Encoded from String; qed"),
-								s + 1, // s starts at 0. todo show step
-								self.steps,
+								components.len(),
+								s + 1, // s starts at 0.
+								all_components.len(),
 								r + 1,
 								self.external_repeat,
 							);