diff --git a/moli-renderer-v8/src/host/timers.rs b/moli-renderer-v8/src/host/timers.rs index c019569cb9..ca64fe9e45 100644 --- a/moli-renderer-v8/src/host/timers.rs +++ b/moli-renderer-v8/src/host/timers.rs @@ -22,7 +22,7 @@ use crate::{ get_private_value, script_nonce_from_host_defined_options, }, }; -use moli_time::{TimerId, TimerReadyAllowance, TimerScheduler}; +use moli_time::{TimerId, TimerScheduler}; use moli_webapi_declare::WebApiObject; #[derive(WebApiObject)] @@ -211,7 +211,6 @@ impl ScheduledTimerTask { } } -const MIN_DELAY_TIMER_READY_EARLY_ALLOWANCE: Duration = Duration::from_millis(1); const TIMER_CALLBACK_WATCHDOG_TIMEOUT: Duration = Duration::from_secs(8); #[derive(Default)] @@ -469,18 +468,17 @@ impl HostTimeoutScheduler { ) else { return 0; }; - self.scheduler - .schedule_after( - ScheduledTimerTask { - callback: ScheduledTimerCallback::WindowWebIdl(callback), - owner, - is_interval, - extra_args, - }, - u64::from(delay_ms), - Instant::now(), - ) - .get() + let task = ScheduledTimerTask { + callback: ScheduledTimerCallback::WindowWebIdl(callback), + owner, + is_interval, + extra_args, + }; + let now = Instant::now(); + let id = self + .scheduler + .schedule_after(task, u64::from(delay_ms), now); + id.get() } pub(crate) fn queue_source_once_with_receiver<'s>( @@ -737,11 +735,12 @@ impl HostTimeoutScheduler { selection: RendererPageTimerSelection, ) -> HostTimeoutRunResult { let now_ms = crate::window_host::current_time_ms(); - let Some(timer) = self.scheduler.take_next_ready_matching( - Instant::now(), - min_delay_ready_allowance(), - |task| task.matches_selection(selection, now_ms), - ) else { + let Some(timer) = self + .scheduler + .take_next_ready_matching(Instant::now(), |task| { + task.matches_selection(selection, now_ms) + }) + else { return HostTimeoutRunResult::Idle; }; self.run_timer(scope, timer) @@ -749,8 +748,7 @@ impl HostTimeoutScheduler { #[cfg(test)] pub(crate) fn has_ready_timer(&self) -> bool { - self.scheduler - .has_ready_timer(Instant::now(), min_delay_ready_allowance()) + self.scheduler.has_ready_timer(Instant::now()) } pub(crate) fn next_ready_timer_deadline( @@ -758,11 +756,10 @@ impl HostTimeoutScheduler { selection: RendererPageTimerSelection, ) -> Option { let now_ms = crate::window_host::current_time_ms(); - self.scheduler.next_ready_deadline_matching( - Instant::now(), - min_delay_ready_allowance(), - |task| task.matches_selection(selection, now_ms), - ) + self.scheduler + .next_ready_deadline_matching(Instant::now(), |task| { + task.matches_selection(selection, now_ms) + }) } fn run_timer( @@ -1076,13 +1073,6 @@ fn timer_callback_relevant_context_is_current( }) } -fn min_delay_ready_allowance() -> TimerReadyAllowance { - TimerReadyAllowance { - max_delay_ms: 1, - allowance: MIN_DELAY_TIMER_READY_EARLY_ALLOWANCE, - } -} - fn run_window_timer_callback( scope: &mut v8::PinScope<'_, '_>, callback: &ScheduledTimerCallback, diff --git a/moli-time/src/lib.rs b/moli-time/src/lib.rs index b5cafac5f9..6cd8699d74 100644 --- a/moli-time/src/lib.rs +++ b/moli-time/src/lib.rs @@ -6,7 +6,7 @@ use std::{ }; mod timers; -pub use timers::{ReadyTimer, TimerId, TimerReadyAllowance, TimerScheduler}; +pub use timers::{ReadyTimer, TimerId, TimerScheduler}; pub fn unix_epoch_millis() -> f64 { SystemTime::now() diff --git a/moli-time/src/timers.rs b/moli-time/src/timers.rs index da0e3fe32f..76f45280b9 100644 --- a/moli-time/src/timers.rs +++ b/moli-time/src/timers.rs @@ -17,19 +17,6 @@ impl TimerId { } } -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -pub struct TimerReadyAllowance { - pub max_delay_ms: u32, - pub allowance: Duration, -} - -impl TimerReadyAllowance { - pub const NONE: Self = Self { - max_delay_ms: 0, - allowance: Duration::ZERO, - }; -} - #[derive(Debug)] pub struct ReadyTimer { pub id: TimerId, @@ -46,6 +33,12 @@ struct ScheduledTimer { payload: T, } +impl ScheduledTimer { + fn is_ready_at(&self, now: Instant) -> bool { + self.run_at <= now + } +} + impl Ord for ScheduledTimer { fn cmp(&self, other: &Self) -> Ordering { match self.run_at.cmp(&other.run_at).reverse() { @@ -69,6 +62,7 @@ impl PartialEq for ScheduledTimer { } } +/// Timers become runnable only at or after their stored monotonic deadline. #[derive(Debug)] pub struct TimerScheduler { pending: BinaryHeap>, @@ -137,18 +131,14 @@ impl TimerScheduler { .flatten() } - pub fn take_next_ready( - &mut self, - now: Instant, - allowance: TimerReadyAllowance, - ) -> Option> { + pub fn take_next_ready(&mut self, now: Instant) -> Option> { loop { let timer = self.pending.peek()?; if !self.active.contains(&timer.id) { let _ = self.pending.pop(); continue; } - if !timer_ready(timer.run_at, timer.delay_ms, now, allowance) { + if !timer.is_ready_at(now) { return None; } @@ -168,13 +158,12 @@ impl TimerScheduler { pub fn take_next_ready_matching( &mut self, now: Instant, - allowance: TimerReadyAllowance, predicate: F, ) -> Option> where F: FnMut(&T) -> bool, { - let selected = self.next_ready_matching_timer(now, allowance, predicate)?; + let selected = self.next_ready_matching_timer(now, predicate)?; let selected_id = selected.id; let selected_sequence = selected.sequence; @@ -203,19 +192,12 @@ impl TimerScheduler { }) } - pub fn has_ready_matching( - &self, - now: Instant, - allowance: TimerReadyAllowance, - mut predicate: F, - ) -> bool + pub fn has_ready_matching(&self, now: Instant, mut predicate: F) -> bool where F: FnMut(&T) -> bool, { self.pending.iter().any(|timer| { - self.active.contains(&timer.id) - && timer_ready(timer.run_at, timer.delay_ms, now, allowance) - && predicate(&timer.payload) + self.active.contains(&timer.id) && timer.is_ready_at(now) && predicate(&timer.payload) }) } @@ -239,23 +221,17 @@ impl TimerScheduler { true } - pub fn has_ready_timer(&self, now: Instant, allowance: TimerReadyAllowance) -> bool { - self.pending.iter().any(|timer| { - self.active.contains(&timer.id) - && timer_ready(timer.run_at, timer.delay_ms, now, allowance) - }) + pub fn has_ready_timer(&self, now: Instant) -> bool { + self.pending + .iter() + .any(|timer| self.active.contains(&timer.id) && timer.is_ready_at(now)) } - pub fn next_ready_deadline_matching( - &self, - now: Instant, - allowance: TimerReadyAllowance, - predicate: F, - ) -> Option + pub fn next_ready_deadline_matching(&self, now: Instant, predicate: F) -> Option where F: FnMut(&T) -> bool, { - self.next_ready_matching_timer(now, allowance, predicate) + self.next_ready_matching_timer(now, predicate) .map(|timer| timer.run_at) } @@ -309,22 +285,17 @@ impl TimerScheduler { fn next_ready_matching_timer( &self, now: Instant, - allowance: TimerReadyAllowance, mut predicate: F, ) -> Option<&ScheduledTimer> where F: FnMut(&T) -> bool, { - let mut first_non_ready = None; let mut selected = None; for timer in &self.pending { if !self.active.contains(&timer.id) { continue; } - if !timer_ready(timer.run_at, timer.delay_ms, now, allowance) { - if first_non_ready.is_none_or(|current| timer_precedes(timer, current)) { - first_non_ready = Some(timer); - } + if !timer.is_ready_at(now) { continue; } if predicate(&timer.payload) @@ -334,11 +305,7 @@ impl TimerScheduler { } } - let selected = selected?; - if first_non_ready.is_some_and(|barrier| timer_precedes(barrier, selected)) { - return None; - } - Some(selected) + selected } } @@ -350,21 +317,58 @@ fn timer_precedes(left: &ScheduledTimer, right: &ScheduledTimer) -> boo } } -fn timer_ready( - run_at: Instant, - delay_ms: u64, - now: Instant, - allowance: TimerReadyAllowance, -) -> bool { - run_at <= now - || (delay_ms <= u64::from(allowance.max_delay_ms) - && run_at.duration_since(now).le(&allowance.allowance)) -} - #[cfg(test)] mod tests { use super::*; + #[test] + fn timers_wait_until_their_deadline_in_every_ready_query() { + let now = Instant::now(); + for delay_ms in [1, 2, 100] { + for consume_matching in [false, true] { + let mut scheduler = TimerScheduler::default(); + let id = scheduler.schedule_after("timer", delay_ms, now); + let deadline = now + Duration::from_millis(delay_ms); + + for before_deadline in [now, deadline - Duration::from_nanos(1)] { + assert!(!scheduler.has_ready_timer(before_deadline)); + assert!(!scheduler.has_ready_matching(before_deadline, |_| true)); + assert_eq!( + scheduler.next_ready_deadline_matching(before_deadline, |_| true), + None + ); + assert!(scheduler.take_next_ready(before_deadline).is_none()); + assert!( + scheduler + .take_next_ready_matching(before_deadline, |_| true) + .is_none() + ); + assert_eq!(scheduler.next_deadline(), Some(deadline)); + assert!(scheduler.ms_to_next(before_deadline).unwrap() > 0); + assert_eq!(scheduler.pending_count(), 1); + } + + assert!(scheduler.has_ready_timer(deadline)); + assert!(scheduler.has_ready_matching(deadline, |_| true)); + assert_eq!( + scheduler.next_ready_deadline_matching(deadline, |_| true), + Some(deadline) + ); + assert_eq!(scheduler.ms_to_next(deadline), Some(0)); + let ready = if consume_matching { + scheduler.take_next_ready_matching(deadline, |_| true) + } else { + scheduler.take_next_ready(deadline) + } + .unwrap(); + assert_eq!(ready.id, id); + assert_eq!(ready.payload, "timer"); + scheduler.finish_running(id); + assert_eq!(scheduler.pending_count(), 0); + } + } + } + #[test] fn ready_timers_fire_by_deadline_then_sequence() { let now = Instant::now(); @@ -374,14 +378,14 @@ mod tests { let second = scheduler.schedule_after("second", 10, now); let ready = scheduler - .take_next_ready(now + Duration::from_millis(10), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(10)) .expect("first timer should be ready"); assert_eq!(ready.id, first); assert_eq!(ready.payload, "first"); scheduler.finish_running(ready.id); let ready = scheduler - .take_next_ready(now + Duration::from_millis(10), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(10)) .expect("second timer should be ready"); assert_eq!(ready.id, second); assert_eq!(ready.payload, "second"); @@ -389,12 +393,12 @@ mod tests { assert!( scheduler - .take_next_ready(now + Duration::from_millis(10), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(10)) .is_none() ); let ready = scheduler - .take_next_ready(now + Duration::from_millis(20), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(20)) .expect("slow timer should be ready"); assert_eq!(ready.id, slow); assert_eq!(ready.payload, "slow"); @@ -411,15 +415,10 @@ mod tests { assert_eq!(scheduler.next_deadline(), Some(deadline)); assert!( scheduler - .take_next_ready( - deadline - Duration::from_millis(1), - TimerReadyAllowance::NONE - ) + .take_next_ready(deadline - Duration::from_millis(1)) .is_none() ); - let ready = scheduler - .take_next_ready(deadline, TimerReadyAllowance::NONE) - .unwrap(); + let ready = scheduler.take_next_ready(deadline).unwrap(); assert_eq!(ready.id, id); assert_eq!(ready.delay_ms, delay_ms); assert!(scheduler.reschedule_running_after(id, ready.payload, delay_ms, deadline)); @@ -442,16 +441,12 @@ mod tests { scheduler.cancel(cancelled); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("kept timer should be ready"); assert_eq!(ready.id, kept); assert_eq!(ready.payload, "kept"); scheduler.finish_running(ready.id); - assert!( - scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) - .is_none() - ); + assert!(scheduler.take_next_ready(now).is_none()); } #[test] @@ -461,7 +456,7 @@ mod tests { let interval = scheduler.schedule_after("tick", 0, now); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("interval should be ready"); scheduler.cancel(interval); assert!(!scheduler.reschedule_running_after( @@ -493,7 +488,7 @@ mod tests { assert_eq!(scheduler.pending_count(), 1); let ready = scheduler - .take_next_ready(now + Duration::from_millis(12), TimerReadyAllowance::NONE) + .take_next_ready(now + Duration::from_millis(12)) .expect("kept timer should become ready"); assert_eq!(ready.id, kept); scheduler.finish_running(ready.id); @@ -509,40 +504,40 @@ mod tests { scheduler.cancel(cancelled); assert_eq!( - scheduler - .next_ready_deadline_matching(now, TimerReadyAllowance::NONE, |payload| *payload - == "selected",), + scheduler.next_ready_deadline_matching(now, |payload| *payload == "selected",), None ); assert_eq!( - scheduler.next_ready_deadline_matching( - now + Duration::from_millis(1), - TimerReadyAllowance::NONE, - |payload| *payload == "selected", - ), + scheduler + .next_ready_deadline_matching(now + Duration::from_millis(1), |payload| *payload + == "selected",), Some(now + Duration::from_millis(1)) ); } #[test] - fn matching_ready_deadline_respects_an_earlier_non_ready_barrier() { + fn matching_ready_deadline_does_not_admit_a_future_timer() { let now = Instant::now(); let mut scheduler = TimerScheduler::default(); - scheduler.schedule_after("barrier", 2, now); + scheduler.schedule_after("earlier", 2, now); scheduler.schedule_after("selected", 1, now + Duration::from_micros(1_500)); - let allowance = TimerReadyAllowance { - max_delay_ms: 1, - allowance: Duration::from_millis(1), - }; - + let before_selected = now + Duration::from_micros(2_499); + assert!(scheduler.has_ready_timer(before_selected)); assert_eq!( - scheduler.next_ready_deadline_matching( - now + Duration::from_micros(1_500), - allowance, - |payload| *payload == "selected", - ), - None, - "a later timer admitted by early allowance must not overtake the heap head" + scheduler + .next_ready_deadline_matching(before_selected, |payload| *payload == "selected"), + None + ); + assert!( + scheduler + .take_next_ready_matching(before_selected, |payload| *payload == "selected") + .is_none() + ); + assert_eq!( + scheduler.next_ready_deadline_matching(now + Duration::from_micros(2_500), |payload| { + *payload == "selected" + },), + Some(now + Duration::from_micros(2_500)) ); } @@ -564,25 +559,25 @@ mod tests { } #[test] - fn early_allowance_only_applies_to_short_delays() { + fn rescheduled_short_timer_waits_for_its_new_deadline() { let now = Instant::now(); let mut scheduler = TimerScheduler::default(); - let short = scheduler.schedule_after("short", 1, now); - scheduler.schedule_after("long", 2, now); + let id = scheduler.schedule_after("tick", 1, now); + let first_deadline = now + Duration::from_millis(1); + let ready = scheduler.take_next_ready(first_deadline).unwrap(); + assert!(scheduler.reschedule_running_after(id, ready.payload, 1, first_deadline)); - let allowance = TimerReadyAllowance { - max_delay_ms: 1, - allowance: Duration::from_millis(1), - }; - let just_before = now + Duration::from_micros(500); - assert!(scheduler.has_ready_timer(just_before, allowance)); - let ready = scheduler - .take_next_ready(just_before, allowance) - .expect("short timer should be ready within allowance"); - assert_eq!(ready.id, short); - scheduler.finish_running(ready.id); - - assert!(!scheduler.has_ready_timer(just_before, allowance)); + let next_deadline = first_deadline + Duration::from_millis(1); + for before_deadline in [first_deadline, next_deadline - Duration::from_nanos(1)] { + assert!(!scheduler.has_ready_timer(before_deadline)); + assert!(scheduler.take_next_ready(before_deadline).is_none()); + assert_eq!(scheduler.next_deadline(), Some(next_deadline)); + } + let ready = scheduler.take_next_ready(next_deadline).unwrap(); + assert_eq!(ready.id, id); + assert_eq!(ready.payload, "tick"); + scheduler.finish_running(id); + assert_eq!(scheduler.pending_count(), 0); } #[test] @@ -593,29 +588,23 @@ mod tests { let selected = scheduler.schedule_after("selected", 0, now); let second = scheduler.schedule_after("second", 0, now); - assert!( - scheduler.has_ready_matching(now, TimerReadyAllowance::NONE, |payload| { - *payload == "selected" - }) - ); + assert!(scheduler.has_ready_matching(now, |payload| { *payload == "selected" })); let ready = scheduler - .take_next_ready_matching(now, TimerReadyAllowance::NONE, |payload| { - *payload == "selected" - }) + .take_next_ready_matching(now, |payload| *payload == "selected") .expect("selected timer should be ready"); assert_eq!(ready.id, selected); assert_eq!(ready.payload, "selected"); scheduler.finish_running(ready.id); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("first timer should remain pending"); assert_eq!(ready.id, first); assert_eq!(ready.payload, "first"); scheduler.finish_running(ready.id); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("second timer should remain pending"); assert_eq!(ready.id, second); assert_eq!(ready.payload, "second"); @@ -633,16 +622,14 @@ mod tests { let selected = scheduler.schedule_after("selected", 0, now); let ready = scheduler - .take_next_ready_matching(now, TimerReadyAllowance::NONE, |payload| { - *payload == "selected" - }) + .take_next_ready_matching(now, |payload| *payload == "selected") .expect("selected timer should be ready after skipped timers"); assert_eq!(ready.id, selected); assert_eq!(ready.payload, "selected"); scheduler.finish_running(ready.id); let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("first timer should remain first after matching drain"); assert_eq!(ready.id, first); assert_eq!(ready.payload, "first"); @@ -650,7 +637,7 @@ mod tests { for skipped_id in skipped { let ready = scheduler - .take_next_ready(now, TimerReadyAllowance::NONE) + .take_next_ready(now) .expect("skipped timer should remain pending"); assert_eq!(ready.id, skipped_id); assert_eq!(ready.payload, "skipped");