Skip to content

Fix and update benchmarks #1494

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

Merged
svyatonik merged 6 commits into from
Jul 7, 2022
Merged

Fix and update benchmarks #1494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6c1babe
decrease parameters range in grandpa benchmarks
svyatonik Jul 6, 2022
2b3721a
fix messages benchmarks
svyatonik Jul 6, 2022
d02605c
update all weights
svyatonik Jul 6, 2022
0379d24
dealing with failed test (WiP)
svyatonik Jul 6, 2022
087f71c
Revert "dealing with failed test (WiP)"
svyatonik Jul 7, 2022
503f8c4
proper tests fix
svyatonik Jul 7, 2022
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
14 changes: 12 additions & 2 deletions modules/grandpa/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ const MAX_VOTE_ANCESTRIES: u32 = 1000;
// number of validators.
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;
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;
Comment on lines +66 to +74
Copy link
Collaborator

@serban300 serban300 Jul 19, 2022

Choose a reason for hiding this comment

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

Would it make sense to use an interval that's closer to the worst case scenario ? Or it wouldn't make a difference ?

Copy link
Contributor Author

@svyatonik svyatonik Jul 19, 2022

Choose a reason for hiding this comment

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

When working on this PR I've ran benchmarks for the whole range, for the closer-to-worst-scenario-interval and for that interval. Everywhere results were almost the same. So imo it isn't that important

Copy link
Contributor Author

@svyatonik svyatonik Jul 19, 2022

Choose a reason for hiding this comment

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

Actually, I've made a comment regarding that in description :)


/// Returns number of first header to be imported.
///
/// Since we bootstrap the pallet with `HeadersToKeep` already imported headers,
Expand Down Expand Up @@ -107,8 +117,8 @@ 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 1..MAX_VALIDATOR_SET_SIZE;
let v in 1..MAX_VOTE_ANCESTRIES;
let p in VALIDATOR_SET_SIZE_RANGE_BEGIN..VALIDATOR_SET_SIZE_RANGE_END;
let v in MAX_VOTE_ANCESTRIES_RANGE_BEGIN..MAX_VOTE_ANCESTRIES_RANGE_END;
Comment on lines +120 to +121
Copy link
Collaborator

@serban300 serban300 Jul 19, 2022

Choose a reason for hiding this comment

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

Since we limited these ranges should we use less steps also ? Since the ranges are more or less 50..100, 50 steps would mean testing each value in this range individually. Would it make sense to use for example 25 steps now in order to generate the benchmarks even faster ?

Copy link
Contributor Author

@svyatonik svyatonik Jul 19, 2022

Choose a reason for hiding this comment

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

50 is kinda standard (don't ask me why :) ) across all repos/pallets: https://github.com/paritytech/polkadot/blob/750fcd92288a8f9637f3757b1c3dc874bd397ad5/scripts/ci/run_benches_for_runtime.sh and https://github.com/paritytech/cumulus/blob/master/scripts/benchmarks-ci.sh . We may change it in our repo, but the goal is to make benchmarks run faster when non-test runtime weights are updated. And it happens in other repos, where we can't change that without reason.

Copy link
Collaborator

@serban300 serban300 Jul 19, 2022

Choose a reason for hiding this comment

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

Ok, I see. I'm ok with leaving 50 as well if it's standard.

let caller: T::AccountId = whitelisted_caller();
let (header, justification) = prepare_benchmark_data::<T, I>(p, v);
}: submit_finality_proof(RawOrigin::Signed(caller), Box::new(header), justification)
Expand Down
17 changes: 9 additions & 8 deletions modules/grandpa/src/weights.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,15 @@
//! Autogenerated weights for `pallet_bridge_grandpa`
//!
//! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev
//! DATE: 2021-12-28, STEPS: 50, REPEAT: 20
//! DATE: 2022-07-06, STEPS: 50, REPEAT: 20
//! LOW RANGE: [], HIGH RANGE: []
//! EXECUTION: Some(Wasm), WASM-EXECUTION: Compiled
//! CHAIN: Some("dev"), DB CACHE: 128
//! CHAIN: Some("dev"), DB CACHE: 1024

