Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(769)

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

Created:
3 years, 7 months ago by kapishnikov
Modified:
3 years, 7 months ago
Reviewers:
ianswett, Ryan Hamilton, miu
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 Review-Url: https://codereview.chromium.org/2894173002 Cr-Commit-Position: refs/heads/master@{#474336} Committed: https://chromium.googlesource.com/chromium/src/+/5dc9bafa3dcdc44f93757689ccff9629e8d085cf

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 #

Patch Set 7 : Fixed bug on 32-bit platforms #

Unified diffs Side-by-side diffs Delta from patch set Stats (+25 lines, -18 lines) Patch
M base/time/time_mac.cc View 1 2 3 4 5 6 2 chunks +17 lines, -0 lines 0 comments Download
M net/quic/platform/impl/quic_chromium_clock.cc View 1 2 3 4 5 2 chunks +8 lines, -18 lines 0 comments Download

Messages

Total messages: 40 (25 generated)
kapishnikov
This CL is the continuation of Ryan's work: https://codereview.chromium.org/2871573009/. We have observed that replacing sysctl() ...
3 years, 7 months 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!
3 years, 7 months 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 ...
3 years, 7 months 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): > ...
3 years, 7 months 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 > ...
3 years, 7 months 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 ...
3 years, 7 months 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 ...
3 years, 7 months 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.
3 years, 7 months 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. :)
3 years, 7 months 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: ...
3 years, 7 months ago (2017-05-23 19:16:52 UTC) #19
kapishnikov
Thanks! 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); On 2017/05/23 19:16:52, Ryan Hamilton ...
3 years, 7 months ago (2017-05-23 19:38:37 UTC) #23
kapishnikov
The base tests are consistently failing on 32-bit device simulators (e.g. iPhone 5) but passing ...
3 years, 7 months ago (2017-05-24 14:02:27 UTC) #27
kapishnikov
The tests were failing on 32-bit because the structure member "timespec.tv_spec" was declared as long. ...
3 years, 7 months ago (2017-05-24 15:24:58 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2894173002/120001
3 years, 7 months ago (2017-05-24 17:19:55 UTC) #37
commit-bot: I haz the power
3 years, 7 months ago (2017-05-24 17:26:19 UTC) #40
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/5dc9bafa3dcdc44f93757689ccff...

Powered by Google App Engine
This is Rietveld 408576698