Conversation
Datapath code read `NetworkClock.Instant.now` directly, so no test could control it: the value came from `mach_continuous_time` however the code was driven. The scheduler already owns the timers, so it is the natural owner of time as well; one that holds scheduled tasks rather than arming an OS timer has to report the instant it fires them at. * Added `now` and `nowAbsolute` to `NetworkContext.Scheduler`, and to `DefaultScheduler` as the one conformance allowed to read the real clock. * Renamed `NetworkClock.Instant.now` to `package systemNow`, and `nowAbsolute` to `systemNowAbsolute`, so a direct read is visible at the call site rather than looking ordinary. * Routed the library's remaining reads through `context.scheduler` or the instant the caller already holds. * Passed a scheduler to `QLog`, whose start time and two event timestamps had nowhere else to get a clock from. * Left `QUICStreamLoad` reading the system clock, since a benchmark measuring wall-clock duration wants exactly that. Source-breaking for anything outside the package that conforms to `Scheduler`.
`Pacer.getSendTime` runs once per paced packet and read both clocks on every call, converting between the domains by subtracting one reading from the other. Two reads per packet, and a difference that picks up the gap between them as though it were domain offset. * Added `currentAbsoluteTimestamp` beside the existing batch timestamp, stamped and cleared with it at both the inbound and outbound boundaries. * Added `QUICConnection.nowAbsolute`, falling back to the scheduler when no batch is in flight. * Pointed `Pacer` at the pair, so inside a batch it reads no clock at all. The offset error was the read gap, well under the pacing interval, so this is a hot-path saving rather than a fix. `Pacer` already went through the scheduler, so it was not blocking clock injection.
`NetworkClock.Instant` carried its own manual clock behind `NETWORK_INTERNAL_TESTS`: a static box that `useManualTime` froze and `advanceManualTime` moved, checked on every read of the clock. Being process-global, it could not be scoped to one connection or one test, and the two readers paid for a branch on a hot path to consult it. The scheduler now provides the same control per context, so this is a replacement rather than a removal. * Removed `useSystemTime`, `useManualTime`, `advanceManualTime` and the backing store, and with them the branch each clock read carried. * Removed `SwiftNetworkManualClockTests`, the only caller. The 30 tests in `SwiftNetworkClockTests` alongside it cover `NetworkDuration` and `Instant` arithmetic and are untouched. * Dropped `-DNETWORK_INTERNAL_TESTS` from both CI scripts and the README, since nothing is behind it any more.
rnro
requested review from
ekinnear,
kkuk24,
rpaulo and
tfpauly
as code owners
September 14, 2026 18:45
rnro
force-pushed
the
clock-reads-through-the-scheduler
branch
from
September 14, 2026 18:47
7aa378c to
44f410f
Compare
* Added `NetworkContext.now` and `nowAbsolute` beside `resetTimer`, forwarding to the scheduler.. * Routed the eight clock reads in `QUICConnection` and `IPProtocol` through the context, which is also one member shorter at each site. * Changed `QLog` to hold the context rather than an `any NetworkContext.Scheduler`, of which that build has no instance to hand it.
rpaulo
reviewed
Sep 15, 2026
| } | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Shouldn't we have a test that shows how we can manually advance the clock?
Contributor
Author
There was a problem hiding this comment.
Good point, thanks! This was split out of some larger work which exercised this code in tests but it should stand alone.
Contributor
|
Doesn't this make it more difficult to test smaller objects that don't have a NetworkContext to begin with? How would we test congestion control now? Does that require us passing a NetworkContext to the congestion controller? |
agnosticdev
approved these changes
Sep 15, 2026
agnosticdev
left a comment
agnosticdev
left a comment
Collaborator
There was a problem hiding this comment.
Thank you for checking the performance results on this change!
Contributor
Author
I don't think so. We haven't had to add a context to parts of the codebase which have no interest in the context in this PR and the tests still pass for example. Because we have threaded |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change threads clock reads through the scheduler and exposes new methods for reading the system time with names which clearly communicate that is what is happening.
This PR is probably best reviewed per-commit. The first shifts the reads to go through the scheduler, the second modifies
Pacer.getSendTimeto more efficiently use the new APIs and the third removes the process-global manual clock since matching control is now offered per-context.Motivation
Datapath code read
NetworkClock.Instant.nowdirectly, so no test could control/mock time. This makes tests slow and in many cases racey.Description
The scheduler already owns the timers, so it is the natural owner of time as well.
NetworkContext.Schedulergains now andnowAbsolute, withDefaultSchedulerthe one conformance allowed to read the real clock.NetworkClock.Instant.nowbecomes package-visiblesystemNow, so a syscall read is more visible/intentional at the call site.QUICConnectionstamps the continuous and absolute clocks as a pair per batch, soPacerconverts between the domains without reading either - it previously read both once per paced packet.NETWORK_INTERNAL_TESTSis removed. The scheduler gives the same control per context, so this replaces it rather than dropping it;NETWORK_INTERNAL_TESTSis gone from CI and the README.Performance
This change should be a marginal improvement but certainly not negative.
In test runs of
QUICTransferit seems to shave a few mega cycles, but hard to say with the spread: