quiche: implement QuicClock interface by danzh2010 · Pull Request #7028 · envoyproxy/envoy

danzh2010 · 2019-05-21T20:57:55Z

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

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-05-21T20:58:12Z

/assign @wu-bin

wu-bin

LGTM minus some nits. Also, should we find a better place for this library? Maybe directly under source/extensions/quic_listeners, along with other integration code?

wu-bin · 2019-05-22T12:05:40Z

source/extensions/quic_listeners/quiche/platform/envoy_quic_clock.h

+
+class EnvoyQuicClock : public quic::QuicClock {
+public:
+  EnvoyQuicClock(Event::TimeSystem& time_system) : quic::QuicClock(), time_system_(time_system) {}


quic::QuicClock() is not needed, it is called by default.

wu-bin · 2019-05-22T12:34:33Z

source/extensions/quic_listeners/quiche/platform/envoy_quic_clock.cc

+}
+
+quic::QuicWallTime EnvoyQuicClock::WallNow() const {
+  return quic::QuicWallTime::FromUNIXMicroseconds(timePointToInt64(time_system_.systemTime()));


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?

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.

wu-bin · 2019-05-22T12:36:57Z

source/extensions/quic_listeners/quiche/platform/envoy_quic_clock.h

+  quic::QuicWallTime WallNow() const override;
+
+private:
+  template <typename T> int64_t timePointToInt64(std::chrono::time_point<T> time) const {


nit: Suggest to rename to "MicrosecondsSinceEpoch".

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 · 2019-05-22T16:12:25Z

LGTM minus some nits. Also, should we find a better place for this library? Maybe directly under source/extensions/quic_listeners, along with other integration code?

QuicClock is part of quic API. So I think put EnvoyQuicClock under platform is fine too.

wu-bin · 2019-05-22T17:00:22Z

source/extensions/quic_listeners/quiche/platform/envoy_quic_clock.cc

+
+quic::QuicTime EnvoyQuicClock::Now() const {
+  return quic::QuicTime::Zero() + quic::QuicTime::Delta::FromMicroseconds(
+                                      microsecondsSinceEpoch(time_system_.monotonicTime()));


nit: remove empty line.

which line?

Sorry, it looked like there is an empty line between line 12 and 13, but it's not.

danzh2010 · 2019-05-23T19:23:52Z

/assign @alyssawilk

alyssawilk · 2019-05-23T19:41:06Z

source/extensions/quic_listeners/quiche/platform/envoy_quic_clock.cc

+
+quic::QuicTime EnvoyQuicClock::ApproximateNow() const {
+  // This might be expensive as Dispatcher doesn't store approximate time_point.
+  return Now();


worst case we can have a thread local alarm which latches time once per dispatcher loop.

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.

alyssawilk · 2019-05-23T19:42:34Z

test/extensions/quic_listeners/quiche/platform/envoy_quic_clock_test.cc

+  Event::TestRealTimeSystem time_system;
+  EnvoyQuicClock clock(time_system);
+  quic::QuicTime last_now = clock.Now();
+  for (int i = 0; i < 1e5; ++i) {


how long does this take? I'd think that lowering would be OK given many CI runs

//test/extensions/quic_listeners/quiche/platform:envoy_quic_clock_test PASSED in 0.8s
Is this ok?

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.

reduced it to 1000.

danzh2010 · 2019-05-23T21:45:23Z

/retest

repokitteh-read-only · 2019-05-23T21:45:28Z

🔨 rebuilding ci/circleci: docs (failed build)

🐱

Caused by: a #7028 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2019-05-24T16:30:35Z

/retest

repokitteh-read-only · 2019-05-24T16:30:39Z

🔨 rebuilding ci/circleci: docs (failed build)

🐱

Caused by: a #7028 (comment) was created by @danzh2010.

see: more, trace.

alyssawilk

If you do a master merge I think you'll pick up the fix to the docs build.

alyssawilk · 2019-05-28T14:08:34Z

test/extensions/quic_listeners/quiche/platform/envoy_quic_clock_test.cc

+  Event::TestRealTimeSystem time_system;
+  EnvoyQuicClock clock(time_system);
+  quic::QuicTime last_now = clock.Now();
+  for (int i = 0; i < 1e5; ++i) {


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>

danzh1989 added 3 commits May 17, 2019 17:16

implement with TimeSource

2680577

Signed-off-by: Dan Zhang <danzh@google.com>

Merge branch 'master' into envoyclock

05f8011

Signed-off-by: Dan Zhang <danzh@google.com>

add test

17802f8

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 requested review from alyssawilk and mattklein123 as code owners May 21, 2019 20:57

repokitteh-read-only bot assigned wu-bin May 21, 2019

wu-bin previously approved these changes May 22, 2019

View reviewed changes

rename microsecondsSinceEpoch

d6af84a

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 dismissed wu-bin’s stale review via d6af84a May 22, 2019 16:11

wu-bin previously approved these changes May 22, 2019

View reviewed changes

repokitteh-read-only bot assigned alyssawilk May 23, 2019

alyssawilk reviewed May 23, 2019

View reviewed changes

danzh2010 dismissed wu-bin’s stale review via b16dd70 May 23, 2019 20:47

alyssawilk reviewed May 28, 2019

View reviewed changes

alyssawilk added the waiting:any label May 28, 2019

danzh1989 added 2 commits May 28, 2019 13:13

Merge branch 'master' into envoyclock

b33d2ac

Signed-off-by: Dan Zhang <danzh@google.com>

reduce test loop

4d5fccb

Signed-off-by: Dan Zhang <danzh@google.com>

danzh2010 force-pushed the envoyclock branch from b16dd70 to 4d5fccb Compare May 28, 2019 17:22

repokitteh-read-only bot removed the waiting:any label May 28, 2019

alyssawilk approved these changes May 28, 2019

View reviewed changes

htuch merged commit 47d3a52 into envoyproxy:master May 28, 2019

danzh2010 mentioned this pull request May 29, 2019

quiche:: Implement QuicAlarm interface #7094

Merged

Conversation

danzh2010 commented May 21, 2019

Uh oh!

danzh2010 commented May 21, 2019

Uh oh!

wu-bin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented May 22, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented May 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danzh2010 commented May 23, 2019

Uh oh!

repokitteh-read-only bot commented May 23, 2019

Uh oh!

danzh2010 commented May 24, 2019

Uh oh!

repokitteh-read-only bot commented May 24, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants