Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix HeadersToKeep and MaxBridgedAuthorities in Millau benchmarks #1851

Merged
merged 3 commits into from
Feb 6, 2023
Merged

Fix HeadersToKeep and MaxBridgedAuthorities in Millau benchmarks #1851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1d9ad9f
fix `HeadersToKeep` and `MaxBridgedAuthorities` in Millau benchmarks
svyatonik Feb 6, 2023
785645b
typo
svyatonik Feb 6, 2023
7cd8df0
impl review suggestion
svyatonik Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 2 additions & 20 deletions bin/millau/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,30 +387,12 @@ impl pallet_bridge_relayers::Config for Runtime {
type WeightInfo = ();
}

#[cfg(feature = "runtime-benchmarks")]
parameter_types! {
/// Number of headers to keep in benchmarks.
///
/// In benchmarks we always populate with full number of `HeadersToKeep` to make sure that
/// pruning is taken into account.
///
/// Note: This is lower than regular value, to speed up benchmarking setup.
pub const HeadersToKeep: u32 = 1024;
/// Maximal number of authorities at Rialto.
///
/// In benchmarks we're using sets of up to `1024` authorities to prepare for possible
/// upgrades in the future and see if performance degrades when number of authorities
/// grow.
pub const MaxAuthoritiesAtRialto: u32 = pallet_bridge_grandpa::benchmarking::MAX_VALIDATOR_SET_SIZE;
}

#[cfg(not(feature = "runtime-benchmarks"))]
parameter_types! {
/// Number of headers to keep.
///
/// Assuming the worst case of every header being finalized, we will keep headers at least for a
/// week.
pub const HeadersToKeep: u32 = 7 * bp_rialto::DAYS;
/// day.
pub const HeadersToKeep: u32 = bp_rialto::DAYS;
/// Maximal number of authorities at Rialto.
pub const MaxAuthoritiesAtRialto: u32 = bp_rialto::MAX_AUTHORITIES_COUNT;
}
Expand Down
4 changes: 2 additions & 2 deletions bin/rialto-parachain/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,8 +533,8 @@ parameter_types! {
/// Number of headers to keep.
///
/// Assuming the worst case of every header being finalized, we will keep headers at least for a
/// week.
pub const HeadersToKeep: u32 = 7 * bp_millau::DAYS as u32;
/// day.
pub const HeadersToKeep: u32 = bp_millau::DAYS as u32;

/// Maximal number of authorities at Millau.
pub const MaxAuthoritiesAtMillau: u32 = bp_millau::MAX_AUTHORITIES_COUNT;
Expand Down
4 changes: 2 additions & 2 deletions bin/rialto/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,8 @@ parameter_types! {
/// Number of headers to keep.
///
/// Assuming the worst case of every header being finalized, we will keep headers at least for a
/// week.
pub const HeadersToKeep: u32 = 7 * bp_rialto::DAYS;
/// day.
pub const HeadersToKeep: u32 = bp_rialto::DAYS;

/// Maximal number of authorities at Millau.
pub const MaxAuthoritiesAtMillau: u32 = bp_millau::MAX_AUTHORITIES_COUNT;
Expand Down
33 changes: 18 additions & 15 deletions modules/grandpa/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,26 +53,29 @@ use sp_finality_grandpa::AuthorityId;
use sp_runtime::traits::Zero;
use sp_std::vec::Vec;

// The maximum number of vote ancestries to include in a justification.
//
// In practice this would be limited by the session length (number of blocks a single authority set
// can produce) of a given chain.
/// The maximum number of vote ancestries to include in a justification.
///
/// In practice this would be limited by the session length (number of blocks a single authority set
/// can produce) of a given chain.
const MAX_VOTE_ANCESTRIES: u32 = 1000;

// The maximum number of pre-commits to include in a justification. In practice this scales with the
// number of validators.
pub const MAX_VALIDATOR_SET_SIZE: u32 = 1024;

// `1..MAX_VALIDATOR_SET_SIZE` and `1..MAX_VOTE_ANCESTRIES` are too large && benchmarks are
// running for almost 40m (steps=50, repeat=20) on a decent laptop, which is too much. Since
// we're building linear function here, let's just select some limited subrange for benchmarking.
const VALIDATOR_SET_SIZE_RANGE_BEGIN: u32 = MAX_VALIDATOR_SET_SIZE / 20;
const VALIDATOR_SET_SIZE_RANGE_END: u32 =
VALIDATOR_SET_SIZE_RANGE_BEGIN + VALIDATOR_SET_SIZE_RANGE_BEGIN;
// `1..MAX_VOTE_ANCESTRIES` is too large && benchmarks are running for almost 40m (steps=50,
// repeat=20) on a decent laptop, which is too much. Since we're building linear function here,
// let's just select some limited subrange for benchmarking.
const MAX_VOTE_ANCESTRIES_RANGE_BEGIN: u32 = MAX_VOTE_ANCESTRIES / 20;
const MAX_VOTE_ANCESTRIES_RANGE_END: u32 =
MAX_VOTE_ANCESTRIES_RANGE_BEGIN + MAX_VOTE_ANCESTRIES_RANGE_BEGIN;

// the same with validators - if there are too much validators, let's run benchmarks on subrange
fn validator_set_range_end<T: Config<I>, I: 'static>() -> u32 {
let max_bridged_authorities = T::MaxBridgedAuthorities::get();
if max_bridged_authorities > 128 {
sp_std::cmp::max(128, max_bridged_authorities / 5)
} else {
max_bridged_authorities
}
}

/// Returns number of first header to be imported.
///
/// Since we bootstrap the pallet with `HeadersToKeep` already imported headers,
Expand Down Expand Up @@ -117,7 +120,7 @@ benchmarks_instance_pallet! {
// This is the "gold standard" benchmark for this extrinsic, and it's what should be used to
// annotate the weight in the pallet.
submit_finality_proof {
let p in VALIDATOR_SET_SIZE_RANGE_BEGIN..VALIDATOR_SET_SIZE_RANGE_END;
let p in 1 .. validator_set_range_end::<T, I>();
let v in MAX_VOTE_ANCESTRIES_RANGE_BEGIN..MAX_VOTE_ANCESTRIES_RANGE_END;
let caller: T::AccountId = whitelisted_caller();
let (header, justification) = prepare_benchmark_data::<T, I>(p, v);
Expand Down
21 changes: 19 additions & 2 deletions modules/grandpa/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ pub mod pallet {
#[pallet::constant]
type MaxRequests: Get<u32>;

// Avoid using `HeadersToKeep` directly in the pallet code. Use `headers_to_keep` function
// instead.

/// Maximal number of finalized headers to keep in the storage.
///
/// The setting is there to prevent growing the on-chain state indefinitely. Note
Expand Down Expand Up @@ -464,6 +467,20 @@ pub mod pallet {
})?)
}

/// Return number of headers to keep in the runtime storage.
#[cfg(not(feature = "runtime-benchmarks"))]
pub(crate) fn headers_to_keep<T: Config<I>, I: 'static>() -> u32 {
T::HeadersToKeep::get()
}

