Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Force base weights to be the minimum only when the intercept is negative #12482

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions .maintain/frame-weight-template.hbs
Expand Up @@ -64,6 +64,7 @@ impl<T: frame_system::Config> WeightInfo for SubstrateWeight<T> {
{{~#each benchmark.components as |c| ~}}
{{~#if (not c.is_used)}}_{{/if}}{{c.name}}: u32, {{/each~}}
) -> Weight {
// Minimum execution time: {{underscore cw.min_execution_time}}
shawntabrizi marked this conversation as resolved.
Show resolved Hide resolved
Weight::from_ref_time({{underscore benchmark.base_weight}} as u64)
{{#each benchmark.component_weight as |cw|}}
// Standard Error: {{underscore cw.error}}
Expand Down Expand Up @@ -99,6 +100,7 @@ impl WeightInfo for () {
{{~#each benchmark.components as |c| ~}}
{{~#if (not c.is_used)}}_{{/if}}{{c.name}}: u32, {{/each~}}
) -> Weight {
// Minimum execution time: {{underscore cw.min_execution_time}}
Weight::from_ref_time({{underscore benchmark.base_weight}} as u64)
{{#each benchmark.component_weight as |cw|}}
// Standard Error: {{underscore cw.error}}
Expand Down
59 changes: 46 additions & 13 deletions frame/benchmarking/src/analysis.rs
Expand Up @@ -26,6 +26,7 @@ pub struct Analysis {
pub names: Vec<String>,
pub value_dists: Option<Vec<(Vec<u32>, u128, u128)>>,
pub errors: Option<Vec<u128>>,
pub minimum: u128,
selector: BenchmarkSelector,
}

Expand All @@ -49,7 +50,10 @@ fn mul_1000_into_u128(value: f64) -> u128 {
impl BenchmarkSelector {
fn scale_and_cast_weight(self, value: f64, round_up: bool) -> u128 {
if let BenchmarkSelector::ExtrinsicTime = self {
mul_1000_into_u128(value)
// We add a very slight bias here to counteract the numerical imprecision of the linear
// regression where due to rounding issues it can emit a number like `2999999.999999998`
// which we most certainly always want to round up instead of truncating.
mul_1000_into_u128(value + 0.000_000_005)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you not use ceil() somewhere here to keep the logic simple?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ceil will round the whole thing, e.g. let's assume you have a value of 10.599999999 - using a ceil would round it to 11, while with this it will round it to 10.6, so the behavior would be quite different

} else {
if round_up {
(value + 0.5) as u128
Expand All @@ -74,6 +78,24 @@ impl BenchmarkSelector {
value
}
}

fn get_value(self, result: &BenchmarkResult) -> u128 {
match self {
BenchmarkSelector::ExtrinsicTime => result.extrinsic_time,
BenchmarkSelector::StorageRootTime => result.storage_root_time,
BenchmarkSelector::Reads => result.reads.into(),
BenchmarkSelector::Writes => result.writes.into(),
BenchmarkSelector::ProofSize => result.proof_size.into(),
}
}

fn get_minimum(self, results: &[BenchmarkResult]) -> u128 {
results
.iter()
.map(|result| self.get_value(result))
.min()
.expect("results cannot be empty")
}
}

#[derive(Debug)]
Expand Down Expand Up @@ -109,8 +131,8 @@ impl TryFrom<Option<String>> for AnalysisChoice {
}

fn raw_linear_regression(
xs: Vec<f64>,
ys: Vec<f64>,
xs: &[f64],
ys: &[f64],
x_vars: usize,
with_intercept: bool,
) -> Option<(f64, Vec<f64>, Vec<f64>)> {
Expand Down Expand Up @@ -147,6 +169,14 @@ fn linear_regression(
mut ys: Vec<f64>,
x_vars: usize,
) -> Option<(f64, Vec<f64>, Vec<f64>)> {
let (intercept, params, errors) = raw_linear_regression(&xs, &ys, x_vars, true)?;
if intercept > 0.0 {
return Some((intercept, params, errors[1..].to_vec()))
}
Comment on lines +173 to +175
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should actually be >= right?

cc @koute @ggwpez

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.... well, I guess technically yes, you're right, although in practice I don't think linear interpolation's going to ever emit an intercept that's actually zero since the numbers would have to perfectly line up for that to happen? At least for the execution times. For other metrics I guess it could maybe happen? I'm not sure.

I'll put up a PR and change it to >=.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably does not matter much for floating point number 🤷‍♂️ but if you want to fix it, okay


// The intercept is negative.
// The weights must be always positive, so we can't have that.

let mut min = ys[0];
for &value in &ys {
if value < min {
Expand All @@ -158,7 +188,7 @@ fn linear_regression(
*value -= min;
}

let (intercept, params, errors) = raw_linear_regression(xs, ys, x_vars, false)?;
let (intercept, params, errors) = raw_linear_regression(&xs, &ys, x_vars, false)?;
Some((intercept + min, params, errors[1..].to_vec()))
koute marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down Expand Up @@ -190,6 +220,7 @@ impl Analysis {
names: Vec::new(),
value_dists: None,
errors: None,
minimum: selector.get_minimum(&r),
selector,
})
}
Expand Down Expand Up @@ -289,6 +320,7 @@ impl Analysis {
names: results.into_iter().map(|x| x.0).collect::<Vec<_>>(),
value_dists: None,
errors: None,
minimum: selector.get_minimum(&r),
selector,
})
}
Expand Down Expand Up @@ -361,6 +393,7 @@ impl Analysis {
.map(|value| selector.scale_and_cast_weight(value, false))
.collect(),
),
minimum: selector.get_minimum(&r),
selector,
})
}
Expand Down Expand Up @@ -392,8 +425,9 @@ impl Analysis {
let names = median_slopes.names;
let value_dists = min_squares.value_dists;
let errors = min_squares.errors;
let minimum = selector.get_minimum(&r);

Some(Self { base, slopes, names, value_dists, errors, selector })
Some(Self { base, slopes, names, value_dists, errors, selector, minimum })
}
}

Expand Down Expand Up @@ -533,8 +567,7 @@ mod tests {
16.0, 17.0, 18.0, 19.0, 20.0,
];

let (intercept, params, errors) =
raw_linear_regression(xs.clone(), ys.clone(), 1, true).unwrap();
let (intercept, params, errors) = raw_linear_regression(&xs, &ys, 1, true).unwrap();
assert_eq!(intercept as i64, -2712997);
assert_eq!(params.len(), 1);
assert_eq!(params[0] as i64, 34444926);
Expand Down Expand Up @@ -688,15 +721,15 @@ mod tests {

let extrinsic_time =
Analysis::min_squares_iqr(&data, BenchmarkSelector::ExtrinsicTime).unwrap();
assert_eq!(extrinsic_time.base, 11_500_000_000);
assert_eq!(extrinsic_time.slopes, vec![630635838, 23699421]);
assert_eq!(extrinsic_time.base, 10_000_000_000);
assert_eq!(extrinsic_time.slopes, vec![1000000000, 100000000]);

let reads = Analysis::min_squares_iqr(&data, BenchmarkSelector::Reads).unwrap();
assert_eq!(reads.base, 3);
assert_eq!(reads.base, 2);
assert_eq!(reads.slopes, vec![1, 0]);

let writes = Analysis::min_squares_iqr(&data, BenchmarkSelector::Writes).unwrap();
assert_eq!(writes.base, 2);
assert_eq!(writes.base, 0);
assert_eq!(writes.slopes, vec![0, 2]);
}

Expand All @@ -711,7 +744,7 @@ mod tests {

let extrinsic_time =
Analysis::min_squares_iqr(&data, BenchmarkSelector::ExtrinsicTime).unwrap();
assert_eq!(extrinsic_time.base, 2_000_000_000);
assert_eq!(extrinsic_time.slopes, vec![4_000_000_000]);
assert_eq!(extrinsic_time.base, 3_000_000_000);
assert_eq!(extrinsic_time.slopes, vec![3_000_000_000]);
}
}
1 change: 1 addition & 0 deletions utils/frame/benchmarking-cli/src/pallet/template.hbs
Expand Up @@ -33,6 +33,7 @@ impl<T: frame_system::Config> {{pallet}}::WeightInfo for WeightInfo<T> {
{{~#each benchmark.components as |c| ~}}
{{~#if (not c.is_used)}}_{{/if}}{{c.name}}: u32, {{/each~}}
) -> Weight {
// Minimum execution time: {{underscore cw.min_execution_time}}
Weight::from_ref_time({{underscore benchmark.base_weight}} as u64)
{{#each benchmark.component_weight as |cw|}}
// Standard Error: {{underscore cw.error}}
Expand Down
2 changes: 2 additions & 0 deletions utils/frame/benchmarking-cli/src/pallet/writer.rs
Expand Up @@ -69,6 +69,7 @@ struct BenchmarkData {
component_writes: Vec<ComponentSlope>,
component_ranges: Vec<ComponentRange>,
comments: Vec<String>,
min_execution_time: u128,
}

// This forwards some specific metadata from the `PalletCmd`
Expand Down Expand Up @@ -257,6 +258,7 @@ fn get_benchmark_data(
component_writes: used_writes,
component_ranges,
comments,
min_execution_time: extrinsic_time.minimum,
}
}

Expand Down