HRMP benchmarks #3876

kianenigma · 2021-09-17T12:27:59Z

as you see, nothing is done, just opening a PR to signal that I will work on this and prevent overlap.

Weigh the background-like tasks, and make all of the inter-pallet calls self-weiging. Could be done in a follow-up. All dispatchables area already being weighed.

part of #3850

runtime/kusama/src/lib.rs

runtime/parachains/src/hrmp.rs

runtime/parachains/src/paras.rs

…h-hrmp

kianenigma · 2021-09-21T10:17:39Z

runtime/parachains/src/configuration.rs

+		/// `hrmp_max_parathread_outbound_channels` can be set to.
+		///
+		/// This is used for benchmarking sanely bounding relevant storage items.
+		type HrmpMaxOutboundChannelsBound: Get<u32>;


Long term, I don't think this patter is good. We probably will need to add a lot of bound types here, one for each field of HostConfigurations. I don't see it within the scope of this PR, but if we go down that road, it is probably worthwhile to abstract all of these types into one new type, maybe called struct HostConfigurationBounds and only use that to minimize the footprint in the interface.

Also, note that for simplicity, for now I have combined the parachain and parathread variant. This might help us use BoundedVec in some places as well, but that didn't work out.

kianenigma · 2021-09-21T10:28:26Z

runtime/kusama/src/lib.rs

