Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(73)

Issue 2894173002: Use faster clock_gettime() instead of sysctl() to get current ticks on iOS10

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 days, 12 hours ago by kapishnikov
Modified:
7 hours, 58 minutes ago
CC:
mef, chromium-reviews, net-reviews_chromium.org, danakj+watch_chromium.org, mac-reviews_chromium.org, cbentzel+watch_chromium.org, vmpstr+watch_chromium.org, Lei Zhang
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Use faster clock_gettime() instead of sysctl() to get current ticks on iOS10 BUG=724546

Patch Set 1 #

Patch Set 2 : Remove unused includes #

Total comments: 11

Patch Set 3 : Addressed Yuri's comments. #

Patch Set 4 : Change CLOCK_MONOTONIC_RAW back to CLOCK_MONOTONIC. #

Total comments: 4

Patch Set 5 : Changed DCHECK to DCHECK_GE #

Patch Set 6 : Small fix #

Messages

Total messages: 26 (15 generated)
kapishnikov
This CL is the continuation of Ryan's work: https://codereview.chromium.org/2871573009/. We have observed that replacing sysctl() ...
4 days, 11 hours ago (2017-05-19 18:49:28 UTC) #4
kapishnikov
Yuri, could you please take a look at the change in base/time/time_mac.cc? Thanks!
4 days, 11 hours ago (2017-05-19 18:57:35 UTC) #6
ianswett
lgtm https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#newcode78 base/time/time_mac.cc:78: struct timeval boottime; I didn't realize we were ...
4 days, 10 hours ago (2017-05-19 19:22:58 UTC) #8
kapishnikov
On 2017/05/19 19:22:58, ianswett wrote: > lgtm > > https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc > File base/time/time_mac.cc (right): > ...
4 days, 10 hours ago (2017-05-19 19:28:43 UTC) #9
kapishnikov
On 2017/05/19 19:28:43, kapishnikov wrote: > On 2017/05/19 19:22:58, ianswett wrote: > > lgtm > ...
4 days, 10 hours ago (2017-05-19 19:36:31 UTC) #10
miu
[+cc thestig@, since they've been looking at ToJavaTime() abuse] Comments on PS2: https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc File base/time/time_mac.cc ...
1 day, 7 hours ago (2017-05-22 22:16:14 UTC) #13
kapishnikov
Yuri, thanks for the review. Please see the answers/comments below. https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/2894173002/diff/20001/base/time/time_mac.cc#newcode60 ...
12 hours, 2 minutes ago (2017-05-23 17:56:51 UTC) #16
kapishnikov
I found this CL https://codereview.chromium.org/2172483002/ and switched CLOCK_MONOTONIC_RAW back to CLOCK_MONOTONIC.
11 hours, 44 minutes ago (2017-05-23 18:14:53 UTC) #17
miu
lgtm Looks like you already found the reason we don't use CLOCK_MONOTONIC_RAW. :)
11 hours, 13 minutes ago (2017-05-23 18:45:53 UTC) #18
Ryan Hamilton
net/quic/ LGTM, mod two nits. https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/quic_chromium_clock.cc File net/quic/platform/impl/quic_chromium_clock.cc (right): https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/quic_chromium_clock.cc#newcode27 net/quic/platform/impl/quic_chromium_clock.cc:27: DCHECK(ticks >= 0); nit: ...
10 hours, 42 minutes ago (2017-05-23 19:16:52 UTC) #19
kapishnikov
10 hours, 20 minutes ago (2017-05-23 19:38:37 UTC) #23
Thanks!

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

https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:27: DCHECK(ticks >= 0);
On 2017/05/23 19:16:52, Ryan Hamilton wrote:
> nit: DCHECK_LE(0, ticks);

Done. Used DCHECK_GE instead of DCHECK_LE.

https://codereview.chromium.org/2894173002/diff/60001/net/quic/platform/impl/...
net/quic/platform/impl/quic_chromium_clock.cc:34:
DCHECK(time_since_unix_epoch.InMicroseconds() >= 0);
On 2017/05/23 19:16:52, Ryan Hamilton wrote:
> nit: DCHECK_LE(0, time_since_unix_epoch.InMicroseconds());
> 
> (or perhaps save InMicroseconds() to a local variable to use in both the
DCHECK
> and the FromUNIXMicroseconds() call.)

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06