Skip to content

Commit 4fba586

Browse files
gui1117github-actions[bot]shawntabrizibkchr
authored
frame-omni-bencher: better diagnostic on insufficient data points (#11510)
When a benchmark is run with not enough steps and too many points are skipped then it can make the analysis panic. This PR improves the panic message and gives precise information about which benchmark is at fault. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com> Co-authored-by: Bastian Köcher <git@kchr.de>
1 parent cd3eb6d commit 4fba586

File tree

7 files changed

+112
-70
lines changed

7 files changed

+112
-70
lines changed

‎Cargo.lock‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎prdoc/pr_11510.prdoc‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
title: 'frame-omni-bencher: better diagnostic on insufficient data points'
2+
doc:
3+
- audience: Runtime Dev
4+
description: When a benchmark is run with not enough steps and too many points are
5+
skipped then it can make the analysis panic. This PR improves the panic message
6+
and gives precise information about which benchmark is at fault.
7+
crates:
8+
- name: frame-benchmarking
9+
bump: major
10+
- name: frame-benchmarking-cli
11+
bump: major

‎substrate/frame/benchmarking/Cargo.toml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ workspace = true
1616
targets = ["x86_64-unknown-linux-gnu"]
1717

1818
[dependencies]
19+
anyhow = { optional = true, workspace = true }
1920
codec = { workspace = true }
2021
frame-support = { workspace = true }
2122
frame-support-procedural = { workspace = true }
@@ -45,6 +46,7 @@ sp-state-machine = { workspace = true }
4546
[features]
4647
default = ["std"]
4748
std = [
49+
"anyhow/std",
4850
"codec/std",
4951
"frame-support-procedural/std",
5052
"frame-support/std",

‎substrate/frame/benchmarking/src/analysis.rs‎

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -197,10 +197,11 @@ fn linear_regression(
197197
impl Analysis {
198198
// Useful for when there are no components, and we just need an median value of the benchmark
199199
// results. Note: We choose the median value because it is more robust to outliers.
200-
fn median_value(r: &Vec<BenchmarkResult>, selector: BenchmarkSelector) -> Option<Self> {
201-
if r.is_empty() {
202-
return None;
203-
}
200+
fn median_value(
201+
r: &Vec<BenchmarkResult>,
202+
selector: BenchmarkSelector,
203+
) -> Result<Self, anyhow::Error> {
204+
anyhow::ensure!(!r.is_empty(), "benchmark results cannot be empty");
204205

205206
let mut values: Vec<u128> = r
206207
.iter()
@@ -216,7 +217,7 @@ impl Analysis {
216217
values.sort();
217218
let mid = values.len() / 2;
218219

219-
Some(Self {
220+
Ok(Self {
220221
base: selector.scale_weight(values[mid]),
221222
slopes: Vec::new(),
222223
names: Vec::new(),
@@ -227,7 +228,12 @@ impl Analysis {
227228
})
228229
}
229230

230-
pub fn median_slopes(r: &Vec<BenchmarkResult>, selector: BenchmarkSelector) -> Option<Self> {
231+
pub fn median_slopes(
232+
r: &Vec<BenchmarkResult>,
233+
selector: BenchmarkSelector,
234+
) -> Result<Self, anyhow::Error> {
235+
anyhow::ensure!(!r.is_empty(), "benchmark results cannot be empty");
236+
231237
if r[0].components.is_empty() {
232238
return Self::median_value(r, selector);
233239
}
@@ -273,7 +279,7 @@ impl Analysis {
273279

274280
let models = results
275281
.iter()
276-
.map(|(_, _, _, ref values)| {
282+
.map(|(param_name, _, _, ref values)| {
277283
let mut slopes = vec![];
278284
for (i, &(x1, y1)) in values.iter().enumerate() {
279285
for &(x2, y2) in values.iter().skip(i + 1) {
@@ -282,6 +288,19 @@ impl Analysis {
282288
}
283289
}
284290
}
291+
if slopes.is_empty() {
292+
let unique_values = values
293+
.iter()
294+
.map(|(x, _)| x)
295+
.collect::<std::collections::BTreeSet<_>>()
296+
.len();
297+
return Err(anyhow::anyhow!(
298+
"Parameter `{param_name}` only has \
299+
{unique_values} unique value(s) but needs at least 2 to compute a slope. \
300+
This can happen when too many benchmark samples are skipped. \
301+
Try increasing the number of steps for this parameter or fix the benchmark.",
302+
));
303+
}
285304
slopes.sort_by(|a, b| a.partial_cmp(b).expect("values well defined; qed"));
286305
let slope = slopes[slopes.len() / 2];
287306

@@ -292,9 +311,9 @@ impl Analysis {
292311
offsets.sort_by(|a, b| a.partial_cmp(b).expect("values well defined; qed"));
293312
let offset = offsets[offsets.len() / 2];
294313

295-
(offset, slope)
314+
Ok((offset, slope))
296315
})
297-
.collect::<Vec<_>>();
316+
.collect::<Result<Vec<_>, anyhow::Error>>()?;
298317

299318
let models = models
300319
.iter()
@@ -316,7 +335,7 @@ impl Analysis {
316335
.map(|x| selector.scale_and_cast_weight(x.1.max(0f64), false))
317336
.collect::<Vec<_>>();
318337

319-
Some(Self {
338+
Ok(Self {
320339
base,
321340
slopes,
322341
names: results.into_iter().map(|x| x.0).collect::<Vec<_>>(),
@@ -327,7 +346,12 @@ impl Analysis {
327346
})
328347
}
329348

330-
pub fn min_squares_iqr(r: &Vec<BenchmarkResult>, selector: BenchmarkSelector) -> Option<Self> {
349+
pub fn min_squares_iqr(
350+
r: &Vec<BenchmarkResult>,
351+
selector: BenchmarkSelector,
352+
) -> Result<Self, anyhow::Error> {
353+
anyhow::ensure!(!r.is_empty(), "benchmark results cannot be empty");
354+
331355
if r[0].components.is_empty() || r.len() <= 2 {
332356
return Self::median_value(r, selector);
333357
}
@@ -379,9 +403,12 @@ impl Analysis {
379403
}
380404
}
381405

382-
let (intercept, slopes, errors) = linear_regression(xs, ys, r[0].components.len())?;
406+
let (intercept, slopes, errors) = linear_regression(xs, ys, r[0].components.len())
407+
.ok_or_else(|| {
408+
anyhow::anyhow!("linear regression failed for min_squares_iqr analysis")
409+
})?;
383410

384-
Some(Self {
411+
Ok(Self {
385412
base: selector.scale_and_cast_weight(intercept, true),
386413
slopes: slopes
387414
.into_iter()
@@ -400,16 +427,12 @@ impl Analysis {
400427
})
401428
}
402429

403-
pub fn max(r: &Vec<BenchmarkResult>, selector: BenchmarkSelector) -> Option<Self> {
404-
let median_slopes = Self::median_slopes(r, selector);
405-
let min_squares = Self::min_squares_iqr(r, selector);
406-
407-
if median_slopes.is_none() || min_squares.is_none() {
408-
return None;
409-
}
410-
411-
let median_slopes = median_slopes.unwrap();
412-
let min_squares = min_squares.unwrap();
430+
pub fn max(
431+
r: &Vec<BenchmarkResult>,
432+
selector: BenchmarkSelector,
433+
) -> Result<Self, anyhow::Error> {
434+
let median_slopes = Self::median_slopes(r, selector)?;
435+
let min_squares = Self::min_squares_iqr(r, selector)?;
413436

414437
let base = median_slopes.base.max(min_squares.base);
415438
let slopes = median_slopes
@@ -429,7 +452,7 @@ impl Analysis {
429452
let errors = min_squares.errors;
430453
let minimum = selector.get_minimum(&r);
431454

432-
Some(Self { base, slopes, names, value_dists, errors, selector, minimum })
455+
Ok(Self { base, slopes, names, value_dists, errors, selector, minimum })
433456
}
434457
}
435458

‎substrate/utils/frame/benchmarking-cli/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ targets = ["x86_64-unknown-linux-gnu"]
1717

1818
[dependencies]
1919
Inflector = { workspace = true }
20+
anyhow = { workspace = true, default-features = true }
2021
array-bytes = { workspace = true, default-features = true }
2122
chrono = { workspace = true }
2223
clap = { features = ["derive"], workspace = true }

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

Lines changed: 29 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -893,7 +893,8 @@ impl PalletCmd {
893893

894894
if !self.no_storage_info {
895895
let mut storage_per_prefix = HashMap::<Vec<u8>, Vec<BenchmarkResult>>::new();
896-
let pov_mode = pov_modes.get(&(pallet, benchmark)).cloned().unwrap_or_default();
896+
let pov_mode =
897+
pov_modes.get(&(pallet, benchmark.clone())).cloned().unwrap_or_default();
897898

898899
let comments = writer::process_storage_results(
899900
&mut storage_per_prefix,
@@ -914,49 +915,45 @@ impl PalletCmd {
914915
// Conduct analysis.
915916
if !self.no_median_slopes {
916917
println!("Median Slopes Analysis\n========");
917-
if let Some(analysis) =
918-
Analysis::median_slopes(&batch.time_results, BenchmarkSelector::ExtrinsicTime)
918+
match Analysis::median_slopes(&batch.time_results, BenchmarkSelector::ExtrinsicTime)
919919
{
920-
println!("-- Extrinsic Time --\n{}", analysis);
920+
Ok(analysis) => println!("-- Extrinsic Time --\n{}", analysis),
921+
Err(err) => println!("-- Extrinsic Time --\nError: {:?}", err),
921922
}
922-
if let Some(analysis) =
923-
Analysis::median_slopes(&batch.db_results, BenchmarkSelector::Reads)
924-
{
925-
println!("Reads = {:?}", analysis);
923+
match Analysis::median_slopes(&batch.db_results, BenchmarkSelector::Reads) {
924+
Ok(analysis) => println!("Reads = {:?}", analysis),
925+
Err(err) => println!("Reads: Error: {:?}", err),
926926
}
927-
if let Some(analysis) =
928-
Analysis::median_slopes(&batch.db_results, BenchmarkSelector::Writes)
929-
{
930-
println!("Writes = {:?}", analysis);
927+
match Analysis::median_slopes(&batch.db_results, BenchmarkSelector::Writes) {
928+
Ok(analysis) => println!("Writes = {:?}", analysis),
929+
Err(err) => println!("Writes: Error: {:?}", err),
931930
}
932-
if let Some(analysis) =
933-
Analysis::median_slopes(&batch.db_results, BenchmarkSelector::ProofSize)
934-
{
935-
println!("Recorded proof Size = {:?}", analysis);
931+
match Analysis::median_slopes(&batch.db_results, BenchmarkSelector::ProofSize) {
932+
Ok(analysis) => println!("Recorded proof Size = {:?}", analysis),
933+
Err(err) => println!("Recorded proof Size: Error: {:?}", err),
936934
}
937935
println!();
938936
}
939937
if !self.no_min_squares {
940938
println!("Min Squares Analysis\n========");
941-
if let Some(analysis) =
942-
Analysis::min_squares_iqr(&batch.time_results, BenchmarkSelector::ExtrinsicTime)
943-
{
944-
println!("-- Extrinsic Time --\n{}", analysis);
939+
match Analysis::min_squares_iqr(
940+
&batch.time_results,
941+
BenchmarkSelector::ExtrinsicTime,
942+
) {
943+
Ok(analysis) => println!("-- Extrinsic Time --\n{}", analysis),
944+
Err(err) => println!("-- Extrinsic Time --\nError: {:?}", err),
945945
}
946-
if let Some(analysis) =
947-
Analysis::min_squares_iqr(&batch.db_results, BenchmarkSelector::Reads)
948-
{
949-
println!("Reads = {:?}", analysis);
946+
match Analysis::min_squares_iqr(&batch.db_results, BenchmarkSelector::Reads) {
947+
Ok(analysis) => println!("Reads = {:?}", analysis),
948+
Err(err) => println!("Reads: Error: {:?}", err),
950949
}
951-
if let Some(analysis) =
952-
Analysis::min_squares_iqr(&batch.db_results, BenchmarkSelector::Writes)
953-
{
954-
println!("Writes = {:?}", analysis);
950+
match Analysis::min_squares_iqr(&batch.db_results, BenchmarkSelector::Writes) {
951+
Ok(analysis) => println!("Writes = {:?}", analysis),
952+
Err(err) => println!("Writes: Error: {:?}", err),
955953
}
956-
if let Some(analysis) =
957-
Analysis::min_squares_iqr(&batch.db_results, BenchmarkSelector::ProofSize)
958-
{
959-
println!("Recorded proof Size = {:?}", analysis);
954+
match Analysis::min_squares_iqr(&batch.db_results, BenchmarkSelector::ProofSize) {
955+
Ok(analysis) => println!("Recorded proof Size = {:?}", analysis),
956+
Err(err) => println!("Recorded proof Size: Error: {:?}", err),
960957
}
961958
println!();
962959
}

‎substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs‎

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ use std::{
2323
path::PathBuf,
2424
};
2525

26+
use anyhow::Context;
2627
use inflector::Inflector;
2728
use itertools::Itertools;
2829
use serde::Serialize;
@@ -142,10 +143,10 @@ fn map_results(
142143
pov_analysis_choice: &AnalysisChoice,
143144
worst_case_map_values: u32,
144145
additional_trie_layers: u8,
145-
) -> Result<HashMap<(String, String), Vec<BenchmarkData>>, std::io::Error> {
146+
) -> Result<HashMap<(String, String), Vec<BenchmarkData>>, sc_cli::Error> {
146147
// Skip if batches is empty.
147148
if batches.is_empty() {
148-
return Err(io_error("empty batches"));
149+
return Err(io_error("empty batches").into());
149150
}
150151

151152
let mut all_benchmarks = HashMap::<_, Vec<BenchmarkData>>::new();
@@ -168,7 +169,7 @@ fn map_results(
168169
pov_analysis_choice,
169170
worst_case_map_values,
170171
additional_trie_layers,
171-
);
172+
)?;
172173
let pallet_benchmarks = all_benchmarks.entry((pallet_name, instance_name)).or_default();
173174
pallet_benchmarks.push(benchmark_data);
174175
}
@@ -198,7 +199,7 @@ fn get_benchmark_data(
198199
pov_analysis_choice: &AnalysisChoice,
199200
worst_case_map_values: u32,
200201
additional_trie_layers: u8,
201-
) -> BenchmarkData {
202+
) -> Result<BenchmarkData, sc_cli::Error> {
202203
// Analyze benchmarks to get the linear regression.
203204
let analysis_function = match analysis_choice {
204205
AnalysisChoice::MinSquares => Analysis::min_squares_iqr,
@@ -214,14 +215,18 @@ fn get_benchmark_data(
214215
let benchmark = String::from_utf8(batch.benchmark.clone()).unwrap();
215216

216217
let extrinsic_time = analysis_function(&batch.time_results, BenchmarkSelector::ExtrinsicTime)
217-
.expect("analysis function should return an extrinsic time for valid inputs");
218+
.context(format!("benchmark '{pallet}::{benchmark}'"))
219+
.map_err(|e| sc_cli::Error::Application(e.into()))?;
218220
let reads = analysis_function(&batch.db_results, BenchmarkSelector::Reads)
219-
.expect("analysis function should return the number of reads for valid inputs");
221+
.context(format!("benchmark '{pallet}::{benchmark}'"))
222+
.map_err(|e| sc_cli::Error::Application(e.into()))?;
220223
let writes = analysis_function(&batch.db_results, BenchmarkSelector::Writes)
221-
.expect("analysis function should return the number of writes for valid inputs");
224+
.context(format!("benchmark '{pallet}::{benchmark}'"))
225+
.map_err(|e| sc_cli::Error::Application(e.into()))?;
222226
let recorded_proof_size =
223227
pov_analysis_function(&batch.db_results, BenchmarkSelector::ProofSize)
224-
.expect("analysis function should return proof sizes for valid inputs");
228+
.context(format!("benchmark '{pallet}::{benchmark}'"))
229+
.map_err(|e| sc_cli::Error::Application(e.into()))?;
225230

226231
// Analysis data may include components that are not used, this filters out anything whose value
227232
// is zero.
@@ -298,21 +303,22 @@ fn get_benchmark_data(
298303
additional_trie_layers,
299304
);
300305

301-
let proof_size_per_components = storage_per_prefix
306+
let proof_size_per_components: Vec<_> = storage_per_prefix
302307
.iter()
303308
.map(|(prefix, results)| {
304309
let proof_size = analysis_function(results, BenchmarkSelector::ProofSize)
305-
.expect("analysis function should return proof sizes for valid inputs");
310+
.context(format!("benchmark '{pallet}::{benchmark}'"))
311+
.map_err(|e| sc_cli::Error::Application(e.into()))?;
306312
let slope = proof_size
307313
.slopes
308314
.into_iter()
309315
.zip(proof_size.names.iter())
310316
.zip(extract_errors(&proof_size.errors))
311317
.map(|((slope, name), error)| ComponentSlope { name: name.clone(), slope, error })
312318
.collect::<Vec<_>>();
313-
(prefix.clone(), slope, proof_size.base)
319+
Ok((prefix.clone(), slope, proof_size.base))
314320
})
315-
.collect::<Vec<_>>();
321+
.collect::<Result<_, sc_cli::Error>>()?;
316322

317323
let mut base_calculated_proof_size = 0;
318324
// Sum up the proof sizes per component
@@ -357,7 +363,7 @@ fn get_benchmark_data(
357363
.map(|c| c.clone())
358364
.unwrap_or_default();
359365

360-
BenchmarkData {
366+
Ok(BenchmarkData {
361367
name: benchmark,
362368
components,
363369
base_weight: extrinsic_time.base,
@@ -373,7 +379,7 @@ fn get_benchmark_data(
373379
component_ranges,
374380
comments,
375381
min_execution_time: extrinsic_time.minimum,
376-
}
382+
})
377383
}
378384

379385
/// Create weight file from benchmark data and Handlebars template.

0 commit comments

Comments
 (0)