From ff00f4bfbf1b8397d459b2f4479fd377aa68332b Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 11:03:25 +0200 Subject: [PATCH 01/10] Rename WeightMeter functions --- primitives/weights/src/weight_meter.rs | 28 +++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index ab7b6c63ed383..9b7c70b9f529b 100644 --- a/primitives/weights/src/weight_meter.rs +++ b/primitives/weights/src/weight_meter.rs @@ -72,27 +72,49 @@ impl WeightMeter { } /// Consume some weight and defensively fail if it is over the limit. Saturate in any case. + #[deprecated = "Use `consume` instead"] 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. + /// + /// This is provided as a less noisy version of `defensive_saturating_accrue`. + 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 = "Use `try_consume` instead"] pub fn check_accrue(&mut self, w: Weight) -> bool { + self.try_consume(w).is_ok() + } + + /// Consume the given weight after checking that it can be consumed and return `Ok`. Otherwise + /// do nothing and return `Err`. + pub fn try_consume(&mut self, w: Weight) -> Result<(), ()> { self.consumed.checked_add(&w).map_or(false, |test| { if test.any_gt(self.limit) { - false + Err(()) } else { self.consumed = test; - true + Ok(()) } }) } /// Check if the given weight can be consumed. + #[deprecated = "Use `can_consume` instead"] pub fn can_accrue(&self, w: Weight) -> bool { self.consumed.checked_add(&w).map_or(false, |t| t.all_lte(self.limit)) } + + /// 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)) + } } #[cfg(test)] From 3ff3c4603f2312add39c46e9876c1a4542e532a2 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 11:07:10 +0200 Subject: [PATCH 02/10] Fixes Signed-off-by: Oliver Tale-Yazdi --- primitives/weights/src/weight_meter.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index 9b7c70b9f529b..b5102be54f96a 100644 --- a/primitives/weights/src/weight_meter.rs +++ b/primitives/weights/src/weight_meter.rs @@ -72,7 +72,7 @@ impl WeightMeter { } /// Consume some weight and defensively fail if it is over the limit. Saturate in any case. - #[deprecated = "Use `consume` instead"] + #[deprecated = "Use `consume` instead. Will be removed after December 2023."] pub fn defensive_saturating_accrue(&mut self, w: Weight) { self.consume(w); } @@ -87,7 +87,7 @@ impl WeightMeter { /// Consume the given weight after checking that it can be consumed and return `true`. Otherwise /// do nothing and return `false`. - #[deprecated = "Use `try_consume` instead"] + #[deprecated = "Use `try_consume` instead. Will be removed after December 2023."] pub fn check_accrue(&mut self, w: Weight) -> bool { self.try_consume(w).is_ok() } @@ -95,7 +95,7 @@ impl WeightMeter { /// Consume the given weight after checking that it can be consumed and return `Ok`. Otherwise /// do nothing and return `Err`. pub fn try_consume(&mut self, w: Weight) -> Result<(), ()> { - self.consumed.checked_add(&w).map_or(false, |test| { + self.consumed.checked_add(&w).map_or(Err(()), |test| { if test.any_gt(self.limit) { Err(()) } else { @@ -106,7 +106,7 @@ impl WeightMeter { } /// Check if the given weight can be consumed. - #[deprecated = "Use `can_consume` instead"] + #[deprecated = "Use `can_consume` instead. Will be removed after December 2023."] pub fn can_accrue(&self, w: Weight) -> bool { self.consumed.checked_add(&w).map_or(false, |t| t.all_lte(self.limit)) } From 7d0b904294660f9d73d6bbaa5bca211b4fe2850d Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 11:40:01 +0200 Subject: [PATCH 03/10] Fixup and doc + tests Signed-off-by: Oliver Tale-Yazdi --- primitives/weights/src/weight_meter.rs | 89 +++++++++++++++++++++++--- 1 file changed, 80 insertions(+), 9 deletions(-) diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index b5102be54f96a..0a20a85728bd3 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.check_accrue(Weight::from_parts(6, 0))); +/// assert_eq!(meter.remaining(), Weight::from_parts(4, 0)); +/// // There is not enough weight remaining for an operation with (5, 0) weight. +/// assert!(!meter.check_accrue(Weight::from_parts(5, 0))); +/// // 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,7 +107,7 @@ impl WeightMeter { } /// Consume some weight and defensively fail if it is over the limit. Saturate in any case. - #[deprecated = "Use `consume` instead. Will be removed after December 2023."] + #[deprecated(note = "Use `consume` instead. Will be removed after December 2023.")] pub fn defensive_saturating_accrue(&mut self, w: Weight) { self.consume(w); } @@ -87,7 +122,7 @@ impl WeightMeter { /// Consume the given weight after checking that it can be consumed and return `true`. Otherwise /// do nothing and return `false`. - #[deprecated = "Use `try_consume` instead. Will be removed after December 2023."] + #[deprecated(note = "Use `try_consume` instead. Will be removed after December 2023.")] pub fn check_accrue(&mut self, w: Weight) -> bool { self.try_consume(w).is_ok() } @@ -106,7 +141,7 @@ impl WeightMeter { } /// Check if the given weight can be consumed. - #[deprecated = "Use `can_consume` instead. Will be removed after December 2023."] + #[deprecated(note = "Use `can_consume` instead. Will be removed after December 2023.")] pub fn can_accrue(&self, w: Weight) -> bool { self.consumed.checked_add(&w).map_or(false, |t| t.all_lte(self.limit)) } @@ -120,6 +155,7 @@ impl WeightMeter { #[cfg(test)] mod tests { use crate::*; + use sp_arithmetic::traits::Zero; #[test] fn weight_meter_remaining_works() { @@ -201,4 +237,39 @@ 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)] + #[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)); + } } From 373bc6fdfcb7376dddaaadb9196fa45299ad3f60 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 11:53:43 +0200 Subject: [PATCH 04/10] One more test Signed-off-by: Oliver Tale-Yazdi --- primitives/weights/src/weight_meter.rs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index 0a20a85728bd3..efda744b9f9f1 100644 --- a/primitives/weights/src/weight_meter.rs +++ b/primitives/weights/src/weight_meter.rs @@ -265,6 +265,19 @@ mod tests { 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")] From 66bff346d6c9e5981eb22cf1012962e0125752e4 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 12:32:51 +0200 Subject: [PATCH 05/10] Fixup pallets Signed-off-by: Oliver Tale-Yazdi --- 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 +++++------ 7 files changed, 58 insertions(+), 47 deletions(-) diff --git a/frame/glutton/src/lib.rs b/frame/glutton/src/lib.rs index ae3e4f87f0e80..0f37c210c82c1 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() } } @@ -275,7 +275,7 @@ pub mod pallet { 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); @@ -306,7 +306,7 @@ pub mod pallet { 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..a959743f8a4ff 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( + weight.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); + weight.consume(base_weight); + weight.consume(call_weight); Ok(result) } } From 7d70bda84d47410ffa4a349def791f76a290bb6a Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 13:57:47 +0200 Subject: [PATCH 06/10] Use correct function :facepalm: Signed-off-by: Oliver Tale-Yazdi --- frame/scheduler/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/frame/scheduler/src/lib.rs b/frame/scheduler/src/lib.rs index a959743f8a4ff..77adb18146161 100644 --- a/frame/scheduler/src/lib.rs +++ b/frame/scheduler/src/lib.rs @@ -1073,7 +1073,7 @@ impl Pallet { Err(_) => return Err((Unavailable, Some(task))), }; - weight.consume(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(), @@ -1160,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.consume(base_weight); - weight.consume(call_weight); + let _ = weight.try_consume(base_weight); + let _ = weight.try_consume(call_weight); Ok(result) } } From 9961b606cf4865c0a62a367af44246c602d62b72 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 17:13:44 +0200 Subject: [PATCH 07/10] Apply suggestions from code review Co-authored-by: Juan --- primitives/weights/src/weight_meter.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index efda744b9f9f1..a1848712c9a5d 100644 --- a/primitives/weights/src/weight_meter.rs +++ b/primitives/weights/src/weight_meter.rs @@ -33,10 +33,10 @@ 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 (6, 0) weight. -/// assert!(meter.check_accrue(Weight::from_parts(6, 0))); +/// 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.check_accrue(Weight::from_parts(5, 0))); +/// 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)); /// ``` From 3210272ad780de1c052aa3b31f19e64455b5ac54 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 17:14:46 +0200 Subject: [PATCH 08/10] Update primitives/weights/src/weight_meter.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- primitives/weights/src/weight_meter.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index a1848712c9a5d..400345c0c583c 100644 --- a/primitives/weights/src/weight_meter.rs +++ b/primitives/weights/src/weight_meter.rs @@ -127,8 +127,9 @@ impl WeightMeter { self.try_consume(w).is_ok() } - /// Consume the given weight after checking that it can be consumed and return `Ok`. Otherwise - /// do nothing and return `Err`. + /// 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) { From 339d294b5d225b71a9d723b4bc2cffce19ea3d43 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 17 Jul 2023 17:16:41 +0200 Subject: [PATCH 09/10] Update primitives/weights/src/weight_meter.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- primitives/weights/src/weight_meter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index 400345c0c583c..db1a11cfb4e88 100644 --- a/primitives/weights/src/weight_meter.rs +++ b/primitives/weights/src/weight_meter.rs @@ -144,7 +144,7 @@ impl WeightMeter { /// 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.consumed.checked_add(&w).map_or(false, |t| t.all_lte(self.limit)) + self.can_consume(w) } /// Check if the given weight can be consumed. From d1366234592b946bda92437bb9a8b2cd0f3bbe51 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 18 Jul 2023 11:25:08 +0200 Subject: [PATCH 10/10] Update primitives/weights/src/weight_meter.rs --- primitives/weights/src/weight_meter.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/primitives/weights/src/weight_meter.rs b/primitives/weights/src/weight_meter.rs index db1a11cfb4e88..3b0b21ea8799a 100644 --- a/primitives/weights/src/weight_meter.rs +++ b/primitives/weights/src/weight_meter.rs @@ -113,8 +113,6 @@ impl WeightMeter { } /// Consume some weight and defensively fail if it is over the limit. Saturate in any case. - /// - /// This is provided as a less noisy version of `defensive_saturating_accrue`. pub fn consume(&mut self, w: Weight) { self.consumed.saturating_accrue(w); debug_assert!(self.consumed.all_lte(self.limit), "Weight counter overflow");