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.

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

Jump to bottom

pallet scheduler: fix weight and add safety checks #7785

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

pallet scheduler: fix weight and add safety checks #7785

wants to merge 10 commits into from
+94 −4

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Mar 3, 2025 •
edited
Loading

Changes:

  • Add runtime integrity test for scheduler pallet to ensure that lookups use sensible weights
  • Check all passed storage names in the omni bencher to be known by FRAME metadata
  • Trim storage names in omni bencher to fix V1 bench syntax bug
  • Fix V1 bench syntax storage name sanitization for specific Rust versions

I re-ran the benchmarks with the omni-bencher modifications and it did not change the proof size. I reverted the commit afterwards to reduce the noise for reviewers.

@ggwpez ggwpez requested a review from a team as a code owner March 3, 2025 19:11
@ggwpez ggwpez marked this pull request as draft March 3, 2025 19:12
@ggwpez ggwpez mentioned this pull request Mar 3, 2025
Merged
1 task
@paritytech-workflow-stopper
Copy link

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/13638547036
Failed job name: run-frame-omni-bencher

@ggwpez
Copy link
Member Author

ggwpez commented Mar 4, 2025

/cmd bench --runtime westend asset-hub-westend

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 4, 2025

Command "bench --runtime westend asset-hub-westend" has started 🚀 See logs here

