Weight v1.5 Follow Ups #12155
Weight v1.5 Follow Ups #12155
Conversation
shawntabrizi
requested review from
kianenigma,
athei and
andresilva
as code owners
shawntabrizi
added
B0-silent
|
More follow up here: #12157 |
| } | ||
|
|
||
| /// Checked [`Weight`] addition. Computes `self + rhs`, returning `None` if overflow occurred. | ||
| pub const fn checked_add(&self, rhs: &Self) -> Option<Self> { |
There was a problem hiding this comment.
Why did you use standalone functions instead of implementing Checked and Saturating traits?
There was a problem hiding this comment.
i do implement the checked trait below (where it was possible), but these are const functions.
Saturating implies too many things, and it did not make sense for me to implement all of them
| } | ||
|
|
||
| /// Return a [`Weight`] where all fields are zero. | ||
| pub const fn zero() -> Self { |
There was a problem hiding this comment.
this is implemented, but again, this is a const function, where the trait is not.
also to avoid needing to import zero every time.
There was a problem hiding this comment.
Ahh I didn't notice them being const.
To be fair though, I don't agree with the imports difficulty. You can take the same argument and never use Zero. The benefit is that you can get all the blanket implementations where T: Zero for free in return.
There was a problem hiding this comment.
Well, if I also implement the Zero trait if you look just a little lower on the file... so you get the best of both worlds?
In fact, I am a pretty big fan of having the const impls exposed via the struct, it just makes these functions easier to access, not to mention const.
There was a problem hiding this comment.
I believe this wouldn't be an issue if we had const Trait in Rust, which is awaiting stabilization.
because the traits are not const, and things like saturating, imply that we must implement things like Finally, it can help reduce the number of trait imports you need when using the Weight type and functions. So in this case, every trait which I could implement, i did, but only as a re-export of the internal functions which are all const. |
| @@ -230,8 +230,7 @@ where | |||
| let weight_per_key = (T::WeightInfo::on_initialize_per_trie_key(1) - | |||
| T::WeightInfo::on_initialize_per_trie_key(0)) | |||
| .ref_time(); | |||
| let decoding_weight = | |||
| weight_per_queue_item.scalar_saturating_mul(queue_len as RefTimeWeight); | |||
| let decoding_weight = weight_per_queue_item.saturating_mul(queue_len as RefTimeWeight); | |||
There was a problem hiding this comment.
I don't think queue_len is a weight:
| let decoding_weight = weight_per_queue_item.saturating_mul(queue_len as RefTimeWeight); | |
| let decoding_weight = weight_per_queue_item.saturating_mul(queue_len as u64); |
There was a problem hiding this comment.
should be fixed here: #12157
|
bot merge |
|
Waiting for commit status. |
paritytech-processbot
bot
deleted the
shawntabrizi-weight-v1.5-patches
branch
* update api * update * remove unused * remove `one` api * fix unused * fmt * add saturating accrue * remove `Weight::new()` * use some macros * div makes no sense * Update weight_v2.rs * missed some * more patch * fixes * more fixes * more fix * more fix * Update frame/support/src/weights/weight_v2.rs * not needed * fix weight file
This PR does some follow ups to Weight v1.5: #12138
Weight.Weight * WeightorWeight / Weightwhich is nonsensical in the context of multi-dimensional weights.Weight::one()which doesn't really make sense with multi-dimensional weights.Weight::new()and use justWeight::zero()instead.polkadot companion: paritytech/polkadot#5949
cumulus companion: paritytech/cumulus#1584