Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add storage size component to weights #12277

Merged
merged 41 commits into from
Sep 28, 2022
Merged
Changes from 3 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f8c395e
Add storage size component to weights
KiChjang Sep 15, 2022
779b815
Rename storage_size to proof_size
KiChjang Sep 15, 2022
f7b9346
Update primitives/weights/src/weight_v2.rs
KiChjang Sep 15, 2022
6c84cbd
Fixes
KiChjang Sep 16, 2022
96aa122
cargo fmt
KiChjang Sep 16, 2022
36c2d95
Implement custom Decode and CompactAs
KiChjang Sep 16, 2022
52c9d05
Add missing import
KiChjang Sep 16, 2022
9b1f37e
Fixes
KiChjang Sep 16, 2022
3438ada
Remove CompactAs implementation
KiChjang Sep 16, 2022
3b36396
Properly migrate from 1D weight
KiChjang Sep 16, 2022
7af788c
Remove #[pallet::compact] from Weight parameters
KiChjang Sep 16, 2022
f4b98aa
More #[pallet::compact] removals
KiChjang Sep 18, 2022
eefc1d7
Add unit tests
KiChjang Sep 18, 2022
109be74
Set appropriate default block proof size
KiChjang Sep 19, 2022
bbf072d
cargo fmt
KiChjang Sep 19, 2022
1a07286
Remove nonsensical weight constant
KiChjang Sep 19, 2022
10aed4c
Test only for the reference time weight in frame_system::limits
KiChjang Sep 19, 2022
be3d2f6
Only check for reference time weight on idle
KiChjang Sep 20, 2022
20ef5a3
Merge branch 'master' into kckyeung/storage-size-weights
KiChjang Sep 20, 2022
7908713
Use destructuring syntax
KiChjang Sep 22, 2022
e313544
Update test expectations
KiChjang Sep 22, 2022
9543674
Merge remote-tracking branch 'origin/master' into kckyeung/storage-si…
KiChjang Sep 22, 2022
91a03b2
Fixes
KiChjang Sep 22, 2022
e93dd45
Fixes
KiChjang Sep 22, 2022
9fe54f9
Fixes
KiChjang Sep 22, 2022
acd6d4b
Correctly migrate from 1D weights
KiChjang Sep 22, 2022
386d485
cargo fmt
KiChjang Sep 22, 2022
e85c05e
Migrate using extra extrinsics instead of custom Decode
KiChjang Sep 26, 2022
470e1bf
Merge remote-tracking branch 'origin/master' into kckyeung/storage-si…
KiChjang Sep 26, 2022
f541cb2
Fixes
KiChjang Sep 26, 2022
b34b49d
Silence dispatch call warnings that were previously allowed
KiChjang Sep 26, 2022
000ccad
Fix gas_left test
athei Sep 26, 2022
3c4ce94
Use OldWeight instead of u64
KiChjang Sep 26, 2022
d3b8a60
Fixes
KiChjang Sep 28, 2022
d3e9514
Only check for reference time weight in election provider
KiChjang Sep 28, 2022
4a9f631
Fix test expectations
KiChjang Sep 28, 2022
12b60f8
Fix test expectations
KiChjang Sep 28, 2022
b1a9125
Use only reference time weight in grandpa test
KiChjang Sep 28, 2022
3dec18b
Use only reference time weight in examples test
KiChjang Sep 28, 2022
8559428
Use only reference time weight in examples test
KiChjang Sep 28, 2022
4589902
Fix test expectations
KiChjang Sep 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 140 additions & 49 deletions primitives/weights/src/weight_v2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use codec::{CompactAs, Decode, Encode, MaxEncodedLen};
use codec::{Decode, Encode, MaxEncodedLen};
use core::ops::{Add, AddAssign, Div, Mul, Sub, SubAssign};
use sp_arithmetic::traits::{Bounded, CheckedAdd, CheckedSub, Zero};
use sp_debug_derive::RuntimeDebug;
Expand All @@ -33,12 +33,15 @@ use super::*;
Clone,
RuntimeDebug,
Default,
CompactAs,
)]
#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
pub struct Weight {
#[codec(compact)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need a migration?

Copy link
Member

Choose a reason for hiding this comment

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

yes. but the whole PR will need migration, since weight is now 2-dimensional, so good to have both of these changes in at once.

/// The weight of computational time used based on some reference hardware.
ref_time: u64,
#[codec(compact)]
/// The weight of storage space used by proof of validity.
proof_size: u64,
}

