Use clock_gettime instead of base::TimeTicks::Now() in QUIC

Use clock_gettime instead of base::TimeTicks::Now() in QUIC

Review-Url: https://codereview.chromium.org/2871573009
Cr-Commit-Position: refs/heads/master@{#471434}
Committed: https://chromium.googlesource.com/chromium/src/+/ac0ea495b772c2b91ff94427b5a1cb6646cc4daf

[Ryan Hamilton, 2017-05-09 22:58:37]

The CQ bit was checked by rch@chromium.org to run a CQ dry run

[Ryan Hamilton, 2017-05-09 22:58:59]

rch@chromium.org changed reviewers:
+ ianswett@chromium.org, zhongyi@chromium.org

[Ryan Hamilton, 2017-05-09 22:59:00]

ianswett: FYI
zhongyi: PTAL

[commit-bot: I haz the power, 2017-05-09 22:59:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Zhongyi Shi, 2017-05-09 23:30:57]

Generally looks good to me, just one question.

https://codereview.chromium.org/2871573009/diff/20001/net/quic/platform/impl/...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/20001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:8: // Use clock_gettime on iOS>
IIRC, clock_gettime is supported and works fine on iOS 10 and MacOs, but crashes
iOS 9 according to the email thread? Do we need to get the iOS version when
using clock_gettime?

[commit-bot: I haz the power, 2017-05-10 00:10:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 00:10:55]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[ianswett, 2017-05-10 00:44:56]

Looks good, minus the version check.

https://codereview.chromium.org/2871573009/diff/20001/net/quic/platform/impl/...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/20001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:8: // Use clock_gettime on iOS>
On 2017/05/09 23:30:57, Zhongyi Shi wrote:
> IIRC, clock_gettime is supported and works fine on iOS 10 and MacOs, but
crashes
> iOS 9 according to the email thread? Do we need to get the iOS version when
> using clock_gettime?

+1

[Ryan Hamilton, 2017-05-10 03:06:04]

Thanks!

https://codereview.chromium.org/2871573009/diff/20001/net/quic/platform/impl/...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/20001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:8: // Use clock_gettime on iOS>
On 2017/05/10 00:44:56, ianswett wrote:
> On 2017/05/09 23:30:57, Zhongyi Shi wrote:
> > IIRC, clock_gettime is supported and works fine on iOS 10 and MacOs, but
> crashes
> > iOS 9 according to the email thread? Do we need to get the iOS version when
> > using clock_gettime?
> 
> +1

*facepalm*

Done.

[Ryan Hamilton, 2017-05-10 03:06:33]

Patchset #3 (id:40001) has been deleted

[Ryan Hamilton, 2017-05-10 03:09:14]

Patchset #3 (id:60001) has been deleted

[Ryan Hamilton, 2017-05-10 03:09:22]

The CQ bit was checked by rch@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 03:10:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 07:01:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 07:01:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ianswett, 2017-05-10 10:44:13]

lgtm

Awesome, one possible nit.

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:8: #include <time.h>
nit: Should this be indented, like it is below?

[ianswett, 2017-05-10 11:40:36]

Is it worth considering changing base/time/time_mac.cc?

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:39: return
CreateTimeFromMicroseconds(base::TimeTicks::Now().ToInternalValue());
As mentioned in the thread, Ideally we'd update base/time/time_mac.cc to
optimize other calls, such as the call in
NetworkActivityMonitor::IncrementBytesReceived

[Ryan Hamilton, 2017-05-10 13:17:27]

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:8: #include <time.h>
On 2017/05/10 10:44:13, ianswett wrote:
> nit: Should this be indented, like it is below?

I don't think so, because #includes are not indented.

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:39: return
CreateTimeFromMicroseconds(base::TimeTicks::Now().ToInternalValue());
On 2017/05/10 11:40:36, ianswett wrote:
> As mentioned in the thread, Ideally we'd update base/time/time_mac.cc to
> optimize other calls, such as the call in
> NetworkActivityMonitor::IncrementBytesReceived

That's beyond the scope of this CL, but I'll talk with base OWNERS about this.

[Ryan Hamilton, 2017-05-10 13:17:38]

The CQ bit was checked by rch@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 13:18:06]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 14:20:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 14:20:33]

