Skip to content
Closed
Prev Previous commit
Next Next commit
fmt
  • Loading branch information
Daanvdplas committed Oct 18, 2023
commit d476d7f0eefa4a7702580b5be1e2a96ead02eaf0
6 changes: 3 additions & 3 deletions polkadot/runtime/rococo/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ use frame_support::{
InstanceFilter, KeyOwnerProofSystem, LinearStoragePrice, PrivilegeCmp, ProcessMessage,
ProcessMessageError, StorageMapShim, WithdrawReasons,
},
weights::{ConstantMultiplier, RuntimeDbWeight, WeightMeter},
weights::{ConstantMultiplier, WeightMeter},
PalletId,
};
use frame_system::EnsureRoot;
Expand Down Expand Up @@ -2046,7 +2046,7 @@ sp_api::impl_runtime_apis! {
Vec<frame_benchmarking::BenchmarkList>,
Vec<frame_support::traits::StorageInfo>,
Weight,
RuntimeDbWeight,
frame_support::weights::RuntimeDbWeight,
) {
use frame_benchmarking::{Benchmarking, BenchmarkList};
use frame_support::traits::StorageInfoTrait;
Expand All @@ -2059,7 +2059,7 @@ sp_api::impl_runtime_apis! {

let storage_info = AllPalletsWithSystem::storage_info();
let max_extrinsic_weight = BlockWeights::get().per_class.get(DispatchClass::Normal).max_extrinsic.unwrap();
let db_weight: RuntimeDbWeight = <Self as frame_system::Config>::DbWeight::get();
let db_weight: frame_support::weights::RuntimeDbWeight = <Self as frame_system::Config>::DbWeight::get();
(list, storage_info, max_extrinsic_weight, db_weight)
Copy link
Member

Choose a reason for hiding this comment

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

I hoped that we could somehow around doing a breaking change for all runtimes, but it seems unavoidable, unless we want to squeeze this into into any of the vectors.
Maybe you can create a single struct that holds the Weight and RuntimeDbWeight. Then we can extend that in the future and avoid more breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Creating a single struct for Weight and RuntimeDbWeight is either way a better solution. Would you prefer to have it included in BenchmarkList / StorageInfo and make it backwards compatible or add an additional parameter which results in a breaking change?

Copy link
Member

@ggwpez ggwpez Feb 5, 2024

Choose a reason for hiding this comment

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

I think it would need one breaking change, yes. But from then on we can avoid further breaking changes by using the following pattern:

struct AllBenchmarkData { // not a good name
  list: Vec<frame_benchmarking::BenchmarkList>,
  info: Vec<frame_support::traits::StorageInfo>,
  max_ext_weight: Option<..>,
  db_weights: Option<..>
}

and having Default on that thing. The runtime devs can use ..Default::default() to stay forward compatible and we can keep adding Options without directly breaking their code.

}

Expand Down
3 changes: 1 addition & 2 deletions substrate/client/cli/src/arg_enums.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ pub enum SanityWeightCheck {
}

/// The default [`SanityWeightCheck`].
pub const DEFAULT_SANITY_WEIGHT_CHECK: SanityWeightCheck =
SanityWeightCheck::Error;
pub const DEFAULT_SANITY_WEIGHT_CHECK: SanityWeightCheck = SanityWeightCheck::Error;

/// Converts the execution method and instantiation strategy command line arguments
/// into an execution method which can be used internally.
Expand Down
6 changes: 3 additions & 3 deletions substrate/client/cli/src/params/shared_params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// 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 crate::{SanityWeightCheck, DEFAULT_SANITY_WEIGHT_CHECK, arg_enums::TracingReceiver};
use crate::{arg_enums::TracingReceiver, SanityWeightCheck, DEFAULT_SANITY_WEIGHT_CHECK};
use clap::Args;
use sc_service::config::BasePath;
use std::path::PathBuf;
Expand Down Expand Up @@ -72,8 +72,8 @@ pub struct SharedParams {
#[arg(long, value_name = "RECEIVER", value_enum, ignore_case = true, default_value_t = TracingReceiver::Log)]
pub tracing_receiver: TracingReceiver,

/// Sanity weight check for benchmarks. Checks whether an extrinsic's maximum weight exceeds the max
/// extrinsic weight.
/// Sanity weight check for benchmarks. Checks whether an extrinsic's maximum weight exceeds
/// the max extrinsic weight.
#[arg(
long,
value_name = "OUTPUT",
Expand Down
7 changes: 6 additions & 1 deletion substrate/utils/frame/benchmarking-cli/src/pallet/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,12 @@ impl PalletCmd {

// Sanity check for benchmarks. Checks whether an extrinsic's maximum weight (based on max
// component) exceeds the max extrinsic weight.
writer::sanity_weight_check(all_results, max_extrinsic_weight, db_weight, self.shared_params.sanity_weight_check)?;
writer::sanity_weight_check(
all_results,
max_extrinsic_weight,
db_weight,
self.shared_params.sanity_weight_check,
)?;

Ok(())
}
Expand Down
51 changes: 32 additions & 19 deletions substrate/utils/frame/benchmarking-cli/src/pallet/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ use std::{

use inflector::Inflector;
use itertools::Itertools;
use serde::Serialize;
use sc_cli::SanityWeightCheck;
use serde::Serialize;

use crate::{
pallet::command::{ComponentRange, PovEstimationMode, PovModesMap},
Expand Down Expand Up @@ -176,8 +176,8 @@ pub(crate) fn map_results(
Ok(all_benchmarks)
}

// Calculates the total maximum weight of an extrinsic (if present, based on the max component) and compares it
// with the max extrinsic weight allowed in a single block.
// Calculates the total maximum weight of an extrinsic (if present, based on the max component) and
// compares it with the max extrinsic weight allowed in a single block.
//
// `max_extrinsic_weight` & `db_weight` are obtained from the runtime configuration.
pub(crate) fn sanity_weight_check(
Copy link
Contributor

Choose a reason for hiding this comment

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

Conceptually, the way to avoid all of these breaking changes is to do this check in the runtime. Is there a reason why it was chosen in the way that it is now?

As in, there is a single new runtime API through which you pass in all the computed weights, and it would do any checking necessary for it.

Ofc this is also a breaking change differently, but possibly an alternative design.

I think the current design is also good, but something to keep in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading the code, I guess the runtime also doesn't know the maximum value for each component..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After looking at it again, the component ranges are coming from the runtime actually: parameter list

I'm investigating another approach

Expand All @@ -187,7 +187,7 @@ pub(crate) fn sanity_weight_check(
sanity_weight_check: SanityWeightCheck,
) -> Result<(), std::io::Error> {
if sanity_weight_check == SanityWeightCheck::Ignore {
return Ok(());
return Ok(())
}
// Helper function to return max. component value (i.e. max. complexity parameter).
fn max_component(parameter: &ComponentSlope, component_ranges: &Vec<ComponentRange>) -> u64 {
Expand All @@ -199,14 +199,15 @@ pub(crate) fn sanity_weight_check(
0
}

println!("\n\x1B[1mSanity Weight Check 🧐:\x1B[0m each extrinsic's weight function is executed \
println!(
"\n\x1B[1mSanity Weight Check 🧐:\x1B[0m each extrinsic's weight function is executed \
in the worst case scenario and compared with the maximum extrinsic weight (the maximum weight \
that can be put in a single block for an extrinsic with `DispatchClass::Normal`). In other words, \
each extrinsic is checked whether it will fit in an empty (meaning; empty of \
`DispatchClass::Normal` extrinsics) block.\n\n\x1B[4mResults:\x1B[0m\n"
);
let mut sanity_weight_check_passed = true;
// Loop through all benchmark results.
// Loop through all benchmark results.
for ((_, _), results) in all_results.iter() {
// Per pallet, because there can be multiple instances of a pallet.
for result in results {
Expand Down Expand Up @@ -234,8 +235,8 @@ pub(crate) fn sanity_weight_check(
);
}
// Constant storage writes.
total_weight =
total_weight.saturating_add(db_weight.writes(result.base_writes.try_into().unwrap()));
total_weight = total_weight
.saturating_add(db_weight.writes(result.base_writes.try_into().unwrap()));
// Storage writes multiplied by complexity parameter.
for component in &result.component_writes {
total_weight = total_weight.saturating_add(
Expand All @@ -251,12 +252,16 @@ pub(crate) fn sanity_weight_check(
.saturating_mul(max_component(&component, &result.component_ranges)),
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

kinda baffled that all of this math to calculate the final weight of a weight-fn, for a given parameter, needs to be re-created here. Is this really the only place where we have this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This logic only exists in the benchmark templates (i.e. here & here)

// Comparing (worst case scenario) the extrinsic weight against the maximum extrinsic weight.
// Comparing (worst case scenario) the extrinsic weight against the maximum extrinsic
// weight.
if total_weight.ref_time() > max_extrinsic_weight.ref_time() ||
total_weight.proof_size() > max_extrinsic_weight.proof_size() {
total_weight.proof_size() > max_extrinsic_weight.proof_size()
{
sanity_weight_check_passed = false;
println!("\x1B[31m\x1B[1mWARNING!!!\x1B[0m",);
println!("\x1B[31mThe following extrinsic exceeds the maximum extrinsic weight:\x1B[0m",);
println!(
"\x1B[31mThe following extrinsic exceeds the maximum extrinsic weight:\x1B[0m",
);
}
println!("- \x1B[1m'{}'\x1B[0m: {:?}\nPercentage of max extrinsic weight: {:.2}% (ref_time), {:.2}% (proof_size)\n",
result.name,
Expand All @@ -268,10 +273,14 @@ pub(crate) fn sanity_weight_check(
}
match sanity_weight_check_passed {
false => {
println!("\x1B[31mYour extrinsics failed the Sanity Weight Check, please review \
the extrinsic's logic and/or the associated benchmark function.\x1B[0m\n",);
println!(
"\x1B[31mYour extrinsics failed the Sanity Weight Check, please review \
the extrinsic's logic and/or the associated benchmark function.\x1B[0m\n",
);
if sanity_weight_check == SanityWeightCheck::Error {
return Err(io_error(&String::from("One or more extrinsics exceed the maximum extrinsic weight")));
return Err(io_error(&String::from(
"One or more extrinsics exceed the maximum extrinsic weight",
)))
}
},
true => {
Expand Down Expand Up @@ -1345,7 +1354,7 @@ mod test {
#[test]
fn sanity_check_works() {
let mapped_results = map_results(
&[test_data(b"first", b"first", BenchmarkParameter::a, 10, 3),],
&[test_data(b"first", b"first", BenchmarkParameter::a, 10, 3)],
&test_storage_info(),
&Default::default(),
Default::default(),
Expand All @@ -1354,28 +1363,32 @@ mod test {
&AnalysisChoice::MedianSlopes,
1_000_000,
0,
).unwrap();
)
.unwrap();

assert!(sanity_weight_check(
mapped_results.clone(),
Weight::from_parts(20_000, 1_000_000),
RuntimeDbWeight { read: 1000, write: 1000 },
SanityWeightCheck::Error,
).is_err());
)
.is_err());

assert!(sanity_weight_check(
mapped_results.clone(),
Weight::from_parts(20_000, 1_000_000),
RuntimeDbWeight { read: 1000, write: 1000 },
SanityWeightCheck::Warning,
).is_ok());
)
.is_ok());

assert!(sanity_weight_check(
mapped_results,
Weight::from_parts(20_000, 1_000_000),
RuntimeDbWeight { read: 1000, write: 1000 },
SanityWeightCheck::Ignore,
).is_ok());
)
.is_ok());
}

#[test]
Expand Down