Schedule timers with a NetworkDuration instead of whole milliseconds - #115
Schedule timers with a NetworkDuration instead of whole milliseconds#115rnro wants to merge 1 commit into
NetworkDuration instead of whole milliseconds#115Conversation
`NetworkContext.Scheduler.schedule` took a count of milliseconds, so every caller computing a deadline at nanosecond resolution had to truncate on the way in. A delay under a millisecond truncated to no delay at all, arming a wakeup that could not reach the deadline it was armed for: the handler ran early, recomputed the same delay and armed again, spinning until enough real time had passed. `Timer.recalculate`, the bridge's link delay and `scheduleTimer(duration:)` all fed it. * `Scheduler.schedule` takes an `after` delay, and `FutureTime` carries a duration, so the truncation has nowhere left to happen. * `scheduleWakeup` threads the duration through `TimerSchedulable` and `ProtocolInstanceReference` to the context, replacing four `UInt64(_.milliseconds)` conversions. * `DefaultScheduler` builds its deadline from whole seconds plus a remainder, so neither part can overrun the `Int` that `DispatchTimeInterval` takes, which is 32 bits on 32-bit watchOS. * A recording `Scheduler` lets `testContextTimerKeepsASubMillisecondDelay` assert the delay a caller asked for, which needs no clock and no waiting. Source-breaking for anything outside the package conforming to `Scheduler`.
| let nanoseconds = max(delay.nanoseconds, 0) | ||
| let targetTime = | ||
| DispatchTime.now() | ||
| + .seconds(Int(clamping: nanoseconds / 1_000_000_000)) |
There was a problem hiding this comment.
Why does this split up the seconds and nanoseconds?
There was a problem hiding this comment.
If we put it all in at nanoseconds then we'd easily overrun 32-bit Int max (I think it's ~2.7s). Splitting it up this way means we can express bigger timer durations.
There was a problem hiding this comment.
32-bit Int max milliseconds in the old code was probably long enough to not cause an issue in practice.
There was a problem hiding this comment.
Ah I see this is because of going through DispatchTimeInterval. Makes sense now!
| /// | ||
| /// The `milliseconds` parameter specifies the delay before the task runs. | ||
| func schedule(_ task: @escaping (() -> Void), milliseconds: Int64, reference: TimerReference) | ||
| func schedule(_ task: @escaping (() -> Void), after delay: NetworkDuration, reference: TimerReference) |
There was a problem hiding this comment.
I think you marked this with the corresponding label but this will force a change here:
https://github.com/apple/swift-nio-quic/blob/main/Sources/NIOQUIC/SwiftNetwork/QUICChannelEventLoop.swift#L60
There was a problem hiding this comment.
Yes, I believe that Tommy either has or is planning API-breaking changes so getting them all in the same release can be good for adopters.
There was a problem hiding this comment.
Yeah this will be breaking
| case .milliseconds(let milliseconds, let block): | ||
| scheduler.schedule(block, milliseconds: Int64(milliseconds), reference: reference) | ||
| case .after(let delay, let block): | ||
| scheduler.schedule(block, after: delay, reference: reference) |
There was a problem hiding this comment.
I am guessing its a wash, but just to make sure can you profile QUICTransfer with CPU trace to make sure that since we are operating on an object now that we did not incur a spike in CPU?
There was a problem hiding this comment.
From what I can see the only difference is that the construction of the targetTime now creates two DispatchTimeIntervals, one for the seconds and one for the nanoseconds. We could have the code branch in the case that there are 0 seconds, but I think this difference is in the noise anyway.
| let nanoseconds = max(delay.nanoseconds, 0) | ||
| let targetTime = | ||
| DispatchTime.now() | ||
| + .seconds(Int(clamping: nanoseconds / 1_000_000_000)) |
There was a problem hiding this comment.
Ah I see this is because of going through DispatchTimeInterval. Makes sense now!
| /// | ||
| /// The `milliseconds` parameter specifies the delay before the task runs. | ||
| func schedule(_ task: @escaping (() -> Void), milliseconds: Int64, reference: TimerReference) | ||
| func schedule(_ task: @escaping (() -> Void), after delay: NetworkDuration, reference: TimerReference) |
There was a problem hiding this comment.
Yeah this will be breaking
| func schedule(_ task: @escaping (() -> Void), after delay: NetworkDuration, reference: TimerReference) { | ||
| let nanoseconds = max(delay.nanoseconds, 0) | ||
| let targetTime = | ||
| DispatchTime.now() |
There was a problem hiding this comment.
Shouldn't we be using System.Time.now?
There was a problem hiding this comment.
Possibly, it looks fishy here because the deadlines use system time but that is a broader change than this PR. DispatchTime.now() was used in the existing code and timerList.insert(targetTime: requires a DispatchTime.
4 participants
NetworkContext.Scheduler.scheduletook a count of milliseconds, so every caller computing a deadline at nanosecond resolution had to truncate on the way in. A delay under a millisecond truncated to no delay at all, arming a wakeup that could not reach the deadline it was armed for: the handler ran early, recomputed the same delay and armed again, spinning until enough real time had passed.Timer.recalculate, the bridge's link delay andscheduleTimer(duration:)all fed it.Scheduler.scheduletakes anafterdelay, andFutureTimecarries a duration, so the truncation has nowhere left to happen.scheduleWakeupthreads the duration throughTimerSchedulableandProtocolInstanceReferenceto the context, replacing fourUInt64(_.milliseconds)conversions.DefaultSchedulerbuilds its deadline from whole seconds plus a remainder, so neither part can overrun theIntthatDispatchTimeIntervaltakes, which is 32 bits on 32-bit watchOS.SchedulerletstestContextTimerKeepsASubMillisecondDelayassert the delay a caller asked for.