Dry run: This issue passed the CQ dry run.

[kapishnikov, 2017-05-10 15:25:37]

kapishnikov@chromium.org changed reviewers:
+ kapishnikov@chromium.org

[kapishnikov, 2017-05-10 15:25:38]

Sorry for the late input.

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:36: return
CreateTimeFromMicroseconds(tp.tv_nsec / 1000);
Should it be:
return CreateTimeFromMicroseconds(tp.tv_sec * 1000000 + tp.tv_nsec / 1000)

Otherwise, the time will reset every second.

[Ryan Hamilton, 2017-05-10 17:01:55]

The CQ bit was checked by rch@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 17:03:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Hamilton, 2017-05-10 17:05:56]

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/80001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:36: return
CreateTimeFromMicroseconds(tp.tv_nsec / 1000);
On 2017/05/10 15:25:38, kapishnikov wrote:
> Should it be:
> return CreateTimeFromMicroseconds(tp.tv_sec * 1000000 + tp.tv_nsec / 1000)
> 
> Otherwise, the time will reset every second.

Done.

[kapishnikov, 2017-05-10 17:23:16]

An informal LGTM from me. As it was mentioned earlier, it would be great if we
could maka a similar change in
https://cs.chromium.org/chromium/src/base/time/time_mac.cc?type=cs&l=55.

[Zhongyi Shi, 2017-05-10 17:39:14]

lgtm!

[kapishnikov, 2017-05-10 18:10:12]

https://codereview.chromium.org/2871573009/diff/100001/net/quic/platform/impl...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/100001/net/quic/platform/impl...
net/quic/platform/impl/quic_chromium_clock.cc:35: clock_gettime(CLOCK_MONOTONIC,
&tp);
One extra comment. Maybe we should check the result of clock_gettime() function
and fall back to base::TimeTicks if the result is not '0'. I imagine it should
never happen but who knows.

[Ryan Hamilton, 2017-05-10 18:31:15]

The CQ bit was unchecked by rch@chromium.org

[Ryan Hamilton, 2017-05-10 19:07:47]

https://codereview.chromium.org/2871573009/diff/100001/net/quic/platform/impl...
File net/quic/platform/impl/quic_chromium_clock.cc (right):

https://codereview.chromium.org/2871573009/diff/100001/net/quic/platform/impl...
net/quic/platform/impl/quic_chromium_clock.cc:35: clock_gettime(CLOCK_MONOTONIC,
&tp);
On 2017/05/10 18:10:11, kapishnikov wrote:
> One extra comment. Maybe we should check the result of clock_gettime()
function
> and fall back to base::TimeTicks if the result is not '0'. I imagine it should
> never happen but who knows.

Done. If this sloshes back and forth, that would be terrible but the man page
suggests that if it fails, it'll fail consistently.

[Ryan Hamilton, 2017-05-10 19:07:54]

The CQ bit was checked by rch@chromium.org

[Ryan Hamilton, 2017-05-10 19:07:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
ianswett@chromium.org, kapishnikov@chromium.org, zhongyi@chromium.org
Link to the patchset: https://codereview.chromium.org/2871573009/#ps120001
(title: "Fallback")

[commit-bot: I haz the power, 2017-05-10 19:09:03]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 21:44:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 21:44:34]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ryan Hamilton, 2017-05-12 17:39:29]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2017-05-12 17:40:56]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 20:37:43]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1494610769970380,
"parent_rev": "354e86bea96bc19c3d922f3733b81a8f8b635993", "commit_rev":
"ac0ea495b772c2b91ff94427b5a1cb6646cc4daf"}

[commit-bot: I haz the power, 2017-05-12 20:37:54]

Description was changed from

==========
Use clock_gettime instead of base::TimeTicks::Now() in QUIC
==========

to

==========
Use clock_gettime instead of base::TimeTicks::Now() in QUIC

Review-Url: https://codereview.chromium.org/2871573009
Cr-Commit-Position: refs/heads/master@{#471434}
Committed:
https://chromium.googlesource.com/chromium/src/+/ac0ea495b772c2b91ff94427b5a1...
==========

[commit-bot: I haz the power, 2017-05-12 20:37:55]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/ac0ea495b772c2b91ff94427b5a1...