// Executed Command:
// target/release/millau-bridge-node
// benchmark
// pallet
// --chain=dev
// --steps=50
// --repeat=20
Expand Down Expand Up @@ -55,9 +56,9 @@ pub trait WeightInfo {
pub struct MillauWeight<T>(PhantomData<T>);
impl<T: frame_system::Config> WeightInfo for MillauWeight<T> {
fn submit_finality_proof(p: u32, v: u32) -> Weight {
(115_651_000 as Weight)
.saturating_add((61_465_000 as Weight).saturating_mul(p as Weight))
.saturating_add((3_438_000 as Weight).saturating_mul(v as Weight))
(55_070_000 as Weight)
Copy link
Collaborator

@serban300 serban300 Jul 19, 2022

Choose a reason for hiding this comment

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

The difference between the old value and the new one seems quite big. Here and in other places as well. Is this normal / expected ?

Copy link
Contributor Author

@svyatonik svyatonik Jul 19, 2022

Choose a reason for hiding this comment

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

Yeah. I've also made a comment regarding that in the PR description. Outline: normally (for non-test runtimes) we are running benchmarks (using benchmarking bot) on the dedicated reference machine. But here (for testnets) we don't need such precision, so I've been using my own laptop for that && recently I've upgraded it :) Additionally, the messages pallet configuration has changed significantly, so it is normal that we see drop there

.saturating_add((39_678_000 as Weight).saturating_mul(p as Weight))
.saturating_add((1_540_000 as Weight).saturating_mul(v as Weight))
.saturating_add(T::DbWeight::get().reads(7 as Weight))
.saturating_add(T::DbWeight::get().writes(6 as Weight))
}
Expand All @@ -66,9 +67,9 @@ impl<T: frame_system::Config> WeightInfo for MillauWeight<T> {
// For backwards compatibility and tests
impl WeightInfo for () {
fn submit_finality_proof(p: u32, v: u32) -> Weight {
(115_651_000 as Weight)
.saturating_add((61_465_000 as Weight).saturating_mul(p as Weight))
.saturating_add((3_438_000 as Weight).saturating_mul(v as Weight))
(55_070_000 as Weight)
.saturating_add((39_678_000 as Weight).saturating_mul(p as Weight))
.saturating_add((1_540_000 as Weight).saturating_mul(v as Weight))
.saturating_add(RocksDbWeight::get().reads(7 as Weight))
.saturating_add(RocksDbWeight::get().writes(6 as Weight))
}
Expand Down
38 changes: 14 additions & 24 deletions modules/messages/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@
use crate::{
inbound_lane::InboundLaneStorage, inbound_lane_storage, outbound_lane,
outbound_lane::ReceivalConfirmationResult, weights_ext::EXPECTED_DEFAULT_MESSAGE_LENGTH, Call,
OutboundLanes, OutboundMessages,
};

use bp_messages::{
source_chain::TargetHeaderChain, target_chain::SourceHeaderChain, DeliveredMessages,
InboundLaneData, LaneId, MessageData, MessageNonce, OutboundLaneData, UnrewardedRelayer,
UnrewardedRelayersState,
InboundLaneData, LaneId, MessageData, MessageKey, MessageNonce, OutboundLaneData,
UnrewardedRelayer, UnrewardedRelayersState,
};
use bp_runtime::{messages::DispatchFeePayment, StorageProofSize};
use frame_benchmarking::{account, benchmarks_instance_pallet};
Expand Down Expand Up @@ -244,7 +245,10 @@ benchmarks_instance_pallet! {
send_regular_message_with_payload::<T, I>(vec![42u8; T::maximal_message_size() as _]);
}: increase_message_fee(RawOrigin::Signed(sender.clone()), lane_id, nonce, additional_fee)
verify {
assert_eq!(T::account_balance(&sender), 0.into());
assert_eq!(
OutboundMessages::<T, I>::get(MessageKey { lane_id: T::bench_lane_id(), nonce }).unwrap().fee,
T::message_fee() + additional_fee,
);
}

// Benchmark `increase_message_fee` with following conditions:
Expand All @@ -265,7 +269,10 @@ benchmarks_instance_pallet! {
send_regular_message_with_payload::<T, I>(vec![42u8; i as _]);
}: increase_message_fee(RawOrigin::Signed(sender.clone()), lane_id, nonce, additional_fee)
verify {
assert_eq!(T::account_balance(&sender), 0.into());
assert_eq!(
OutboundMessages::<T, I>::get(MessageKey { lane_id: T::bench_lane_id(), nonce }).unwrap().fee,
Copy link
Collaborator

@serban300 serban300 Jul 19, 2022

Choose a reason for hiding this comment

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

Nit: I think we could use lane_id directly here: OutboundMessages::<T, I>::get(MessageKey { lane_id, nonce }).unwrap().fee. Just saying, but I'm ok with leaving it as it is.

Copy link
Contributor Author

@svyatonik svyatonik Jul 19, 2022

Choose a reason for hiding this comment

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

Thanks! :) I've submitted #1514 - will fix a bit later

T::message_fee() + additional_fee,
);
}

// Benchmark `receive_messages_proof` extrinsic with single minimal-weight message and following conditions:
Expand Down Expand Up @@ -508,10 +515,7 @@ benchmarks_instance_pallet! {
});
}: receive_messages_delivery_proof(RawOrigin::Signed(relayer_id.clone()), proof, relayers_state)
verify {
assert_eq!(
T::account_balance(&relayer_id),
relayer_balance + T::message_fee(),
);
assert_eq!(OutboundLanes::<T, I>::get(T::bench_lane_id()).latest_received_nonce, 1);
}

// Benchmark `receive_messages_delivery_proof` extrinsic with following conditions:
Expand Down Expand Up @@ -552,7 +556,7 @@ benchmarks_instance_pallet! {
});
}: receive_messages_delivery_proof(RawOrigin::Signed(relayer_id.clone()), proof, relayers_state)
verify {
ensure_relayer_rewarded::<T, I>(&relayer_id, &relayer_balance);
assert_eq!(OutboundLanes::<T, I>::get(T::bench_lane_id()).latest_received_nonce, 2);
}

// Benchmark `receive_messages_delivery_proof` extrinsic with following conditions:
Expand Down Expand Up @@ -599,8 +603,7 @@ benchmarks_instance_pallet! {
});
}: receive_messages_delivery_proof(RawOrigin::Signed(relayer1_id.clone()), proof, relayers_state)
verify {
ensure_relayer_rewarded::<T, I>(&relayer1_id, &relayer1_balance);
ensure_relayer_rewarded::<T, I>(&relayer2_id, &relayer2_balance);
assert_eq!(OutboundLanes::<T, I>::get(T::bench_lane_id()).latest_received_nonce, 2);
}
}

Expand Down Expand Up @@ -642,16 +645,3 @@ fn receive_messages<T: Config<I>, I: 'static>(nonce: MessageNonce) {
last_confirmed_nonce: 0,
});
}

fn ensure_relayer_rewarded<T: Config<I>, I: 'static>(
relayer_id: &T::AccountId,
old_balance: &T::OutboundMessageFee,
) {
let new_balance = T::account_balance(relayer_id);
assert!(
new_balance > *old_balance,
"Relayer haven't received reward for relaying message: old balance = {:?}, new balance = {:?}",
old_balance,
new_balance,
);
}
Loading