From cfe2432fdba3ff498b3c867c18bb4c25d0625a74 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 18 Jul 2023 16:38:44 +0200 Subject: [PATCH] `WeightMeter`: more consistent naming (#14586) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Rename WeightMeter functions * Fixes Signed-off-by: Oliver Tale-Yazdi * Fixup and doc + tests Signed-off-by: Oliver Tale-Yazdi * One more test Signed-off-by: Oliver Tale-Yazdi * Fixup pallets Signed-off-by: Oliver Tale-Yazdi * Use correct function :facepalm: Signed-off-by: Oliver Tale-Yazdi * Apply suggestions from code review Co-authored-by: Juan * Update primitives/weights/src/weight_meter.rs Co-authored-by: Bastian Köcher * Update primitives/weights/src/weight_meter.rs Co-authored-by: Bastian Köcher * Update primitives/weights/src/weight_meter.rs --------- Signed-off-by: Oliver Tale-Yazdi Co-authored-by: Juan Co-authored-by: Bastian Köcher --- frame/glutton/src/lib.rs | 8 +- frame/message-queue/src/benchmarking.rs | 2 +- frame/message-queue/src/lib.rs | 46 +++++---- frame/message-queue/src/mock.rs | 4 +- frame/message-queue/src/mock_helpers.rs | 2 +- frame/message-queue/src/tests.rs | 22 ++--- frame/scheduler/src/lib.rs | 21 ++-- primitives/weights/src/weight_meter.rs | 125 ++++++++++++++++++++++-- 8 files changed, 173 insertions(+), 57 deletions(-) diff --git a/frame/glutton/src/lib.rs b/frame/glutton/src/lib.rs index 838bd9b67d296..1db68003abe42 100644 --- a/frame/glutton/src/lib.rs +++ b/frame/glutton/src/lib.rs @@ -175,7 +175,7 @@ pub mod pallet { fn on_idle(_: BlockNumberFor, remaining_weight: Weight) -> Weight { let mut meter = WeightMeter::from_limit(remaining_weight); - if !meter.check_accrue(T::WeightInfo::empty_on_idle()) { + if meter.try_consume(T::WeightInfo::empty_on_idle()).is_err() { return T::WeightInfo::empty_on_idle() } @@ -191,7 +191,7 @@ pub mod pallet { Self::waste_at_most_proof_size(&mut meter); Self::waste_at_most_ref_time(&mut meter); - meter.consumed + meter.consumed() } } @@ -273,7 +273,7 @@ pub mod pallet { pub(crate) fn waste_at_most_proof_size(meter: &mut WeightMeter) { let Ok(n) = Self::calculate_proof_size_iters(&meter) else { return }; - meter.defensive_saturating_accrue(T::WeightInfo::waste_proof_size_some(n)); + meter.consume(T::WeightInfo::waste_proof_size_some(n)); (0..n).for_each(|i| { TrashData::::get(i); @@ -302,7 +302,7 @@ pub mod pallet { /// Tries to come as close to the limit as possible. pub(crate) fn waste_at_most_ref_time(meter: &mut WeightMeter) { let Ok(n) = Self::calculate_ref_time_iters(&meter) else { return }; - meter.defensive_saturating_accrue(T::WeightInfo::waste_ref_time_iter(n)); + meter.consume(T::WeightInfo::waste_ref_time_iter(n)); let clobber = Self::waste_ref_time_iter(vec![0u8; 64], n); diff --git a/frame/message-queue/src/benchmarking.rs b/frame/message-queue/src/benchmarking.rs index 53c84c3da1c07..bbd321ceadd1a 100644 --- a/frame/message-queue/src/benchmarking.rs +++ b/frame/message-queue/src/benchmarking.rs @@ -166,7 +166,7 @@ mod benchmarks { } assert_eq!(ServiceHead::::get().unwrap(), 10u32.into()); - assert_eq!(weight.consumed, T::WeightInfo::bump_service_head()); + assert_eq!(weight.consumed(), T::WeightInfo::bump_service_head()); } #[benchmark] diff --git a/frame/message-queue/src/lib.rs b/frame/message-queue/src/lib.rs index 55a41643993aa..08795914ebe78 100644 --- a/frame/message-queue/src/lib.rs +++ b/frame/message-queue/src/lib.rs @@ -737,7 +737,7 @@ impl Pallet { /// /// Returns the current head if it got be bumped and `None` otherwise. fn bump_service_head(weight: &mut WeightMeter) -> Option> { - if !weight.check_accrue(T::WeightInfo::bump_service_head()) { + if weight.try_consume(T::WeightInfo::bump_service_head()).is_err() { return None } @@ -865,7 +865,7 @@ impl Pallet { book_state.message_count, book_state.size, ); - Ok(weight_counter.consumed.saturating_add(page_weight)) + Ok(weight_counter.consumed().saturating_add(page_weight)) }, } } @@ -948,9 +948,13 @@ impl Pallet { overweight_limit: Weight, ) -> (bool, Option>) { use PageExecutionStatus::*; - if !weight.check_accrue( - T::WeightInfo::service_queue_base().saturating_add(T::WeightInfo::ready_ring_unknit()), - ) { + if weight + .try_consume( + T::WeightInfo::service_queue_base() + .saturating_add(T::WeightInfo::ready_ring_unknit()), + ) + .is_err() + { return (false, None) } @@ -1003,10 +1007,13 @@ impl Pallet { overweight_limit: Weight, ) -> (u32, PageExecutionStatus) { use PageExecutionStatus::*; - if !weight.check_accrue( - T::WeightInfo::service_page_base_completion() - .max(T::WeightInfo::service_page_base_no_completion()), - ) { + if weight + .try_consume( + T::WeightInfo::service_page_base_completion() + .max(T::WeightInfo::service_page_base_no_completion()), + ) + .is_err() + { return (0, Bailed) } @@ -1066,7 +1073,7 @@ impl Pallet { if page.is_complete() { return ItemExecutionStatus::NoItem } - if !weight.check_accrue(T::WeightInfo::service_page_item()) { + if weight.try_consume(T::WeightInfo::service_page_item()).is_err() { return ItemExecutionStatus::Bailed } @@ -1163,7 +1170,7 @@ impl Pallet { ) -> MessageExecutionStatus { let hash = sp_io::hashing::blake2_256(message); use ProcessMessageError::*; - let prev_consumed = meter.consumed; + let prev_consumed = meter.consumed(); let mut id = hash; match T::MessageProcessor::process_message(message, origin.clone(), meter, &mut id) { @@ -1193,7 +1200,7 @@ impl Pallet { }, Ok(success) => { // Success - let weight_used = meter.consumed.saturating_sub(prev_consumed); + let weight_used = meter.consumed().saturating_sub(prev_consumed); Self::deposit_event(Event::::Processed { id, origin, weight_used, success }); MessageExecutionStatus::Processed }, @@ -1254,7 +1261,7 @@ impl ServiceQueues for Pallet { let mut next = match Self::bump_service_head(&mut weight) { Some(h) => h, - None => return weight.consumed, + None => return weight.consumed(), }; // The last queue that did not make any progress. // The loop aborts as soon as it arrives at this queue again without making any progress @@ -1280,7 +1287,7 @@ impl ServiceQueues for Pallet { None => break, } } - weight.consumed + weight.consumed() } /// Execute a single overweight message. @@ -1291,10 +1298,13 @@ impl ServiceQueues for Pallet { (message_origin, page, index): Self::OverweightMessageAddress, ) -> Result { let mut weight = WeightMeter::from_limit(weight_limit); - if !weight.check_accrue( - T::WeightInfo::execute_overweight_page_removed() - .max(T::WeightInfo::execute_overweight_page_updated()), - ) { + if weight + .try_consume( + T::WeightInfo::execute_overweight_page_removed() + .max(T::WeightInfo::execute_overweight_page_updated()), + ) + .is_err() + { return Err(ExecuteOverweightError::InsufficientWeight) } diff --git a/frame/message-queue/src/mock.rs b/frame/message-queue/src/mock.rs index 5a68a161eb374..04b763d59cffd 100644 --- a/frame/message-queue/src/mock.rs +++ b/frame/message-queue/src/mock.rs @@ -188,7 +188,7 @@ impl ProcessMessage for RecordingMessageProcessor { }; let required = Weight::from_parts(weight, weight); - if meter.check_accrue(required) { + if meter.try_consume(required).is_ok() { let mut m = MessagesProcessed::get(); m.push((message.to_vec(), origin)); MessagesProcessed::set(m); @@ -245,7 +245,7 @@ impl ProcessMessage for CountingMessageProcessor { } let required = Weight::from_parts(1, 1); - if meter.check_accrue(required) { + if meter.try_consume(required).is_ok() { NumMessagesProcessed::set(NumMessagesProcessed::get() + 1); Ok(true) } else { diff --git a/frame/message-queue/src/mock_helpers.rs b/frame/message-queue/src/mock_helpers.rs index 65b1e5af9099a..4e3cb323be729 100644 --- a/frame/message-queue/src/mock_helpers.rs +++ b/frame/message-queue/src/mock_helpers.rs @@ -66,7 +66,7 @@ where ) -> Result { let required = Weight::from_parts(REQUIRED_WEIGHT, REQUIRED_WEIGHT); - if meter.check_accrue(required) { + if meter.try_consume(required).is_ok() { Ok(true) } else { Err(ProcessMessageError::Overweight(required)) diff --git a/frame/message-queue/src/tests.rs b/frame/message-queue/src/tests.rs index 6587c949bde05..7b7b51efc66f2 100644 --- a/frame/message-queue/src/tests.rs +++ b/frame/message-queue/src/tests.rs @@ -381,7 +381,7 @@ fn service_queue_bails() { let mut meter = WeightMeter::from_limit(1.into_weight()); assert_storage_noop!(MessageQueue::service_queue(0u32.into(), &mut meter, Weight::MAX)); - assert!(meter.consumed.is_zero()); + assert!(meter.consumed().is_zero()); }); // Not enough weight for `ready_ring_unknit`. test_closure(|| { @@ -389,7 +389,7 @@ fn service_queue_bails() { let mut meter = WeightMeter::from_limit(1.into_weight()); assert_storage_noop!(MessageQueue::service_queue(0u32.into(), &mut meter, Weight::MAX)); - assert!(meter.consumed.is_zero()); + assert!(meter.consumed().is_zero()); }); // Not enough weight for `service_queue_base` and `ready_ring_unknit`. test_closure(|| { @@ -398,7 +398,7 @@ fn service_queue_bails() { let mut meter = WeightMeter::from_limit(3.into_weight()); assert_storage_noop!(MessageQueue::service_queue(0.into(), &mut meter, Weight::MAX)); - assert!(meter.consumed.is_zero()); + assert!(meter.consumed().is_zero()); }); } @@ -458,7 +458,7 @@ fn service_page_bails() { &mut meter, Weight::MAX )); - assert!(meter.consumed.is_zero()); + assert!(meter.consumed().is_zero()); }); // Not enough weight for `service_page_base_no_completion`. test_closure(|| { @@ -475,7 +475,7 @@ fn service_page_bails() { &mut meter, Weight::MAX )); - assert!(meter.consumed.is_zero()); + assert!(meter.consumed().is_zero()); }); } @@ -586,7 +586,7 @@ fn bump_service_head_bails() { let _guard = StorageNoopGuard::default(); let mut meter = WeightMeter::from_limit(1.into_weight()); assert!(MessageQueue::bump_service_head(&mut meter).is_none()); - assert_eq!(meter.consumed, 0.into_weight()); + assert_eq!(meter.consumed(), 0.into_weight()); }); } @@ -597,16 +597,16 @@ fn bump_service_head_trivial_works() { let mut meter = WeightMeter::max_limit(); assert_eq!(MessageQueue::bump_service_head(&mut meter), None, "Cannot bump"); - assert_eq!(meter.consumed, 2.into_weight()); + assert_eq!(meter.consumed(), 2.into_weight()); setup_bump_service_head::(0.into(), 1.into()); assert_eq!(MessageQueue::bump_service_head(&mut meter), Some(0.into())); assert_eq!(ServiceHead::::get().unwrap(), 1.into(), "Bumped the head"); - assert_eq!(meter.consumed, 4.into_weight()); + assert_eq!(meter.consumed(), 4.into_weight()); assert_eq!(MessageQueue::bump_service_head(&mut meter), None, "Cannot bump"); - assert_eq!(meter.consumed, 6.into_weight()); + assert_eq!(meter.consumed(), 6.into_weight()); }); } @@ -649,7 +649,7 @@ fn service_page_item_consumes_correct_weight() { ), ItemExecutionStatus::Executed(true) ); - assert_eq!(weight.consumed, 5.into_weight()); + assert_eq!(weight.consumed(), 5.into_weight()); }); } @@ -673,7 +673,7 @@ fn service_page_item_skips_perm_overweight_message() { ), ItemExecutionStatus::Executed(false) ); - assert_eq!(weight.consumed, 2.into_weight()); + assert_eq!(weight.consumed(), 2.into_weight()); assert_last_event::( Event::OverweightEnqueued { id: blake2_256(b"TooMuch"), diff --git a/frame/scheduler/src/lib.rs b/frame/scheduler/src/lib.rs index 4e12e0332f422..77adb18146161 100644 --- a/frame/scheduler/src/lib.rs +++ b/frame/scheduler/src/lib.rs @@ -296,7 +296,7 @@ pub mod pallet { fn on_initialize(now: BlockNumberFor) -> Weight { let mut weight_counter = WeightMeter::from_limit(T::MaximumWeight::get()); Self::service_agendas(&mut weight_counter, now, u32::max_value()); - weight_counter.consumed + weight_counter.consumed() } } @@ -959,7 +959,7 @@ use ServiceTaskError::*; impl Pallet { /// Service up to `max` agendas queue starting from earliest incompletely executed agenda. fn service_agendas(weight: &mut WeightMeter, now: BlockNumberFor, max: u32) { - if !weight.check_accrue(T::WeightInfo::service_agendas_base()) { + if weight.try_consume(T::WeightInfo::service_agendas_base()).is_err() { return } @@ -970,7 +970,7 @@ impl Pallet { let max_items = T::MaxScheduledPerBlock::get(); let mut count_down = max; let service_agenda_base_weight = T::WeightInfo::service_agenda_base(max_items); - while count_down > 0 && when <= now && weight.can_accrue(service_agenda_base_weight) { + while count_down > 0 && when <= now && weight.can_consume(service_agenda_base_weight) { if !Self::service_agenda(weight, &mut executed, now, when, u32::max_value()) { incomplete_since = incomplete_since.min(when); } @@ -1001,8 +1001,9 @@ impl Pallet { }) .collect::>(); ordered.sort_by_key(|k| k.1); - let within_limit = - weight.check_accrue(T::WeightInfo::service_agenda_base(ordered.len() as u32)); + let within_limit = weight + .try_consume(T::WeightInfo::service_agenda_base(ordered.len() as u32)) + .is_ok(); debug_assert!(within_limit, "weight limit should have been checked in advance"); // Items which we know can be executed and have postponed for execution in a later block. @@ -1020,7 +1021,7 @@ impl Pallet { task.maybe_id.is_some(), task.maybe_periodic.is_some(), ); - if !weight.can_accrue(base_weight) { + if !weight.can_consume(base_weight) { postponed += 1; break } @@ -1072,7 +1073,7 @@ impl Pallet { Err(_) => return Err((Unavailable, Some(task))), }; - weight.check_accrue(T::WeightInfo::service_task( + let _ = weight.try_consume(T::WeightInfo::service_task( lookup_len.map(|x| x as usize), task.maybe_id.is_some(), task.maybe_periodic.is_some(), @@ -1148,7 +1149,7 @@ impl Pallet { // We only allow a scheduled call if it cannot push the weight past the limit. let max_weight = base_weight.saturating_add(call_weight); - if !weight.can_accrue(max_weight) { + if !weight.can_consume(max_weight) { return Err(Overweight) } @@ -1159,8 +1160,8 @@ impl Pallet { (error_and_info.post_info.actual_weight, Err(error_and_info.error)), }; let call_weight = maybe_actual_call_weight.unwrap_or(call_weight); - weight.check_accrue(base_weight); - weight.check_accrue(call_weight); + let _ = weight.try_consume(base_weight); + let _ = weight.try_consume(call_weight); Ok(result) } } diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index ab7b6c63ed383..3b0b21ea8799a 100644 --- a/primitives/weights/src/weight_meter.rs +++ b/primitives/weights/src/weight_meter.rs @@ -32,18 +32,21 @@ use sp_arithmetic::Perbill; /// /// // The weight is limited to (10, 0). /// let mut meter = WeightMeter::from_limit(Weight::from_parts(10, 0)); -/// // There is enough weight remaining for an operation with (5, 0) weight. -/// assert!(meter.check_accrue(Weight::from_parts(5, 0))); -/// // There is not enough weight remaining for an operation with (6, 0) weight. -/// assert!(!meter.check_accrue(Weight::from_parts(6, 0))); +/// // There is enough weight remaining for an operation with (6, 0) weight. +/// assert!(meter.try_consume(Weight::from_parts(6, 0)).is_ok()); +/// assert_eq!(meter.remaining(), Weight::from_parts(4, 0)); +/// // There is not enough weight remaining for an operation with (5, 0) weight. +/// assert!(!meter.try_consume(Weight::from_parts(5, 0)).is_ok()); +/// // The total limit is obviously unchanged: +/// assert_eq!(meter.limit(), Weight::from_parts(10, 0)); /// ``` #[derive(Debug, Clone)] pub struct WeightMeter { /// The already consumed weight. - pub consumed: Weight, + consumed: Weight, /// The maximal consumable weight. - pub limit: Weight, + limit: Weight, } impl WeightMeter { @@ -57,6 +60,16 @@ impl WeightMeter { Self::from_limit(Weight::MAX) } + /// The already consumed weight. + pub fn consumed(&self) -> Weight { + self.consumed + } + + /// The limit can ever be accrued. + pub fn limit(&self) -> Weight { + self.limit + } + /// The remaining weight that can still be consumed. pub fn remaining(&self) -> Weight { self.limit.saturating_sub(self.consumed) @@ -65,6 +78,28 @@ impl WeightMeter { /// The ratio of consumed weight to the limit. /// /// Calculates one ratio per component and returns the largest. + /// + /// # Example + /// ```rust + /// use sp_weights::{Weight, WeightMeter}; + /// use sp_arithmetic::Perbill; + /// + /// let mut meter = WeightMeter::from_limit(Weight::from_parts(10, 20)); + /// // Nothing consumed so far: + /// assert_eq!(meter.consumed_ratio(), Perbill::from_percent(0)); + /// meter.consume(Weight::from_parts(5, 5)); + /// // The ref-time is the larger ratio: + /// assert_eq!(meter.consumed_ratio(), Perbill::from_percent(50)); + /// meter.consume(Weight::from_parts(1, 10)); + /// // Now the larger ratio is proof-size: + /// assert_eq!(meter.consumed_ratio(), Perbill::from_percent(75)); + /// // Eventually it reaches 100%: + /// meter.consume(Weight::from_parts(4, 0)); + /// assert_eq!(meter.consumed_ratio(), Perbill::from_percent(100)); + /// // Saturating the second component won't change anything anymore: + /// meter.consume(Weight::from_parts(0, 5)); + /// assert_eq!(meter.consumed_ratio(), Perbill::from_percent(100)); + /// ``` pub fn consumed_ratio(&self) -> Perbill { let time = Perbill::from_rational(self.consumed.ref_time(), self.limit.ref_time()); let pov = Perbill::from_rational(self.consumed.proof_size(), self.limit.proof_size()); @@ -72,25 +107,46 @@ impl WeightMeter { } /// Consume some weight and defensively fail if it is over the limit. Saturate in any case. + #[deprecated(note = "Use `consume` instead. Will be removed after December 2023.")] pub fn defensive_saturating_accrue(&mut self, w: Weight) { + self.consume(w); + } + + /// Consume some weight and defensively fail if it is over the limit. Saturate in any case. + pub fn consume(&mut self, w: Weight) { self.consumed.saturating_accrue(w); debug_assert!(self.consumed.all_lte(self.limit), "Weight counter overflow"); } - /// Consume the given weight after checking that it can be consumed. Otherwise do nothing. + /// Consume the given weight after checking that it can be consumed and return `true`. Otherwise + /// do nothing and return `false`. + #[deprecated(note = "Use `try_consume` instead. Will be removed after December 2023.")] pub fn check_accrue(&mut self, w: Weight) -> bool { - self.consumed.checked_add(&w).map_or(false, |test| { + self.try_consume(w).is_ok() + } + + /// Consume the given weight after checking that it can be consumed. + /// + /// Returns `Ok` if the weight can be consumed or otherwise an `Err`. + pub fn try_consume(&mut self, w: Weight) -> Result<(), ()> { + self.consumed.checked_add(&w).map_or(Err(()), |test| { if test.any_gt(self.limit) { - false + Err(()) } else { self.consumed = test; - true + Ok(()) } }) } /// Check if the given weight can be consumed. + #[deprecated(note = "Use `can_consume` instead. Will be removed after December 2023.")] pub fn can_accrue(&self, w: Weight) -> bool { + self.can_consume(w) + } + + /// Check if the given weight can be consumed. + pub fn can_consume(&self, w: Weight) -> bool { self.consumed.checked_add(&w).map_or(false, |t| t.all_lte(self.limit)) } } @@ -98,6 +154,7 @@ impl WeightMeter { #[cfg(test)] mod tests { use crate::*; + use sp_arithmetic::traits::Zero; #[test] fn weight_meter_remaining_works() { @@ -179,4 +236,52 @@ mod tests { assert!(meter.check_accrue(Weight::from_parts(0, 4))); assert_eq!(meter.consumed_ratio(), Perbill::from_percent(100)); } + + #[test] + fn try_consume_works() { + let mut meter = WeightMeter::from_limit(Weight::from_parts(10, 0)); + + assert!(meter.try_consume(Weight::from_parts(11, 0)).is_err()); + assert!(meter.consumed().is_zero(), "No modification"); + + assert!(meter.try_consume(Weight::from_parts(9, 0)).is_ok()); + assert!(meter.try_consume(Weight::from_parts(2, 0)).is_err()); + assert!(meter.try_consume(Weight::from_parts(1, 0)).is_ok()); + assert!(meter.remaining().is_zero()); + assert_eq!(meter.consumed(), Weight::from_parts(10, 0)); + } + + #[test] + fn can_consume_works() { + let mut meter = WeightMeter::from_limit(Weight::from_parts(10, 0)); + + assert!(!meter.can_consume(Weight::from_parts(11, 0))); + assert!(meter.consumed().is_zero(), "No modification"); + + assert!(meter.can_consume(Weight::from_parts(9, 0))); + meter.consume(Weight::from_parts(9, 0)); + assert!(!meter.can_consume(Weight::from_parts(2, 0))); + assert!(meter.can_consume(Weight::from_parts(1, 0))); + } + + #[test] + #[cfg(debug_assertions)] + fn consume_works() { + let mut meter = WeightMeter::from_limit(Weight::from_parts(5, 10)); + + meter.consume(Weight::from_parts(4, 0)); + assert_eq!(meter.remaining(), Weight::from_parts(1, 10)); + meter.consume(Weight::from_parts(1, 0)); + assert_eq!(meter.remaining(), Weight::from_parts(0, 10)); + meter.consume(Weight::from_parts(0, 10)); + assert_eq!(meter.consumed(), Weight::from_parts(5, 10)); + } + + #[test] + #[cfg(debug_assertions)] + #[should_panic(expected = "Weight counter overflow")] + fn consume_defensive_fail() { + let mut meter = WeightMeter::from_limit(Weight::from_parts(10, 0)); + let _ = meter.consume(Weight::from_parts(11, 0)); + } }