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.

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

Weights per runtime #3019

Merged
merged 45 commits into from
Nov 21, 2024
Merged

Weights per runtime #3019

merged 45 commits into from
Nov 21, 2024

Conversation

gonzamontiel
Copy link
Contributor

@gonzamontiel gonzamontiel commented Oct 21, 2024

What does it do?

This PR replaces #2939.

It modifies pallet's benchmarks and removes magic numbers to be runtime-dependant.

What important points reviewers should know?

Pallet parachain staking had some fixed numbers that I replaced by values we get from the runtime. This might go against the testing purpose, because now a change on those parameters might go unnoticed in the test. Anyway I think it is an acceptable trade off.

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

What value does it bring to the blockchain users?

TarekkMA and others added 3 commits October 17, 2024 15:55
…r-runtime

# Conflicts:
#	runtime/common/src/lib.rs
#	runtime/moonbase/src/weights/pallet_xcm_weight_trader.rs
#	runtime/moonbeam/src/lib.rs
#	runtime/moonbeam/src/precompiles.rs
#	runtime/moonriver/src/precompiles.rs
@gonzamontiel gonzamontiel added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Oct 21, 2024
@gonzamontiel gonzamontiel marked this pull request as draft October 21, 2024 12:34
Copy link
Contributor

github-actions bot commented Oct 29, 2024

WASM runtime size check:

Compared to target branch

Moonbase runtime: 2260 KB (no changes) ✅

Moonbeam runtime: 2224 KB (no changes) ✅

Moonriver runtime: 2224 KB (no changes) ✅

Compared to latest release (runtime-3300)

Moonbase runtime: 2260 KB (+232 KB compared to latest release) ⚠️

Moonbeam runtime: 2224 KB (+228 KB compared to latest release) ⚠️

Moonriver runtime: 2224 KB (+232 KB compared to latest release) ⚠️

@gonzamontiel gonzamontiel marked this pull request as ready for review October 30, 2024 14:28
Copy link
Contributor

github-actions bot commented Nov 1, 2024

Coverage Report

@@                      Coverage Diff                      @@
##           master   gonza/weights-per-runtime      +/-   ##
=============================================================
- Coverage   79.34%                      74.81%   -4.53%     
+ Files         301                         369      +68     
+ Lines       87834                       94127    +6293     
=============================================================
+ Hits        69685                       70421     +736     
+ Misses      18149                       23706    +5557     
Files Changed Coverage
/precompiles/relay-data-verifier/src/mock.rs 85.53% (-2.16%) 🔽
/runtime/moonbeam/tests/common/mod.rs 94.74% (+0.06%) 🔼
/runtime/moonriver/tests/common/mod.rs 94.85% (+0.05%) 🔼

Coverage generated Thu Nov 21 00:04:46 UTC 2024

Copy link
Contributor

@RomarQ RomarQ left a comment

Choose a reason for hiding this comment

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

Changes in specs should be reverted.

@gonzamontiel
Copy link
Contributor Author

Changes in specs should be reverted.

good catch, I don't know, maybe it was a bad merge.

@gonzamontiel gonzamontiel requested a review from RomarQ November 19, 2024 17:52
TarekkMA
TarekkMA previously approved these changes Nov 20, 2024
@crystalin
Copy link
Collaborator

We should flag weights that are being changed by more than 10% so we can discuss their impacts

runtime/moonbase/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/moonriver/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/moonbeam/src/xcm_config.rs Outdated Show resolved Hide resolved
runtime/moonbeam/src/lib.rs Outdated Show resolved Hide resolved
@gonzamontiel
Copy link
Contributor Author

gonzamontiel commented Nov 20, 2024

We should flag weights that are being changed by more than 10% so we can discuss their impacts

Here the list of the changes greater than 5% . The negatives are improvements.
pallet_moonbeam_foreign_assets has huge changes.

File Extrinsic Moonbeam % Moonriver %
pallet_treasury.rs payout -31.44 -31.44
pallet_xcm_transactor.rs transact_through_sovereign -27.23 -27.23
pallet_xcm_transactor.rs transact_through_derivative -21.72 -21.72
pallet_moonbeam_orbiters.rs on_new_round -20.18 0.20
pallet_referenda.rs kill -18.76 -18.76
pallet_parachain_staking.rs notify_inactive_collator -11.52 -0.82
pallet_whitelist.rs dispatch_whitelisted_call -6.55 -5.11
pallet_xcm_transactor.rs transact_through_signed -5.80 -5.80
pallet_xcm_transactor.rs hrmp_manage -5.68 -5.68
pallet_message_queue.rs execute_overweight_page_updated 5.05 2.66
pallet_scheduler.rs service_task_fetched 9.68 9.88
pallet_moonbeam_foreign_assets.rs create_foreign_asset 35.60 35.60
pallet_moonbeam_foreign_assets.rs freeze_foreign_asset 55.36 55.36
pallet_moonbeam_foreign_assets.rs unfreeze_foreign_asset 68.09 68.09

@gonzamontiel
Copy link
Contributor Author

We should flag weights that are being changed by more than 10% so we can discuss their impacts

Here the list of the changes greater than 5% . The negatives are improvements. pallet_moonbeam_foreign_assets has huge changes.
...

Btw, the comparison is against weights in moonbase (master). But the weights difference between moonbase (master) and moonbase (this branch) is negligible: in average -0.22 %, which is consistent becase we use the same machine.

@RomarQ
Copy link
Contributor

RomarQ commented Nov 21, 2024

We should flag weights that are being changed by more than 10% so we can discuss their impacts

Here the list of the changes greater than 5% . The negatives are improvements. pallet_moonbeam_foreign_assets has huge changes.
...

Btw, the comparison is against weights in moonbase (master). But the weights difference between moonbase (master) and moonbase (this branch) is negligible: in average -0.22 %, which is consistent becase we use the same machine.

The weight difference is now reasonable. Do you know why it was different before?

@RomarQ RomarQ merged commit 5b3642d into master Nov 21, 2024
39 checks passed
@RomarQ RomarQ deleted the gonza/weights-per-runtime branch November 21, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants