Fix and update benchmarks #1494

svyatonik · 2022-07-06T12:23:30Z

All weights are decreased a bit. There are two main reasons: (1) messages pallet configuration has changed significantly and (2) my new laptop is a bit faster than my old one, where previous results have been gathered (yes, we need to run benchmarks on the reference machine, but it is fine to use test weights on our testnets).

Apart from weights update, this PR also:

fixes broken messages pallet benchmarks - they're broken since instant-payments removal. I've changed it to something more robust;
decreases ranges for GRANDPA pallet benchmarks components. I've ran it with three different configurations: (1) old version (2) new version and (3) configuration, where subranges are shifted towards the "big" range end. Everywhere results were almost similar, but the updated version is the fastest one, so let's use this new configuration;
fixes the failing messages-pallet-weights-correctness test - this is still WiP, since it is very important part of the pallet. I'm gonna sleep over it && review it once again => draft

This reverts commit 0379d24.

serban300

Even though I did some research these days, I'm not familiar with benchmarking. I left some comments / questions. I hope they make sense.

serban300 · 2022-07-19T06:48:08Z

modules/grandpa/src/benchmarking.rs

+		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;


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 ?

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.

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

serban300 · 2022-07-19T07:02:45Z

modules/grandpa/src/benchmarking.rs

+// `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;


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

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

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

serban300 · 2022-07-19T07:54:19Z

modules/messages/src/benchmarking.rs

 	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,


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.

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

serban300 · 2022-07-19T08:08:07Z

modules/grandpa/src/weights.rs

-		(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)


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

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

…ets parachains. (#1340) (#1494) * Transaction version bump + updated spec_version * Reformatting issue * Adding missing runtimes * Upgrading contracts pallet runtime version * Upgrading Seedling runtime * Bump polkadot-parachain version Co-authored-by: Wilfried Kopp <[email protected]> Co-authored-by: Hector Bulgarini <[email protected]> Co-authored-by: Wilfried Kopp <[email protected]>

* decrease parameters range in grandpa benchmarks * fix messages benchmarks * update all weights * dealing with failed test (WiP) * Revert "dealing with failed test (WiP)" This reverts commit 0379d24. * proper tests fix

svyatonik added 4 commits July 6, 2022 14:09

decrease parameters range in grandpa benchmarks

6c1babe

fix messages benchmarks

2b3721a

update all weights

d02605c

dealing with failed test (WiP)

0379d24

svyatonik added the A-chores Something that has to be done, as part of regular maintenance label Jul 6, 2022

svyatonik added 2 commits July 7, 2022 10:49

Revert "dealing with failed test (WiP)"

087f71c

This reverts commit 0379d24.

proper tests fix

503f8c4

svyatonik marked this pull request as ready for review July 7, 2022 08:09

svyatonik merged commit f3d7986 into master Jul 7, 2022

svyatonik deleted the fix-and-update-benchmarks branch July 7, 2022 08:29

serban300 reviewed Jul 19, 2022

View reviewed changes

svyatonik mentioned this pull request Jul 19, 2022

Use lane_id variable directly in benchmarks #1514

Closed

wuminzhe mentioned this pull request Aug 9, 2022

Sync upstream darwinia-network/darwinia-messages-substrate#174

Closed

30 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix and update benchmarks #1494

Fix and update benchmarks #1494

Uh oh!

svyatonik commented Jul 6, 2022

Uh oh!

serban300 left a comment

Uh oh!

serban300 Jul 19, 2022

Uh oh!

svyatonik Jul 19, 2022

Uh oh!

serban300 Jul 19, 2022

Uh oh!

serban300 Jul 19, 2022

Uh oh!

svyatonik Jul 19, 2022

Uh oh!

svyatonik Jul 19, 2022

Uh oh!

serban300 Jul 19, 2022

Uh oh!

svyatonik Jul 19, 2022

Uh oh!

serban300 Jul 19, 2022

Uh oh!

svyatonik Jul 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		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;

Fix and update benchmarks #1494

Fix and update benchmarks #1494

Uh oh!

Conversation

svyatonik commented Jul 6, 2022

Uh oh!

serban300 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants