quiche: implement QuicClock interface #7028
Conversation
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
|
/assign @wu-bin |
|
|
||
| class EnvoyQuicClock : public quic::QuicClock { | ||
| public: | ||
| EnvoyQuicClock(Event::TimeSystem& time_system) : quic::QuicClock(), time_system_(time_system) {} |
There was a problem hiding this comment.
quic::QuicClock() is not needed, it is called by default.
| } | ||
|
|
||
| quic::QuicWallTime EnvoyQuicClock::WallNow() const { | ||
| return quic::QuicWallTime::FromUNIXMicroseconds(timePointToInt64(time_system_.systemTime())); |
There was a problem hiding this comment.
The standard says std::chrono::system_clock, the source of systemTime(), has a unspecified epoch, although
- Most implementations already uses Unix epoch.
- It will be Unix epoch from c++ 20.
So I guess this is fine in practice?
There was a problem hiding this comment.
This is also the system time used by Envoy, so I guess it's okay. Plus Quic doesn't need to know the absolute time anyway.
| quic::QuicWallTime WallNow() const override; | ||
|
|
||
| private: | ||
| template <typename T> int64_t timePointToInt64(std::chrono::time_point<T> time) const { |
There was a problem hiding this comment.
nit: Suggest to rename to "MicrosecondsSinceEpoch".
Signed-off-by: Dan Zhang <danzh@google.com>
QuicClock is part of quic API. So I think put EnvoyQuicClock under platform is fine too. |
|
|
||
| quic::QuicTime EnvoyQuicClock::Now() const { | ||
| return quic::QuicTime::Zero() + quic::QuicTime::Delta::FromMicroseconds( | ||
| microsecondsSinceEpoch(time_system_.monotonicTime())); |
There was a problem hiding this comment.
Sorry, it looked like there is an empty line between line 12 and 13, but it's not.
|
/assign @alyssawilk |
|
|
||
| quic::QuicTime EnvoyQuicClock::ApproximateNow() const { | ||
| // This might be expensive as Dispatcher doesn't store approximate time_point. | ||
| return Now(); |
There was a problem hiding this comment.
worst case we can have a thread local alarm which latches time once per dispatcher loop.
There was a problem hiding this comment.
LibeventScheduler::onPrepare() is called each libevent loop. But there are some plumbing to pass the timestamp get from onPrepare() to TimeSystem. But it's doable, and can be an future improvement.
| Event::TestRealTimeSystem time_system; | ||
| EnvoyQuicClock clock(time_system); | ||
| quic::QuicTime last_now = clock.Now(); | ||
| for (int i = 0; i < 1e5; ++i) { |
There was a problem hiding this comment.
how long does this take? I'd think that lowering would be OK given many CI runs
There was a problem hiding this comment.
//test/extensions/quic_listeners/quiche/platform:envoy_quic_clock_test PASSED in 0.8s
Is this ok?
There was a problem hiding this comment.
I'd still prefer to lower it - it's not a huge deal but it'll be longer running on asan and tsan and I think the gains are minimal.
There was a problem hiding this comment.
reduced it to 1000.
|
/retest |
|
🔨 rebuilding |
|
/retest |
|
🔨 rebuilding |
| Event::TestRealTimeSystem time_system; | ||
| EnvoyQuicClock clock(time_system); | ||
| quic::QuicTime last_now = clock.Now(); | ||
| for (int i = 0; i < 1e5; ++i) { |
There was a problem hiding this comment.
I'd still prefer to lower it - it's not a huge deal but it'll be longer running on asan and tsan and I think the gains are minimal.
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Implement QuicClock using Event::TimeSystem.
Currently implement ApproximateNow() as Now() because TimeSystem doesn't support approximate time.
Risk Level: low, not in use
Testing: added test/extentions/quic_listeners/quiche/platform:envoy_quic_clock_test
Part of #2557