Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Comments

Runtime dependent weights#5064

Merged
paritytech-processbot[bot] merged 20 commits intomasterfrom
oty-move-weights
Mar 11, 2022
Merged

Runtime dependent weights#5064
paritytech-processbot[bot] merged 20 commits intomasterfrom
oty-move-weights

Conversation

@ggwpez
Copy link
Member

@ggwpez ggwpez commented Mar 9, 2022 •
edited
Loading

This MR makes the BlockExecution, ExtrinsicBase, RocksDb and ParityDb Weights runtime dependent.
Later on paritytech/substrate#10977 will be used to generate weights for each runtime instead of using the Substrate defaults.

Changes:

  • Copy the Substrate weights from frame_support into each *_runtime_constants module
  • Move the parameter weight types out of runtime_common into each runtime with a new impl_runtime_weights! macro
  • Make NORMAL_DISPATCH_RATIO public since it is mentioned in the doc of TargetBlockFullness. Can make it private again if that is better.
  • Fix some test in rococo

I checked that the runtime did not change: #5064 (comment), CI confirmation would still be appreciated.

cumulus companion: paritytech/cumulus#1076

ggwpez added 2 commits March 9, 2022 12:41
@ggwpez
Copy Substrate weights into each runtime
8e14d92 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Define weights per runtime
edf017e 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added I8-refactor Code needs refactoring. A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Mar 9, 2022
@ggwpez ggwpez requested a review from shawntabrizi March 9, 2022 11:45
@ggwpez ggwpez added the B0-silent Changes should not be mentioned in any release notes label Mar 9, 2022
ggwpez added 2 commits March 9, 2022 13:11
@ggwpez
Do the same for test-runtime
c40cdfb 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Fix CI
26c9893 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ggwpez
ggwpez commented Mar 9, 2022
View reviewed changes
runtime/common/src/lib.rs
/// We allow `Normal` extrinsics to fill up the block up to 75%, the rest can be used
/// by Operational extrinsics.
const NORMAL_DISPATCH_RATIO: Perbill = Perbill::from_percent(75);
pub const NORMAL_DISPATCH_RATIO: Perbill = Perbill::from_percent(75);
Copy link
Member Author

@ggwpez ggwpez Mar 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is the only intended logic change.

ggwpez added 7 commits March 9, 2022 14:01
@ggwpez
Generate multiplier tests via macro
8fe576c 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Fix tests
a5fe30f 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Fix CI
d147d8d 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Fix imports
81c7100 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Fix test
c9197e3 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Fix imports
49c35e4 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Change rococo tests to use ExtrinsicBaseWeight
8b2c032 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ggwpez
ggwpez commented Mar 9, 2022
View reviewed changes
runtime/rococo/constants/src/lib.rs
println!("Base: {}", BlockWeights::get().get(DispatchClass::Normal).base_extrinsic);
let x = WeightToFee::calc(&BlockWeights::get().max_block);
println!("Base: {}", ExtrinsicBaseWeight::get());
let x = WeightToFee::calc(&MAXIMUM_BLOCK_WEIGHT);
Copy link
Member Author

@ggwpez ggwpez Mar 9, 2022

Choose a reason for hiding this comment

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

Test here looks weird so I changed it to look like in the other runtimes; in polkadot, kusama and westend it is all done this way.
Could also be macro generated 🤷

ggwpez added 4 commits March 9, 2022 17:15
@ggwpez
Fix imports
fd59c61 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Spellcheck on
bcede28 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Spellcheck on
85aca13 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Spellcheck
90fdabd 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ggwpez and others added 2 commits March 9, 2022 20:19
@ggwpez
Re-Export weight constants in the runtime
0ed2949 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@shawntabrizi
Merge branch 'master' into oty-move-weights
0ea4366
@shawntabrizi
Copy link
Member

shawntabrizi commented Mar 9, 2022

would be good to sanity check the metadata constants have not changed with regard to these weight parameters.

@ggwpez
Copy link
Member Author

ggwpez commented Mar 9, 2022

Waiting for this to be resolved, since cumulus should 🤞 already build with this version: #5037 (comment)

would be good to sanity check the metadata constants have not changed with regard to these weight parameters.

I will extract the metadata manually and check but CI would be much preferred.

@ggwpez
Prepare for cumulus companion
749c669 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez mentioned this pull request Mar 9, 2022
Merged
ggwpez added 2 commits March 9, 2022 23:58
@ggwpez
Fmt
a564113 
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez
Merge remote-tracking branch 'origin/master' into oty-move-weights
e627557
@ggwpez
Copy link
Member Author

ggwpez commented Mar 10, 2022

I wrote a small script to compare the metadata between branches and there is no change for kusama-dev, westend-dev,
polkadot-dev, rococo-dev. compare.sh.txt maybe the CI devs can reuse that somehow.

kianenigma
kianenigma approved these changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Didn't look at the whole diff, but sounds right to me, and much cleaner now 👍

@mustermeiszer mustermeiszer mentioned this pull request Mar 11, 2022
Open
2 tasks
@ggwpez
Copy link
Member Author

ggwpez commented Mar 11, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e6fa871 into master Mar 11, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-move-weights branch March 11, 2022 16:36
@ggwpez ggwpez mentioned this pull request Mar 22, 2022
Closed
@github-actions github-actions bot mentioned this pull request Apr 20, 2022
Closed
@github-actions github-actions bot mentioned this pull request Jul 18, 2022
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@kianenigma kianenigma kianenigma approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. I8-refactor Code needs refactoring.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ggwpez @shawntabrizi @kianenigma