Weight v1.5: Opaque Struct #12138
Weight v1.5: Opaque Struct #12138
Conversation
shawntabrizi
requested review from
kianenigma,
athei and
andresilva
as code owners
|
|
||
| use super::*; | ||
|
|
||
| pub type RefTimeWeight = u64; |
There was a problem hiding this comment.
Is the plan to eventually make this type private? Some docs would be nice here.
There was a problem hiding this comment.
no, i think it will stick around. i added docs
There was a problem hiding this comment.
I am kind of allergic to primitive type aliases. I think we should either use the primitive type directly or crate a newtype struct RefTimeWeight(u64) with a Deref impl on it. Primitive type aliases are just the worst of both worlds: Adding cognitive load by making you look up another type but offer nothing in refactoring advantages as a newtype would do.
There was a problem hiding this comment.
okay, i am happy enough to remove it in the next round
There was a problem hiding this comment.
Maybe a better way is to use a wrapper struct like @athei mentioned, but implement all the basic math traits on it like Add, Sub, Mul and Div instead of Deref, so that RefTimeWeight can act almost like a number.
There was a problem hiding this comment.
i dont think we need to do that, u64 should be fine for a single field here.
The issue is only when the whole struct is represented as u64.
There was a problem hiding this comment.
If we are relative sure that this type will never change, similar to Weight. I think keeping a type alias is fine. Type aliases are used are used to reduce cognitive load, not to increase it, imo. The point being that you directly know by reading what the types means and not need to find out where it comes from and what the function says about its usage.
There was a problem hiding this comment.
Type aliases are used are used to reduce cognitive load, not to increase it, imo
The road to hell is paved with good intentions. I never felt they helped me. They just hide the truth from me and offer zero type safety. But I am not gonna die on that hill.
|
Error: "Check reviews" status is not passing for paritytech/polkadot#5943 |
| // AccountData for inner call origin accountdata. | ||
| .saturating_add(T::DbWeight::get().reads_writes(1, 1)), | ||
| .saturating_add(T::DbWeight::get().reads_writes(1, 1)) | ||
| .saturating_add(dispatch_info.weight), |
There was a problem hiding this comment.
Does the ordering here really matter?
There was a problem hiding this comment.
no, this is left over from some other changes where it did matter, and then changes undone
There was a problem hiding this comment.
should be superficial, so not gonna go back to fix
|
bot merge |
|
After the last iterations and changes related to this PR (see #12157), the regex provided on the description will look like: Find: Find: Find: Find: |
louismerlin
added
D1-audited 👍
* initial idea * update frame_support * update a bunch more * add ord * adjust RuntimeDbWeight * frame_system builds * re-export * frame_support tests pass * frame_executive compile * frame_executive builds * frame_system tests passing * pallet-utility tests pass * fix a bunch of pallets * more * phragmen * state-trie-migration * scheduler and referenda * pallet-election-provider-multi-phase * aura * staking * more * babe * balances * bunch more * sudo * transaction-payment * asset-tx-payment * last pallets * fix alliance merge * fix node template runtime * fix pallet-contracts cc @athei * fix node runtime * fix compile on runtime-benchmarks feature * comment * fix frame-support-test * fix more tests * weight regex * frame system works * fix a bunch * more * more * more * more * more * more fixes * update templates * fix contracts benchmarks * Update lib.rs * Update lib.rs * fix ui * make scalar saturating mul const * more const functions * scalar div * refactor using constant functions * move impl * fix overhead template * use compactas * Update lib.rs
This PR is an intermediate step between our current u64 Weight V1, and the upcoming multi-dimensional Weight v2.
This PR changes
type Weight = u64intostruct Weight { ref_time: u64 }, and that is all.This PR has a huge number of changes, but NONE of them should be logically different than before.
This will allow us to separate logical changes in the Weight V2 PR, from random syntax fixes.
polkadot companion: paritytech/polkadot#5943
cumulus companion: paritytech/cumulus#1581
Developer Notes
Look at the weight_v2.rs file to see the exposed APIs of
struct Weight:https://github.com/paritytech/substrate/blob/6b3a7d2115e7b6e31139e6860862d7d72bb0ed9e/frame/support/src/weights/weight_v2.rs
Here is some Regex find/replace which can help you migrate all your weight files:
Updated from comment of @al3mart #12138 (comment)
Find:
\(([0-9_]+) as Weight\)\nReplace:
Weight::from_ref_time($1 as u64)\nFind:
_add\(\(([0-9_]+) as Weight\).saturating_mulReplace:
_add(Weight::from_ref_time($1 as u64).saturating_mulFind:
\(([0-9_]+) as Weight\)Replace:
($1 as u64)Find:
\(([a-z]) as Weight\)Replace:
($1 as u64)Find:
use frame_support::{traits::Get, weights::{Weight, constants::RocksDbWeight}};Replace:
use frame_support::{traits::Get, weights::{RefTimeWeight, Weight, constants::RocksDbWeight}};