/// Return number of headers to keep in the runtime storage.
#[cfg(feature = "runtime-benchmarks")]
pub(crate) fn headers_to_keep<T: Config<I>, I: 'static>() -> u32 {
// every db operation (significantly) slows down benchmarks, so let's keep as min as
// possible
2
}
Comment on lines +471 to +482
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Could we do this distinction in the mock runtime instead ? Is the mock runtime used for benchmarks ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, benchmarks are using the actual runtimes. That's why the weights of our pallets may be (are) different on Rialto, on Millau and on WBH/RBH. So every runtime generates its own weights.rs file (e.g. for frame-system ; https://github.com/paritytech/polkadot/blob/master/runtime/rococo/src/weights/frame_system.rs, https://github.com/paritytech/substrate/blob/master/frame/system/src/weights.rs, https://github.com/paritytech/cumulus/blob/master/parachains/runtimes/bridge-hubs/bridge-hub-rococo/src/weights/frame_system.rs)


/// Import a previously verified header to the storage.
///
/// Note this function solely takes care of updating the storage and pruning old entries,
Expand All @@ -479,7 +496,7 @@ pub mod pallet {
<ImportedHashes<T, I>>::insert(index, hash);

// Update ring buffer pointer and remove old header.
<ImportedHashesPointer<T, I>>::put((index + 1) % T::HeadersToKeep::get());
<ImportedHashesPointer<T, I>>::put((index + 1) % headers_to_keep::<T, I>());
if let Ok(hash) = pruning {
log::debug!(target: LOG_TARGET, "Pruning old header: {:?}.", hash);
<ImportedHeaders<T, I>>::remove(hash);
Expand Down Expand Up @@ -523,7 +540,7 @@ pub mod pallet {
init_params: super::InitializationData<BridgedHeader<T, I>>,
) {
let start_number = *init_params.header.number();
let end_number = start_number + T::HeadersToKeep::get().into();
let end_number = start_number + headers_to_keep::<T, I>().into();
initialize_bridge::<T, I>(init_params).expect("benchmarks are correct");

let mut number = start_number;
Expand Down
44 changes: 22 additions & 22 deletions modules/grandpa/src/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ impl<T: frame_system::Config> WeightInfo for BridgeWeight<T> {
///
/// Storage: BridgeRialtoGrandpa CurrentAuthoritySet (r:1 w:0)
///
/// Proof: BridgeRialtoGrandpa CurrentAuthoritySet (max_values: Some(1), max_size: Some(40970),
/// added: 41465, mode: MaxEncodedLen)
/// Proof: BridgeRialtoGrandpa CurrentAuthoritySet (max_values: Some(1), max_size: Some(209),
/// added: 704, mode: MaxEncodedLen)
///
/// Storage: BridgeRialtoGrandpa ImportedHashesPointer (r:1 w:1)
///
Expand All @@ -93,19 +93,19 @@ impl<T: frame_system::Config> WeightInfo for BridgeWeight<T> {
/// Proof: BridgeRialtoGrandpa ImportedHeaders (max_values: None, max_size: Some(68), added:
/// 2543, mode: MaxEncodedLen)
///
/// The range of component `p` is `[51, 102]`.
/// The range of component `p` is `[1, 5]`.
///
/// The range of component `v` is `[50, 100]`.
fn submit_finality_proof(p: u32, v: u32) -> Weight {
// Proof Size summary in bytes:
// Measured: `2524 + p * (40 ±0)`
// Estimated: `46001`
// Minimum execution time: 2_282_140 nanoseconds.
Weight::from_parts(142_496_714, 46001)
// Standard Error: 32_796
.saturating_add(Weight::from_ref_time(40_232_935).saturating_mul(p.into()))
// Standard Error: 33_574
.saturating_add(Weight::from_ref_time(1_185_407).saturating_mul(v.into()))
// Measured: `459 + p * (40 ±0)`
// Estimated: `5240`
// Minimum execution time: 368_734 nanoseconds.
Weight::from_parts(64_214_587, 5240)
// Standard Error: 226_504
.saturating_add(Weight::from_ref_time(41_231_918).saturating_mul(p.into()))
// Standard Error: 20_667
.saturating_add(Weight::from_ref_time(2_770_962).saturating_mul(v.into()))
.saturating_add(T::DbWeight::get().reads(6_u64))
.saturating_add(T::DbWeight::get().writes(6_u64))
}
Expand All @@ -130,8 +130,8 @@ impl WeightInfo for () {
///
/// Storage: BridgeRialtoGrandpa CurrentAuthoritySet (r:1 w:0)
///
/// Proof: BridgeRialtoGrandpa CurrentAuthoritySet (max_values: Some(1), max_size: Some(40970),
/// added: 41465, mode: MaxEncodedLen)
/// Proof: BridgeRialtoGrandpa CurrentAuthoritySet (max_values: Some(1), max_size: Some(209),
/// added: 704, mode: MaxEncodedLen)
///
/// Storage: BridgeRialtoGrandpa ImportedHashesPointer (r:1 w:1)
///
Expand All @@ -148,19 +148,19 @@ impl WeightInfo for () {
/// Proof: BridgeRialtoGrandpa ImportedHeaders (max_values: None, max_size: Some(68), added:
/// 2543, mode: MaxEncodedLen)
///
/// The range of component `p` is `[51, 102]`.
/// The range of component `p` is `[1, 5]`.
///
/// The range of component `v` is `[50, 100]`.
fn submit_finality_proof(p: u32, v: u32) -> Weight {
// Proof Size summary in bytes:
// Measured: `2524 + p * (40 ±0)`
// Estimated: `46001`
// Minimum execution time: 2_282_140 nanoseconds.
Weight::from_parts(142_496_714, 46001)
// Standard Error: 32_796
.saturating_add(Weight::from_ref_time(40_232_935).saturating_mul(p.into()))
// Standard Error: 33_574
.saturating_add(Weight::from_ref_time(1_185_407).saturating_mul(v.into()))
// Measured: `459 + p * (40 ±0)`
// Estimated: `5240`
// Minimum execution time: 368_734 nanoseconds.
Weight::from_parts(64_214_587, 5240)
// Standard Error: 226_504
.saturating_add(Weight::from_ref_time(41_231_918).saturating_mul(p.into()))
// Standard Error: 20_667
.saturating_add(Weight::from_ref_time(2_770_962).saturating_mul(v.into()))
.saturating_add(RocksDbWeight::get().reads(6_u64))
.saturating_add(RocksDbWeight::get().writes(6_u64))
}
Expand Down