FRAME: Register on_initialize after each pallet#9756
Conversation
|
A few comments/questions:
Part of our worries in #9619 also related to this. On AH, a single block might might:
And all of this is |
from the discussion we had on Element, my take-aways are the following:
|
The 500ms blocks, where I'm afraid someone will do something bad :) And overall I think it is just better to have there a correct weight tracking. For sure it is not perfect, but still better than the status quo.
Yes
Inherents, scheduler on-init, message-queue etc all should honor the weight much better and should be more tightly controlled when it comes to this. |
gui1117
left a comment
There was a problem hiding this comment.
I think the trait doesn't need the generic BlockNumber otherwise looks good to me
substrate/frame/executive/src/lib.rs
Outdated
| frame_support::impl_for_tuples_attr! { | ||
| #[tuple_types_custom_trait_bound(OnInitialize<BlockNumber>)] | ||
| impl<BlockNumber: Clone, T: frame_system::Config> OnInitializeWithWeightRegistration<T, BlockNumber> for Tuple { | ||
| fn on_initialize_with_weight_registration(n: BlockNumber) -> Weight { |
There was a problem hiding this comment.
Fortunately we define all pallets as a single tuple like: type AllPallet = (System, Balances, Assets); and not type AllPallet = (System, (Balances, Assets));. Otherwise this will not help.
But we have to be careful not to change this, maybe we can have a test specifically written for this.
(We could also generate OnInitializeWithWeightRegistration with the macro pallet, and make this trait implemented for tuple of OnInitializeWithWeightRegistration.)
There was a problem hiding this comment.
Fortunately we define all pallets as a single tuple like:
type AllPallet = (System, Balances, Assets);and nottype AllPallet = (System, (Balances, Assets));. Otherwise this will not help.
Yeah I know. I also checked this.
But we have to be careful not to change this, maybe we can have a test specifically written for this.
I first thought about writing a integrity_test, but this is right now not supported by Executive.
There was a problem hiding this comment.
In the worst case we have some pallets that don't register the weight immediately.
There was a problem hiding this comment.
I first thought about writing a integrity_test, but this is right now not supported by Executive.
yes, I was a thinking a test that ensure construct_runtime defines AllPallets as we expect, with 2 defined pallet that ensure the weight is updated inside each on_initialize. The test you wrote in scheduler already tests for it also. So in all case it is good to me
|
Would you be willing to backport this to 2507 and 2509? Not sure if it has added risk for us. If yes, we can use this in |
|
@kianenigma if you approve, I can backport it. :P |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
|
/cmd prdoc --audience runtime_dev --bump patch |
…time_dev --bump patch'
|
@bkchr , @kianenigma, @ggwpez : how confident are we in backporting to 2509 for the early October deployment on Westend? Additionally, how do we feel about using 2507 in the post-KAHM and pre- or post-PAHM phases with EPMB using it? |
|
This seems pretty innocent to me, I think we can safely backport. But lemme see if it is will be useful for EPMB, will let you know. If I can migrate to poll already, it will be easier and done once and for all. |
|
(this PR is basically making on-init able to 👺 as poll) |
|
I'm also fine with backporting. It doesn't change that much. |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-9756-to-unstable2507
git worktree add --checkout .worktree/backport-9756-to-unstable2507 backport-9756-to-unstable2507
cd .worktree/backport-9756-to-unstable2507
git reset --hard HEAD^
git cherry-pick -x 19320f104fc4b5cb6663cffc41f340b6b5239be8
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-9756-to-stable2509
git worktree add --checkout .worktree/backport-9756-to-stable2509 backport-9756-to-stable2509
cd .worktree/backport-9756-to-stable2509
git reset --hard HEAD^
git cherry-pick -x 19320f104fc4b5cb6663cffc41f340b6b5239be8
git push --force-with-lease |
|
@sigurpol the backports are prepared. |
Backport #9756 into `stable2509` from bkchr. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Co-authored-by: Bastian Köcher <git@kchr.de> Co-authored-by: Bastian Köcher <info@kchr.de>
Before this pull request, FRAME was executing all pallets `on_initialize` and then register the weight, including the weight of `on_runtime_upgrade`. Thus, other pallets were not aware on how much weight was already used when they were executing their `on_initialize` code. As some pallets are doing some work in `on_initialize`, they need to be aware of how much weight is still left. To register the weight after each `on_initialize` call, a new trait is added. This new trait is implemented for tuples of types that implement `OnInitialize` and then it registers the weight after each call to `on_initialize`. `pallet-scheduler` is changed to take the remaining weight into account and to not just assume that its configured weight is always available. --------- Co-authored-by: cmd[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Before this pull request, FRAME was executing all pallets
on_initializeand then register the weight, including the weight ofon_runtime_upgrade. Thus, other pallets were not aware on how much weight was already used when they were executing theiron_initializecode. As some pallets are doing some work inon_initialize, they need to be aware of how much weight is still left.To register the weight after each
on_initializecall, a new trait is added. This new trait is implemented for tuples of types that implementOnInitializeand then it registers the weight after each call toon_initialize.pallet-scheduleris changed to take the remaining weight into account and to not just assume that its configured weight is always available.