-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(s2n-quic-core): add Cached clock implementation #2133
Conversation
108a74f
to
377c52e
Compare
377c52e
to
cc8d04f
Compare
impl<'a, C: Clock> Clock for Cached<'a, C> { | ||
#[inline] | ||
fn get_time(&self) -> Timestamp { | ||
if let Some(time) = self.cached_value.get() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this cache doesn't need a TTL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this borrows an inner clock for 'a
, the lifetime of the borrow is the TTL, if that makes sense. Its intended use is something like this:
{
let clock = clock.cached();
for packet in ack_packets {
packet.ack_time = clock.get_time();
}
}
{
let clock = clock.cached();
for packet in lost_packets {
packet.lost_time = clock.get_time();
}
}
In this example, we'll only query the cache at most twice, regardless of how many packets are in the two iterators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got a few ad-hoc examples of doing this in the codebase than I'm going to clean up after this is merged:
s2n-quic/quic/s2n-quic-transport/src/endpoint/mod.rs
Lines 103 to 112 in e2f32e5
let mut now: Option<Timestamp> = None; | |
queue.for_each(|mut header, payload| { | |
let timestamp = match now { | |
Some(time) => time, | |
None => { | |
now = Some(clock.get_time()); | |
now.expect("value should be set") | |
} | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah nice. Would it be possible to a add a debug_assertion or something to prevent misuse? Something like if the difference between the cache and now is > x milliseconds, panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem is what would the x
be without triggering false positives... :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok to merge this though for now, though if its mainly used in pretty tight loops I think we could start with something like 1 or 2 ms
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's way too low, especially in debug mode. I was thinking 1s+. But yeah we can figure something else later. I'm not super worried about it being misused since it borrows a clock rather than owning it so it'd really only live as long as a stack frame.
Description of changes:
This adds a mechanism to cache the clock value after the first call. This can reduce the number of times we read from the system clock, where we don't care about high precision between events.
Call-outs:
I've also added a timer debugger that you can query all of the locations in the timer tree that print where they are and if they're armed or not.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.