Thread 'now' through the stack - #116
Conversation
Each of them read `NetworkClock.Instant.now` where it needed a time, so one inbound batch sampled the clock repeatedly and figures that should have agreed did not. The `= .now` defaults on `Timer` and `Recovery.findLostPacket` are the worst of it: they let a caller take a time without saying which one it meant, and loss detection ended up comparing recorded deadlines against an instant sampled later than the one its timer fired at. `CongestionControlProtocol` and its three implementations now take `now`, `QUICPath` supplies the instant `QUICConnection` already caches for the batch, and the defaults are gone so a caller has to name what it means. `NetworkClock.Instant.testBase` and two `Ack` conveniences over it keep the ACK-bookkeeping callsites free of `now:` noise. `IPProtocol.processInbound` resolves one instant per inbound batch, lazily, so a stack that never asks for receive timestamps pays nothing. This changes what a receive timestamp means: the frames of a batch share one instant where each used to carry its own, so inter-arrival within a batch now reads zero. `CubicTests.testCubicCongestionLimited` and its Prague twin assert an exact congestion window. Seeded from the system clock they had dated send times into their own future, so every loss opened a new recovery period instead of one per round.
The `timestamp: NetworkClock.Instant = .now` defaults stamped an entry when it was written rather than when the thing it describes happened, so entries could be ordered differently from the events. The defaults are gone from the methods whose callers hold an instant, and `Recovery` and `QUICConnection` pass theirs. `congestionControlUpdated` and `logCongestionStateUpdated` keep theirs. Their callers do not hold an instant, and the ordering is not worth threading one down to them; what these two want is a clock to default from rather than an argument.
`Timer.timerFired(timeNow:)` compares each deadline against the instant it was given, but called its closures with no argument, so every closure read the clock again. The ACK, recovery and PMTUD handlers therefore ran against an instant strictly later than the one that decided they were due. `TimerEntry.closure` now takes a `NetworkClock.Instant` and `timerFired` passes `timeNow` rather than the threshold-slackened value it compares against. The migration, keepalive, idle and draining handlers ignore it, since none of them measures against a deadline.
agnosticdev
left a comment
agnosticdev
left a comment
There was a problem hiding this comment.
Given that NetworkClock.Instant is a copyable struct, and that you are passing this through functions in the IP data path can you profile QUICTransfer with and without this change to make sure we are not adding extra CPU calls for outlined copy of NetworkClock.Instant and then outlined destroy of NetworkClock.Instant? We'll want to make sure of that before proceeding here.
There was a problem hiding this comment.
Since this in the QUICTests directory, does the file name need "QUIC"?
There was a problem hiding this comment.
Good point, I've dropped the QUIC
| /// the real clock makes every duration depend on how long the test itself took to run. | ||
| /// | ||
| /// Non-zero because much of the stack treats `.zero` as "unset". | ||
| static var testBase: NetworkClock.Instant { |
There was a problem hiding this comment.
How about we re-use the value from the other test?
final class SwiftNetworkManualClockTests: NetTestCase {
private let base = NetworkClock.Instant(milliseconds: 1000)
There was a problem hiding this comment.
If we did that we'd have to introduce a dependency across test targets or create a new common re-usable target and I'm not sure that's worth the benefit.
There was a problem hiding this comment.
Why not put it under NETWORK_INTERNAL_TESTS in NetworkClock?
There was a problem hiding this comment.
I personally don't think it's good practice to put this in the Source tree. Having it accessible from production code pollutes the namespace and creates a footgun. I'm also hoping to be able to remove NETWORK_INTERNAL_TESTS in the future if we have a fully injectable timing mechanism.
| /// and not about time at all, so restating `now:` several hundred times would be noise. | ||
| /// The default here is a *fixed* instant, so those tests stay deterministic. A test | ||
| /// whose premise is timing calls the full form and passes its own instant. | ||
| @available(Network 0.1.0, *) |
There was a problem hiding this comment.
Shouldn't this be in AckTests?
I was pretty sure that the compiler would just optimize through such a trivial object but to check I attempted to solve this by inspecting the binary. I ran |
What does it say when you run the benchmark though in Instruments? |
I think any change if one exists is within the noise |
25fcef8 to
f29e5da
Compare
3 participants
This PR's primary goal is to lay the groundwork for making a clock/time injectable: while datapath code reads
NetworkClock.Instant.nowdirectly, no test can control it. This threads the instant through from the caller instead, so a later change can supply it, e.g. from a scheduler.Whilst doing this work I discovered a couple of potential issues/improvements.
Sampling the clock wherever a time was needed meant one inbound batch read it repeatedly, and loss detection compared recorded deadlines against an instant sampled later than the one its timer fired at.
Changes:
nowfrom their caller;QUICPathsupplies the instantQUICConnectionalready caches for the batch.QLogevents take the timestamp of the event rather than of the write.Timerclosures receive the instant the timer fired instead of reading the clock again.Two behavior changes:
@autoclosurekeeps the read out of stacks that never ask for timestamps.CubicTests.testCubicCongestionLimitedand its Prague analogue now seed from a fixed instant. From the system clock they had send times in their own future, so every loss opened a new recovery period instead of one per round.A few
NetworkClock.Instant.nowreads remain; those are where a scheduler-supplied clock would hook in.