fellowship-merge-bot bot pushed a commit to polkadot-fellows/runtimes that referenced this pull request Mar 4, 2025
@ggwpez @actions-user
Fix scheduler weight and add test (#614)
40f124f 
- **Add test to collectives pallet**
- **Manually edit scheduler weight**

## Investigation

The Rust compiler changed how the `stringify!` macro formats paths on
[Aug 12th
'24](rust-lang/rust#128992 (comment)).
I assume that this broke the V1 benchmarking
[here](https://github.com/paritytech/polkadot-sdk/blob/7ecf3f757a5d6f622309cea7f788e8a547a5dce8/substrate/frame/benchmarking/src/v1.rs#L1011).
We had updated the scheduler pallet to V2 syntax (which is [not
affected](https://github.com/paritytech/polkadot-sdk/blob/df99fb9431a579589c1c87832222c9551b5c4f7c/substrate/frame/support/procedural/src/benchmark.rs#L565))
on [Nov 20th '24](paritytech/polkadot-sdk#6292).
We did not back-port this since there was no apparent reason for it.
The is why it seemingly fixed it self on SDK master but not in the
runtimes repo (since that is using V1 scheduler benchmarking).
Another indication for this is the fact that it did work on the
[whitelist
pallet](https://github.com/polkadot-fellows/runtimes/blob/6b85bf6adb427942976648e6d235e0169dfced16/relay/polkadot/src/weights/pallet_whitelist.rs#L85),
which is on V2 much longer but used the same [custom pov
mode](https://github.com/paritytech/polkadot-sdk/blob/b76e91acc953e682b3ddcfc45ecacaaf26c694a1/substrate/frame/whitelist/src/benchmarking.rs#L68).

Adding some sanity checks to prevent this in the future here
paritytech/polkadot-sdk#7785

- [ ] Does not require a CHANGELOG entry

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
Co-authored-by: GitHub Action <[email protected]>
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 5, 2025

Command "bench --runtime westend asset-hub-westend" has finished ✅ See logs here

Subweight results:
File Extrinsic Old New Change [%]
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/cumulus_pallet_weight_reclaim.rs storage_weight_reclaim 3.98us 9.21us +131.51
polkadot/runtime/westend/src/weights/pallet_utility.rs if_else 9.34us 12.05us +29.02
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/frame_system.rs remark 43.58ms 55.31ms +26.93
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/frame_system.rs remark_with_event 49.71ms 60.65ms +22.01
polkadot/runtime/westend/src/weights/pallet_beefy_mmr.rs n_leafs_proof_is_optimal 1.87us 2.09us +11.71
polkadot/runtime/westend/src/weights/frame_system_extensions.rs check_non_zero_sender 594.00ns 643.00ns +8.25
polkadot/runtime/westend/src/weights/polkadot_runtime_parachains_initializer.rs force_approve 53.82us 57.93us +7.64
polkadot/runtime/westend/src/weights/frame_system_extensions.rs check_spec_version 446.00ns 478.00ns +7.17
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_timestamp.rs on_finalize 4.66us 4.94us +5.94
polkadot/runtime/westend/src/weights/frame_system_extensions.rs check_tx_version 468.00ns 495.00ns +5.77
polkadot/runtime/westend/src/weights/pallet_staking.rs force_no_eras 108.37us 114.52us +5.67
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/xcm/pallet_xcm_benchmarks_fungible.rs receive_teleported_asset 3.09us 3.26us +5.51
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs expect_transact_status 884.00ns 931.00ns +5.32
polkadot/runtime/westend/src/weights/pallet_staking.rs force_new_era_always 108.33us 113.85us +5.10
polkadot/runtime/westend/src/weights/pallet_staking.rs force_new_era 108.31us 113.80us +5.07
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs refund_surplus 1.48us 1.40us -5.08
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs asset_claimer 827.00ns 785.00ns -5.08
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs clear_error 774.00ns 731.00ns -5.56
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs expect_pallet 5.66us 5.33us -5.99
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_migrations.rs on_init_loop 229.00ns 213.00ns -6.99
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs clear_transact_status 853.00ns 793.00ns -7.03
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs set_error_handler 827.00ns 768.00ns -7.13
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs burn_asset 1.25us 1.15us -8.21
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs unpaid_execution 812.00ns 744.00ns -8.37
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs expect_error 783.00ns 716.00ns -8.56
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs clear_topic 759.00ns 693.00ns -8.70
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs execute_with_origin 907.00ns 826.00ns -8.93
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs descend_origin 834.00ns 756.00ns -9.35
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs expect_origin 815.00ns 738.00ns -9.45
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs expect_transact_status 973.00ns 875.00ns -10.07
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs clear_origin 808.00ns 726.00ns -10.15
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs set_appendix 817.00ns 733.00ns -10.28
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs set_topic 782.00ns 701.00ns -10.36
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs set_fees_mode 793.00ns 706.00ns -10.97
polkadot/runtime/westend/src/weights/pallet_staking.rs do_elect_paged_inner 238.63ns 209.26ns -12.31
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs expect_asset 911.00ns 795.00ns -12.73
cumulus/parachains/runtimes/assets/asset-hub-westend/src/weights/pallet_multisig.rs as_multi_threshold_1 24.27us 21.13us -12.96
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs trap 825.00ns 718.00ns -12.97
polkadot/runtime/westend/src/weights/xcm/pallet_xcm_benchmarks_generic.rs buy_execution 843.00ns 723.00ns -14.23
polkadot/runtime/westend/src/weights/pallet_multisig.rs as_multi_threshold_1 26.53us 21.14us -20.32
Command output:

✅ Successful benchmarks of runtimes/pallets:
-- westend: ['pallet_xcm_benchmarks::fungible', 'polkadot_runtime_parachains::coretime', 'pallet_message_queue', 'polkadot_runtime_common::auctions', 'pallet_utility', 'frame_system', 'pallet_staking', 'pallet_identity', 'pallet_transaction_payment', 'polkadot_runtime_common::assigned_slots', 'pallet_treasury', 'pallet_offences', 'pallet_whitelist', 'pallet_sudo', 'polkadot_runtime_parachains::paras_inherent', 'pallet_bags_list', 'pallet_scheduler', 'polkadot_runtime_parachains::disputes', 'polkadot_runtime_parachains::disputes::slashing', 'polkadot_runtime_parachains::inclusion', 'pallet_mmr', 'polkadot_runtime_parachains::hrmp', 'frame_election_provider_support', 'polkadot_runtime_common::paras_registrar', 'pallet_beefy_mmr', 'pallet_parameters', 'polkadot_runtime_common::crowdloan', 'pallet_xcm', 'pallet_recovery', 'pallet_preimage', 'pallet_asset_rate', 'pallet_conviction_voting', 'pallet_nomination_pools', 'pallet_multisig', 'polkadot_runtime_common::slots', 'polkadot_runtime_parachains::paras', 'pallet_referenda', 'frame_system_extensions', 'pallet_migrations', 'polkadot_runtime_parachains::initializer', 'polkadot_runtime_parachains::configuration', 'polkadot_runtime_common::identity_migrator', 'pallet_fast_unstake', 'pallet_timestamp', 'polkadot_runtime_parachains::on_demand', 'pallet_balances', 'pallet_session', 'pallet_election_provider_multi_phase', 'pallet_vesting', 'pallet_indices', 'pallet_xcm_benchmarks::generic', 'pallet_proxy']
-- asset-hub-westend: ['pallet_assets', 'pallet_xcm_benchmarks::fungible', 'pallet_collator_selection', 'cumulus_pallet_parachain_system', 'pallet_uniques', 'pallet_message_queue', 'pallet_utility', 'frame_system', 'pallet_asset_rewards', 'pallet_nft_fractionalization', 'cumulus_pallet_weight_reclaim', 'pallet_asset_conversion_tx_payment', 'pallet_transaction_payment', 'pallet_xcm_bridge_hub_router', 'pallet_asset_conversion_ops', 'pallet_xcm', 'pallet_multisig', 'frame_system_extensions', 'pallet_migrations', 'pallet_asset_conversion', 'pallet_nfts', 'pallet_timestamp', 'pallet_balances', 'pallet_session', 'cumulus_pallet_xcmp_queue', 'pallet_xcm_benchmarks::generic', 'pallet_proxy']

@ggwpez
Copy link
Member Author

ggwpez commented Mar 5, 2025

/cmd prdoc --bump minor --audience runtime_dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@ggwpez