@@ -1134,6 +1134,8 @@ impl parachains_origin::Config for Runtime {}

 impl parachains_configuration::Config for Runtime {
 	type WeightInfo = weights::runtime_parachains_configuration::WeightInfo<Runtime>;
+	type HrmpMaxOutboundChannelsBound = ConstU32<128>;
+	type HrmpMaxInboundChannelsBound = ConstU32<128>;


rather arbitrary numbers.

pepyakin

It looks fine, assuming the formatting changes will be reverted.

PS: For the future, I would really appreciate if a PR didn't include so much formatting. It complicates reviewing as it takes effort to see what actually requires attention.

runtime/parachains/src/hrmp.rs

kianenigma · 2021-09-21T12:22:10Z

It looks fine, assuming the formatting changes will be reverted.

PS: For the future, I would really appreciate if a PR didn't include so much formatting. It complicates reviewing as it takes effort to see what actually requires attention.

Yeah, I am having some weird issues with cargo fmt: #3876 (comment).

Only ones that I did fix intentionally was that your comments were sometimes were not wrapped at ~~120~~ 100. Only a handful, and should not make review that hard.

pepyakin

Ah, actually, I also assumed that comments underwent some kind of automatic formatting.

I am really sensitive about the line width and never go over 120. The ones you wrapped looked more like they were 100, which I believe is fine to do occasionally, if that improves reading experience. In some instances it is fine and I do not feel strongly, but in others (I mentioned them below) I do.

runtime/parachains/src/hrmp.rs

kianenigma · 2021-09-21T13:01:08Z

Ah, actually, I also assumed that comments underwent some kind of automatic formatting.

I am really sensitive about the line width and never go over 120. The ones you wrapped looked more like they were 100, which I believe is fine to do occasionally, if that improves reading experience. In some instances it is fine and I do not feel strongly, but in others (I mentioned them below) I do.

Fixed your feedbacks, indeed my fixings (which where done with a plugin, not manually) made a few things a bit unclear.

And indeed I try and enforce 100 where possible, what I said earlier about 120 was wrong.

drahnr

Changes look reasonable to me, I did not do a deep dive on the parameterization logic of the actual benchmarking code.

runtime/parachains/src/hrmp.rs

Co-authored-by: Zeke Mostov <[email protected]>

pepyakin

A couple of fixes and good to go

runtime/polkadot/src/weights/runtime_parachains_hrmp.rs

runtime/parachains/src/hrmp.rs

pepyakin · 2022-01-20T18:12:17Z

runtime/parachains/src/hrmp.rs

-		pub fn force_process_hrmp_open(origin: OriginFor<T>) -> DispatchResult {
+		///
+		/// Total number of opening channels must be provided as witness data of weighing.
+		#[pallet::weight(<T as Config>::WeightInfo::force_process_hrmp_open(*_channels))]


This appears to be not checked. Do you think this right? It also applies to other places above and below.

In any case, this looks weird for reviewers/auditors and as such required a comment.

It is not checked because it is coming from root origin and it is assumed to be correct, but I could easily add that, but IMO it is just noise. It will clutter both the dispatch code and the error variant.

About the comment, if you look around Frame, this a pretty common patter, where some additional data needs to be passed to the dispatch, sometime not really used other than checked for correctness, just to measure the weight of some transaction ahead of time and have efficient pool processing. These are typically called "witness data".

pepyakin · 2022-01-20T18:16:41Z

runtime/parachains/src/configuration.rs

@@ -1005,6 +1020,7 @@ pub mod pallet {
 			new: u32,
 		) -> DispatchResult {
 			ensure_root(origin)?;
+			ensure!(new <= T::HrmpMaxOutboundChannelsBound::get(), Error::<T>::InvalidNewValue);


ensuring the conditions in setters are now discouraged in favor of checking it centrally in fn check_consistency

I settled this in e22a79f, LMK what you think.

So, I will also change this in Ump? #3889

i missed your comment... ༼ ༎ຶ ෴ ༎ຶ༽

But that seems to be the same cases

pepyakin · 2022-01-20T18:18:42Z

runtime/parachains/src/hrmp/benchmarking.rs

+		// ingress channels to a single leaving parachain that need to be closed.
+		let i in 0 .. (<T as configuration::Config>::HrmpMaxInboundChannelsBound::get() - 1);
+		// egress channels to a single leaving parachain that need to be closed.
+		let e in 0 .. (<T as configuration::Config>::HrmpMaxOutboundChannelsBound::get() - 1);


Just out of my inexperience: why do we check it for every variable and just not once for the bound const variable?

(if I have understood your question correctly:) We want the benchmarking machinery to tell us the dependency on i and e, for which you need to run the benchmark in a range of values for both i and e. The final outcome of this would be fn(i, e) -> weight.

But why do we want us to tell the dependency on those? i and e are constants anyway?

their upper bound is constant and we happen to use that here as well, If you see where T::WeightInfo::force_clean_hrmp is being called, these two parameters need to be provided (similar to the other comment, it is a root call and we expect root to give a proper estimate of the witness data)

pepyakin · 2022-01-20T18:21:30Z

runtime/parachains/src/hrmp/benchmarking.rs

+	let channel_id = HrmpChannelId { sender, recipient };
+	assert_ok!(Hrmp::<T>::hrmp_close_channel(sender_origin.clone().into(), channel_id));
+	if matches!(until, ParachainSetupStep::CloseRequested) {
+		// NOTE: this is just for expressiveness, otherwise the if-statement is 100% useless.


Nit: This function has an obvious code pattern which makes this comment superfluous. It does not harm though.

runtime/parachains/src/hrmp/benchmarking.rs

pepyakin · 2022-01-20T18:37:46Z

runtime/parachains/src/hrmp/benchmarking.rs

+	from: u32,
+	to: u32,
+	until: ParachainSetupStep,
+) -> [(ParaId, crate::Origin); 2]


Nit: You probably know what I am going to say about this.

In fact I don't, enlighten me please.

Use a tuple instead and/or define a helper struct :)

What is the rationale? an array of two is perfectly fine in my opinion and practically synonym to a tuple of two in my head.

Sorry, I assumed too much.

While I think an array of two is definitely uncommon in such cases, and by default I would expect a tuple in cases like this. However, I agree it's more or less synonymous (It seems a bit novel and I even like it a bit compared to a tuple, because you use different brackets for the outer and inner items)

I, however, meant to propose to use a struct instead of the array. The inner tuples are OK since they are at least typed. That has an advantage that you fix names in one place — at the declaration side compared to the each place (which promotes consistency). Names are a bit better in this setting compared to indices:

What's more clear to refer the recipient, a field named recipient or an element at index 1?

emostov

👍

…city.

…kiz-bench-hrmp

…used for benchmarks as well.

pepyakin · 2022-01-21T10:11:32Z

runtime/parachains/src/configuration.rs

+	/// Maximum number of HRMP outbound channels exceeded.
+	MaxHrmpOutboundChannelsExceeded,
+	/// Maximum number of HRMP inbound channels exceeded.
+	MaxHrmpInboundChannelsExceeded,


Nit: mention the value for consistency with other similar variants?

kianenigma · 2022-01-21T12:53:42Z

bot merge

paritytech-processbot · 2022-01-21T12:53:52Z

Waiting for commit status.

* wip template for hrmp benchmarks * add all of the benchmarks, first draft * File was not saved :/ * cargo +nightly fmt * Use configs * add configs * Fix rococo * Final touches * revert fmt changes, one last time * Fix wrappings * Fix a bunch of tests * cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=kusama-dev --steps=50 --repeat=20 --pallet=runtime_parachains::hrmp --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/kusama/src/weights/runtime_parachains_hrmp.rs * add to westend * actually use everything * cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=runtime_parachains::hrmp --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/runtime_parachains_hrmp.rs * Update runtime/parachains/src/hrmp.rs Co-authored-by: Keith Yeung <[email protected]> * use real weight in wnd * reorg * minor cleanup * weigh some of the internal stuff as well * cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=kusama-dev --steps=50 --repeat=20 --pallet=runtime_parachains::hrmp --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/kusama/src/weights/runtime_parachains_hrmp.rs * cargo run --quiet --release --features=runtime-benchmarks -- benchmark --chain=westend-dev --steps=50 --repeat=20 --pallet=runtime_parachains::hrmp --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --header=./file_header.txt --output=./runtime/westend/src/weights/runtime_parachains_hrmp.rs * add files * Master.into() * add validation * fmt * fmt * final fixes * all runtimes build * undo formatting * Update runtime/parachains/src/hrmp.rs Co-authored-by: Zeke Mostov <[email protected]> * non-controversial changes * do it the parachain-way: use const instead of type configs for simplicity. * borrow assert_storage_consistency_exhaustive * move assert_storage_consistency_exhaustive to Pallet, so it can be reused for benchmarks as well. * fix typo Co-authored-by: Parity Bot <[email protected]> Co-authored-by: Keith Yeung <[email protected]> Co-authored-by: Sergey Shulepov <[email protected]> Co-authored-by: Zeke Mostov <[email protected]>

wip template for hrmp benchmarks

743bbbe

kianenigma mentioned this pull request Sep 17, 2021

Benchmark Parachains Runtime: Tracking Issue #3850

Closed

14 tasks

kianenigma added 2 commits September 20, 2021 19:18

add all of the benchmarks, first draft

4c1e545

Master.into()

d9cc590

kianenigma changed the title ~~WIP: HRMP benchmarks~~ HRMP benchmarks Sep 20, 2021

File was not saved :/

42ea875

kianenigma marked this pull request as ready for review September 20, 2021 17:25

kianenigma requested review from shawntabrizi, KiChjang and pepyakin September 20, 2021 17:25

kianenigma commented Sep 20, 2021

View reviewed changes

runtime/kusama/src/lib.rs Outdated Show resolved Hide resolved

cargo +nightly fmt

477731c

kianenigma commented Sep 20, 2021

View reviewed changes

runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

shawntabrizi reviewed Sep 21, 2021

View reviewed changes

runtime/parachains/src/paras.rs Outdated Show resolved Hide resolved

shawntabrizi reviewed Sep 21, 2021

View reviewed changes

runtime/parachains/src/paras.rs Outdated Show resolved Hide resolved

kianenigma added 4 commits September 21, 2021 12:07

Use configs

e7aadf5

add configs

9f0d525

Merge branch 'master' of github.com:paritytech/polkadot into kiz-benc…

2387d7f

…h-hrmp

Fix rococo

21b2cf5

kianenigma commented Sep 21, 2021

View reviewed changes

Final touches

40eb5ca

kianenigma commented Sep 21, 2021

View reviewed changes

pepyakin reviewed Sep 21, 2021

View reviewed changes

runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

revert fmt changes, one last time

a6a1af2

pepyakin reviewed Sep 21, 2021

View reviewed changes

runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

Fix wrappings

948c03d

kianenigma added 3 commits January 20, 2022 08:24

all runtimes build

0558cdf

undo formatting

a91ce3f

Master.into()

4a1e33d

drahnr approved these changes Jan 20, 2022

View reviewed changes

emostov reviewed Jan 20, 2022

View reviewed changes

runtime/parachains/src/hrmp.rs Outdated Show resolved Hide resolved

Update runtime/parachains/src/hrmp.rs

ca91f06

Co-authored-by: Zeke Mostov <[email protected]>

pepyakin reviewed Jan 20, 2022

View reviewed changes

emostov approved these changes Jan 20, 2022

View reviewed changes

kianenigma added 5 commits January 21, 2022 07:46

non-controversial changes

2cb4e67

do it the parachain-way: use const instead of type configs for simpli…

e22a79f

…city.

Merge branch 'kiz-bench-hrmp' of github.com:paritytech/polkadot into …

2ec5575

…kiz-bench-hrmp

borrow assert_storage_consistency_exhaustive

a47ea77

move assert_storage_consistency_exhaustive to Pallet, so it can be re…

745e146

…used for benchmarks as well.

pepyakin reviewed Jan 21, 2022

View reviewed changes

pepyakin approved these changes Jan 21, 2022

View reviewed changes

fix typo

523ff33

paritytech-processbot bot merged commit 95581b8 into master Jan 21, 2022

paritytech-processbot bot deleted the kiz-bench-hrmp branch January 21, 2022 13:42

pepyakin added B7-runtimenoteworthy and removed B0-silent Changes should not be mentioned in any release notes labels Jan 24, 2022

kianenigma added D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Feb 14, 2022

louismerlin added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Jun 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HRMP benchmarks #3876

HRMP benchmarks #3876

kianenigma commented Sep 17, 2021 •

edited

Loading

kianenigma Sep 21, 2021

kianenigma Sep 21, 2021

kianenigma Sep 21, 2021

pepyakin left a comment

kianenigma commented Sep 21, 2021 •

edited

Loading

pepyakin left a comment

kianenigma commented Sep 21, 2021

drahnr left a comment

pepyakin left a comment

pepyakin Jan 20, 2022

kianenigma Jan 21, 2022

pepyakin Jan 20, 2022

kianenigma Jan 21, 2022

hirschenberger Jan 21, 2022

pepyakin Jan 21, 2022

pepyakin Jan 20, 2022

kianenigma Jan 21, 2022

pepyakin Jan 21, 2022

kianenigma Jan 21, 2022

pepyakin Jan 20, 2022

pepyakin Jan 20, 2022

kianenigma Jan 21, 2022

drahnr Jan 21, 2022 •

edited

Loading

kianenigma Jan 21, 2022

pepyakin Jan 21, 2022

emostov left a comment

pepyakin Jan 21, 2022

kianenigma commented Jan 21, 2022

paritytech-processbot bot commented Jan 21, 2022

HRMP benchmarks #3876

HRMP benchmarks #3876

Conversation

kianenigma commented Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepyakin left a comment

Choose a reason for hiding this comment

kianenigma commented Sep 21, 2021 • edited Loading

pepyakin left a comment

Choose a reason for hiding this comment

kianenigma commented Sep 21, 2021

drahnr left a comment

Choose a reason for hiding this comment

pepyakin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drahnr Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emostov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma commented Jan 21, 2022

paritytech-processbot bot commented Jan 21, 2022

kianenigma commented Sep 17, 2021 •

edited

Loading

kianenigma commented Sep 21, 2021 •

edited

Loading

drahnr Jan 21, 2022 •

edited

Loading