impl Weight {
Expand All @@ -48,71 +51,118 @@ impl Weight {
self
}

/// Set the storage size part of the weight.
pub const fn set_proof_size(mut self, c: u64) -> Self {
self.proof_size = c;
self
}

/// Return the reference time part of the weight.
pub const fn ref_time(&self) -> u64 {
self.ref_time
}

/// Return a mutable reference time part of the weight.
/// Return the storage size part of the weight.
pub const fn proof_size(&self) -> u64 {
self.proof_size
}

/// Return a mutable reference to the reference time part of the weight.
pub fn ref_time_mut(&mut self) -> &mut u64 {
&mut self.ref_time
}

pub const MAX: Self = Self { ref_time: u64::MAX };
/// Return a mutable reference to the storage size part of the weight.
pub fn proof_size_mut(&mut self) -> &mut u64 {
&mut self.proof_size
}

pub const MAX: Self = Self { ref_time: u64::MAX, proof_size: u64::MAX };

/// Get the conservative min of `self` and `other` weight.
pub fn min(&self, other: Self) -> Self {
Self { ref_time: self.ref_time.min(other.ref_time) }
Self {
ref_time: self.ref_time.min(other.ref_time),
proof_size: self.proof_size.min(other.proof_size),
}
}

/// Get the aggressive max of `self` and `other` weight.
pub fn max(&self, other: Self) -> Self {
Self { ref_time: self.ref_time.max(other.ref_time) }
Self {
ref_time: self.ref_time.max(other.ref_time),
proof_size: self.proof_size.max(other.proof_size),
}
}

/// Try to add some `other` weight while upholding the `limit`.
pub fn try_add(&self, other: &Self, limit: &Self) -> Option<Self> {
let total = self.checked_add(other)?;
if total.ref_time > limit.ref_time {
if total.any_gt(limit) {
None
} else {
Some(total)
}
}

/// Construct [`Weight`] with reference time weight.
/// Construct [`Weight`] with reference time weight and 0 storage size weight.
pub const fn from_ref_time(ref_time: u64) -> Self {
Self { ref_time }
Self { ref_time, proof_size: 0 }
}

/// Construct [`Weight`] with storage size weight and 0 reference time weight.
pub const fn from_proof_size(proof_size: u64) -> Self {
Self { ref_time: 0, proof_size }
}

/// Construct [`Weight`] with weight components, namely reference time and storage size weights.
pub const fn from_components(ref_time: u64, proof_size: u64) -> Self {
Self { ref_time, proof_size }
}
KiChjang marked this conversation as resolved.
Show resolved Hide resolved

/// Saturating [`Weight`] addition. Computes `self + rhs`, saturating at the numeric bounds of
/// all fields instead of overflowing.
pub const fn saturating_add(self, rhs: Self) -> Self {
Self { ref_time: self.ref_time.saturating_add(rhs.ref_time) }
Self {
ref_time: self.ref_time.saturating_add(rhs.ref_time),
proof_size: self.proof_size.saturating_add(rhs.proof_size),
}
Comment on lines +123 to +126
Copy link
Member

@ggwpez ggwpez Sep 15, 2022

Choose a reason for hiding this comment

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

Just an idea: Could have opted for a private field iter, such that this becomes something like:

Suggested change
Self {
ref_time: self.ref_time.saturating_add(rhs.ref_time),
proof_size: self.proof_size.saturating_add(rhs.proof_size),
}
self.fields_iter().map(|s| saturating_add(s, rhs)).collect::<Self>()

Then it is future proof when changing or adding fields. But could be overkill for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That also assumes that all fields are u64, which might not be the case in the future.

Copy link
Member

@ggwpez ggwpez Sep 15, 2022

Choose a reason for hiding this comment

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

No... it just assumes that they are Saturating. This iter would probably not be a homogeneous collection but a custom type.
I just saw that my example code does not work, since it would need to be zipped with the rhs.fields_iter() first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That, and the fact that all the fields must be homogeneous, because you're mapping over a collection of Items, which must have the same type.

}

/// Saturating [`Weight`] subtraction. Computes `self - rhs`, saturating at the numeric bounds
/// of all fields instead of overflowing.
pub const fn saturating_sub(self, rhs: Self) -> Self {
Self { ref_time: self.ref_time.saturating_sub(rhs.ref_time) }
Self {
ref_time: self.ref_time.saturating_sub(rhs.ref_time),
proof_size: self.proof_size.saturating_sub(rhs.proof_size),
}
}

/// Saturating [`Weight`] scalar multiplication. Computes `self.field * scalar` for all fields,
/// saturating at the numeric bounds of all fields instead of overflowing.
pub const fn saturating_mul(self, scalar: u64) -> Self {
Self { ref_time: self.ref_time.saturating_mul(scalar) }
Self {
ref_time: self.ref_time.saturating_mul(scalar),
proof_size: self.proof_size.saturating_mul(scalar),
}
}

/// Saturating [`Weight`] scalar division. Computes `self.field / scalar` for all fields,
/// saturating at the numeric bounds of all fields instead of overflowing.
pub const fn saturating_div(self, scalar: u64) -> Self {
Self { ref_time: self.ref_time.saturating_div(scalar) }
Self {
ref_time: self.ref_time.saturating_div(scalar),
proof_size: self.proof_size.saturating_div(scalar),
}
}

/// Saturating [`Weight`] scalar exponentiation. Computes `self.field.pow(exp)` for all fields,
/// saturating at the numeric bounds of all fields instead of overflowing.
pub const fn saturating_pow(self, exp: u32) -> Self {
Self { ref_time: self.ref_time.saturating_pow(exp) }
Self {
ref_time: self.ref_time.saturating_pow(exp),
proof_size: self.proof_size.saturating_pow(exp),
}
}

/// Increment [`Weight`] by `amount` via saturating addition.
Expand All @@ -122,96 +172,116 @@ impl Weight {

/// Checked [`Weight`] addition. Computes `self + rhs`, returning `None` if overflow occurred.
pub const fn checked_add(&self, rhs: &Self) -> Option<Self> {
match self.ref_time.checked_add(rhs.ref_time) {
Some(ref_time) => Some(Self { ref_time }),
None => None,
}
let ref_time = match self.ref_time.checked_add(rhs.ref_time) {
Some(t) => t,
None => return None,
};
let proof_size = match self.proof_size.checked_add(rhs.proof_size) {
Some(s) => s,
None => return None,
};
Some(Self { ref_time, proof_size })
}

/// Checked [`Weight`] subtraction. Computes `self - rhs`, returning `None` if overflow
/// occurred.
pub const fn checked_sub(&self, rhs: &Self) -> Option<Self> {
match self.ref_time.checked_sub(rhs.ref_time) {
Some(ref_time) => Some(Self { ref_time }),
None => None,
}
let ref_time = match self.ref_time.checked_sub(rhs.ref_time) {
Some(t) => t,
None => return None,
};
let proof_size = match self.proof_size.checked_sub(rhs.proof_size) {
Some(s) => s,
None => return None,
};
Some(Self { ref_time, proof_size })
}

/// Checked [`Weight`] scalar multiplication. Computes `self.field * scalar` for each field,
/// returning `None` if overflow occurred.
pub const fn checked_mul(self, scalar: u64) -> Option<Self> {
match self.ref_time.checked_mul(scalar) {
Some(ref_time) => Some(Self { ref_time }),
None => None,
}
let ref_time = match self.ref_time.checked_mul(scalar) {
Some(t) => t,
None => return None,
};
let proof_size = match self.proof_size.checked_mul(scalar) {
Some(s) => s,
None => return None,
};
Some(Self { ref_time, proof_size })
}

/// Checked [`Weight`] scalar division. Computes `self.field / scalar` for each field, returning
/// `None` if overflow occurred.
pub const fn checked_div(self, scalar: u64) -> Option<Self> {
match self.ref_time.checked_div(scalar) {
Some(ref_time) => Some(Self { ref_time }),
None => None,
}
let ref_time = match self.ref_time.checked_div(scalar) {
Some(t) => t,
None => return None,
};
let proof_size = match self.proof_size.checked_div(scalar) {
Some(s) => s,
None => return None,
};
Some(Self { ref_time, proof_size })
}

/// Return a [`Weight`] where all fields are zero.
pub const fn zero() -> Self {
Self { ref_time: 0 }
Self { ref_time: 0, proof_size: 0 }
}

/// Returns true if any of `self`'s constituent weights is strictly greater than that of the
/// `other`'s, otherwise returns false.
pub const fn any_gt(self, other: Self) -> bool {
self.ref_time > other.ref_time
self.ref_time > other.ref_time || self.proof_size > other.proof_size
}

/// Returns true if all of `self`'s constituent weights is strictly greater than that of the
/// `other`'s, otherwise returns false.
pub const fn all_gt(self, other: Self) -> bool {
self.ref_time > other.ref_time
self.ref_time > other.ref_time && self.proof_size > other.proof_size
}

/// Returns true if any of `self`'s constituent weights is strictly less than that of the
/// `other`'s, otherwise returns false.
pub const fn any_lt(self, other: Self) -> bool {
self.ref_time < other.ref_time
self.ref_time < other.ref_time || self.proof_size < other.proof_size
}

/// Returns true if all of `self`'s constituent weights is strictly less than that of the
/// `other`'s, otherwise returns false.
pub const fn all_lt(self, other: Self) -> bool {
self.ref_time < other.ref_time
self.ref_time < other.ref_time && self.proof_size < other.proof_size
}

/// Returns true if any of `self`'s constituent weights is greater than or equal to that of the
/// `other`'s, otherwise returns false.
pub const fn any_gte(self, other: Self) -> bool {
self.ref_time >= other.ref_time
self.ref_time >= other.ref_time || self.proof_size >= other.proof_size
}

/// Returns true if all of `self`'s constituent weights is greater than or equal to that of the
/// `other`'s, otherwise returns false.
pub const fn all_gte(self, other: Self) -> bool {
self.ref_time >= other.ref_time
self.ref_time >= other.ref_time && self.proof_size >= other.proof_size
}

/// Returns true if any of `self`'s constituent weights is less than or equal to that of the
/// `other`'s, otherwise returns false.
pub const fn any_lte(self, other: Self) -> bool {
self.ref_time <= other.ref_time
self.ref_time <= other.ref_time || self.proof_size <= other.proof_size
}

/// Returns true if all of `self`'s constituent weights is less than or equal to that of the
/// `other`'s, otherwise returns false.
pub const fn all_lte(self, other: Self) -> bool {
self.ref_time <= other.ref_time
self.ref_time <= other.ref_time && self.proof_size <= other.proof_size
}

/// Returns true if any of `self`'s constituent weights is equal to that of the `other`'s,
/// otherwise returns false.
pub const fn any_eq(self, other: Self) -> bool {
self.ref_time == other.ref_time
self.ref_time == other.ref_time || self.proof_size == other.proof_size
}

// NOTE: `all_eq` does not exist, as it's simply the `eq` method from the `PartialEq` trait.
Expand All @@ -230,14 +300,20 @@ impl Zero for Weight {
impl Add for Weight {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we could make this std only, since the runtime should not use unsafe math anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Not really unsafe math.

type Output = Self;
fn add(self, rhs: Self) -> Self {
Self { ref_time: self.ref_time + rhs.ref_time }
Self {
ref_time: self.ref_time + rhs.ref_time,
proof_size: self.proof_size + rhs.proof_size,
}
}
}

impl Sub for Weight {
type Output = Self;
fn sub(self, rhs: Self) -> Self {
Self { ref_time: self.ref_time - rhs.ref_time }
Self {
ref_time: self.ref_time - rhs.ref_time,
proof_size: self.proof_size - rhs.proof_size,
}
}
}

Expand All @@ -247,7 +323,10 @@ where
{
type Output = Self;
fn mul(self, b: T) -> Self {
Self { ref_time: b * self.ref_time }
Self {
ref_time: b * self.ref_time,
proof_size: b * self.proof_size,
}
}
}

Expand All @@ -257,7 +336,10 @@ macro_rules! weight_mul_per_impl {
impl Mul<Weight> for $t {
type Output = Weight;
fn mul(self, b: Weight) -> Weight {
Weight { ref_time: self * b.ref_time }
Weight {
ref_time: self * b.ref_time,
proof_size: self * b.proof_size,
}
}
}
)*
Expand All @@ -277,7 +359,10 @@ macro_rules! weight_mul_primitive_impl {
impl Mul<Weight> for $t {
type Output = Weight;
fn mul(self, b: Weight) -> Weight {
Weight { ref_time: u64::from(self) * b.ref_time }
Weight {
ref_time: u64::from(self) * b.ref_time,
proof_size: u64::from(self) * b.proof_size,
}
}
}
)*
Expand All @@ -292,7 +377,7 @@ where
{
type Output = Self;
fn div(self, b: T) -> Self {
Self { ref_time: self.ref_time / b }
Self { ref_time: self.ref_time / b, proof_size: self.proof_size / b }
}
}

Expand All @@ -310,7 +395,7 @@ impl CheckedSub for Weight {

impl core::fmt::Display for Weight {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
write!(f, "Weight(ref_time: {})", self.ref_time)
write!(f, "Weight(ref_time: {}, proof_size: {})", self.ref_time, self.proof_size)
}
}

Expand All @@ -325,12 +410,18 @@ impl Bounded for Weight {

impl AddAssign for Weight {
fn add_assign(&mut self, other: Self) {
*self = Self { ref_time: self.ref_time + other.ref_time };
*self = Self {
ref_time: self.ref_time + other.ref_time,
proof_size: self.proof_size + other.proof_size,
};
Comment on lines +435 to +438
Copy link
Member

Choose a reason for hiding this comment

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

Is Rust not default implementing these?!

Suggested change
*self = Self {
ref_time: self.ref_time + other.ref_time,
proof_size: self.proof_size + other.proof_size,
};
*self = *self + other

}
}

impl SubAssign for Weight {
fn sub_assign(&mut self, other: Self) {
*self = Self { ref_time: self.ref_time - other.ref_time };
*self = Self {
ref_time: self.ref_time - other.ref_time,
proof_size: self.proof_size - other.proof_size,
};
Comment on lines +444 to +447
Copy link
Member

@ggwpez ggwpez Sep 15, 2022

Choose a reason for hiding this comment

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

Suggested change
*self = Self {
ref_time: self.ref_time - other.ref_time,
proof_size: self.proof_size - other.proof_size,
};
*self = *self - other

}
}