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

Issue 797893003: [Windows] One TimeTicks clock: efficient/reliable high-res, with low-res fallback. (Closed)

Created:
5 years, 11 months ago by miu
Modified:
4 years, 5 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Windows] One TimeTicks clock: efficient/reliable high-res, with low-res fallback. This change resolves a critical issue where two different clocks' values were being mixed in computations involving TimeTicks for ~32% of users on Windows. The solution is to force the use of a single clock for TimeTicks::Now() and HighResNow(). In other words, we're moving towards a world where the TimeTicks type should guarantee all time values are on the same timeline. New behavior: On systems where QPC is reliable AND efficient, Now() will simply use the QPC clock. This is consistent with how Now() behaves on the Mac and POSIX platforms. However, if QPC is working but is inefficient, Now() will use the low-res clock. Finally, like before, if the QPC is not working on the current system, Now() will use the low-res clock. NowFromSystemTraceTime() will always use the QPC clock if it is working, just as before, and regardless of whether it is expensive to use. This is to ensure tracing data remains useful. When QPC uses either HPET or ACPI PM as a clock source, online literature suggests clock sampling will be ~20 to ~40 times as costly (about 1.2 to 2.4us per call). Note: From discussion with stakeholders, there is a concern this change will increase latency and jitter in the compositor pipeline on the affected Windows machines. Future changes will take alternate approaches to resolve these problems. Note 2: This change minimizes TimeTicks API changes, in favor of future clean-up changes. BUG=396384, 435931, 439656 Committed: https://crrev.com/5990719bcd1403b5a5b45874ce808727c033e7ec Cr-Commit-Position: refs/heads/master@{#311414}

Patch Set 1 : #

Total comments: 19

Patch Set 2 : Resolve lack of std::abs(int64) overload on some toolchains. #

Total comments: 10

Patch Set 3 : Addressed comments from PS1 and PS2. Threading fixes in InitializeNowFunctionPointers(). #

Total comments: 8

Patch Set 4 : One more memory barrier, and update technical discussion. #

Total comments: 2

Patch Set 5 : Use std::atomic, and everyone is happy. #

Total comments: 2

Patch Set 6 : Use std::atomic_thread_fence() instead, to avoid subtle sync error. #

Patch Set 7 : Workaround because use of <atomic> is not allowed yet. :-( #

Total comments: 8

Patch Set 8 : Fix comment about atomic_thread_fence. #

Patch Set 9 : Fix macro naming style. #

Total comments: 2

Patch Set 10 : Better testing of TimeTicks::FromQPCValue(). #

Total comments: 5

Patch Set 11 : Rewrite FromQPCValue() test again. #

Patch Set 12 : rebase #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+269 lines, -259 lines) Patch
M base/time/time.h View 1 2 3 chunks +47 lines, -21 lines 0 comments Download
M base/time/time_mac.cc View 1 chunk +1 line, -6 lines 0 comments Download
M base/time/time_posix.cc View 1 chunk +1 line, -6 lines 0 comments Download
M base/time/time_unittest.cc View 1 3 chunks +22 lines, -3 lines 0 comments Download
M base/time/time_win.cc View 1 2 3 4 5 6 7 8 4 chunks +139 lines, -160 lines 1 comment Download
M base/time/time_win_unittest.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +56 lines, -60 lines 0 comments Download
M ui/gfx/frame_time.h View 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 54 (10 generated)
DaleCurtis
Does this respect the API contract for TimeTicks when the low resolution clock is used? ...
5 years, 11 months ago (2015-01-06 01:15:03 UTC) #4
miu
On 2015/01/06 01:15:03, DaleCurtis wrote: > Does this respect the API contract for TimeTicks when ...
5 years, 11 months ago (2015-01-06 05:00:35 UTC) #5
miu
all reviewers: PTAL.
5 years, 11 months ago (2015-01-06 22:45:20 UTC) #8
DaleCurtis
Do we have data which shows exactly what a 20x-40x cost in QPC calls will ...
5 years, 11 months ago (2015-01-07 00:14:12 UTC) #9
DaleCurtis
Please add a more significant test section as well. I.e. what does this look like ...
5 years, 11 months ago (2015-01-07 00:14:55 UTC) #10
brianderson
https://codereview.chromium.org/797893003/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/797893003/diff/60001/base/time/time.h#newcode105 base/time/time.h:105: bool is_negative() const { This is not used and ...
5 years, 11 months ago (2015-01-07 00:53:44 UTC) #11
miu
On 2015/01/07 00:14:12, DaleCurtis wrote: > Do we have data which shows exactly what a ...
5 years, 11 months ago (2015-01-07 01:33:27 UTC) #12
miu
On 2015/01/07 00:14:55, DaleCurtis wrote: > Please add a more significant test section as well. ...
5 years, 11 months ago (2015-01-07 01:36:47 UTC) #13
DaleCurtis
I just meant you should add some commit notes (TEST=?) about exactly what you've done ...
5 years, 11 months ago (2015-01-07 01:38:36 UTC) #14
brucedawson
https://codereview.chromium.org/797893003/diff/80001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/797893003/diff/80001/base/time/time.h#newcode643 base/time/time.h:643: // perform math with TimeTicks values returned by Now() ...
5 years, 11 months ago (2015-01-07 01:49:26 UTC) #16
brianderson
https://codereview.chromium.org/797893003/diff/80001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/80001/base/time/time_win.cc#newcode440 base/time/time_win.cc:440: InterlockedExchangePointer( On 2015/01/07 01:49:26, brucedawson wrote: > What protection ...
5 years, 11 months ago (2015-01-07 01:53:07 UTC) #17
brianderson
https://codereview.chromium.org/797893003/diff/80001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/80001/base/time/time_win.cc#newcode440 base/time/time_win.cc:440: InterlockedExchangePointer( On 2015/01/07 01:53:07, brianderson wrote: > On 2015/01/07 ...
5 years, 11 months ago (2015-01-07 01:54:54 UTC) #18
brucedawson
> If we do get rid of the InterlockedExchanges, we'd probably need to add barriers ...
5 years, 11 months ago (2015-01-07 01:58:10 UTC) #19
miu
Thanks, everyone. All comments addressed. PTAL. https://codereview.chromium.org/797893003/diff/60001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/797893003/diff/60001/base/time/time.h#newcode105 base/time/time.h:105: bool is_negative() const ...
5 years, 11 months ago (2015-01-07 05:22:46 UTC) #20
cpu_(ooo_6.6-7.5)
https://codereview.chromium.org/797893003/diff/100001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/100001/base/time/time_win.cc#newcode362 base/time/time_win.cc:362: // RDTSC on each processor are consistent with each ...
5 years, 11 months ago (2015-01-07 17:41:31 UTC) #21
cpu_(ooo_6.6-7.5)
ah one more important question. We are branching in ~3 days. Do we want to ...
5 years, 11 months ago (2015-01-07 17:42:33 UTC) #22
miu
On 2015/01/07 17:42:33, cpu wrote: > ah one more important question. We are branching in ...
5 years, 11 months ago (2015-01-07 19:01:43 UTC) #23
miu
https://codereview.chromium.org/797893003/diff/100001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/100001/base/time/time_win.cc#newcode362 base/time/time_win.cc:362: // RDTSC on each processor are consistent with each ...
5 years, 11 months ago (2015-01-07 19:20:18 UTC) #24
brianderson
https://codereview.chromium.org/797893003/diff/100001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/100001/base/time/time_win.cc#newcode384 base/time/time_win.cc:384: TimeDelta QPCValueToTimeDelta(LONGLONG qpc_value) { @brucedawson makes a good point ...
5 years, 11 months ago (2015-01-07 19:53:11 UTC) #25
cpu_(ooo_6.6-7.5)
we do this kind of stuff in several places for example content_main_runner.cc Init() is one ...
5 years, 11 months ago (2015-01-07 21:49:01 UTC) #26
miu
PTAL. The one-time-init threading issues should be all resolved now. There's been some additional discussion ...
5 years, 11 months ago (2015-01-07 22:04:33 UTC) #27
brucedawson
Consider not inserting a memory barrier *instruction* on the read. https://codereview.chromium.org/797893003/diff/120001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/120001/base/time/time_win.cc#newcode392 ...
5 years, 11 months ago (2015-01-07 22:29:13 UTC) #28
miu
Bruce: PTAL. Addressed your comment: https://codereview.chromium.org/797893003/diff/120001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/120001/base/time/time_win.cc#newcode392 base/time/time_win.cc:392: base::subtle::MemoryBarrier(); On 2015/01/07 22:29:13, ...
5 years, 11 months ago (2015-01-08 00:36:39 UTC) #29
brucedawson
https://codereview.chromium.org/797893003/diff/140001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/140001/base/time/time_win.cc#newcode463 base/time/time_win.cc:463: std::memory_order_release); I *think* this is wrong. I believe that ...
5 years, 11 months ago (2015-01-08 01:36:58 UTC) #30
miu
https://codereview.chromium.org/797893003/diff/140001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/140001/base/time/time_win.cc#newcode463 base/time/time_win.cc:463: std::memory_order_release); On 2015/01/08 01:36:57, brucedawson wrote: > I *think* ...
5 years, 11 months ago (2015-01-08 03:15:45 UTC) #31
cpu_(ooo_6.6-7.5)
wait, can we use <atomic> ? last I heard most library features were disallowed.
5 years, 11 months ago (2015-01-08 05:08:53 UTC) #32
brucedawson
If <atomic> is disallowed (and it looks like it is -- no sign of it ...
5 years, 11 months ago (2015-01-08 07:49:55 UTC) #33
miu
On 2015/01/08 07:49:55, brucedawson wrote: > If <atomic> is disallowed (and it looks like it ...
5 years, 11 months ago (2015-01-08 19:18:04 UTC) #34
brucedawson
https://codereview.chromium.org/797893003/diff/180001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/180001/base/time/time_win.cc#newcode389 base/time/time_win.cc:389: // what std::atomic_thread_fence does on Windows on all Intel ...
5 years, 11 months ago (2015-01-08 19:37:48 UTC) #35
miu
https://codereview.chromium.org/797893003/diff/180001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/180001/base/time/time_win.cc#newcode389 base/time/time_win.cc:389: // what std::atomic_thread_fence does on Windows on all Intel ...
5 years, 11 months ago (2015-01-08 19:45:56 UTC) #36
jamesr
https://codereview.chromium.org/797893003/diff/180001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/180001/base/time/time_win.cc#newcode390 base/time/time_win.cc:390: #define atomic_thread_fence(memory_order) _ReadWriteBarrier(); this doesn't follow macro naming rules: ...
5 years, 11 months ago (2015-01-08 20:31:54 UTC) #38
brianderson
https://codereview.chromium.org/797893003/diff/180001/ui/gfx/frame_time.h File ui/gfx/frame_time.h (right): https://codereview.chromium.org/797893003/diff/180001/ui/gfx/frame_time.h#newcode30 ui/gfx/frame_time.h:30: return base::TimeTicks::IsHighResolution(); On 2015/01/08 20:31:54, jamesr wrote: > imo ...
5 years, 11 months ago (2015-01-08 21:10:36 UTC) #39
miu
https://codereview.chromium.org/797893003/diff/180001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/180001/base/time/time_win.cc#newcode390 base/time/time_win.cc:390: #define atomic_thread_fence(memory_order) _ReadWriteBarrier(); On 2015/01/08 20:31:54, jamesr wrote: > ...
5 years, 11 months ago (2015-01-08 22:16:01 UTC) #40
cpu_(ooo_6.6-7.5)
I mentioned <atomic> also because in another thread we were looking if a binary generated ...
5 years, 11 months ago (2015-01-08 22:37:14 UTC) #41
brianderson
https://codereview.chromium.org/797893003/diff/240001/base/time/time_win_unittest.cc File base/time/time_win_unittest.cc (right): https://codereview.chromium.org/797893003/diff/240001/base/time/time_win_unittest.cc#newcode241 base/time/time_win_unittest.cc:241: for (int i = 0; i < kIterations; ++i) ...
5 years, 11 months ago (2015-01-09 00:02:02 UTC) #42
cpu_(ooo_6.6-7.5)
lgtm on my side, only looked at the windows parts in detail.
5 years, 11 months ago (2015-01-09 22:02:20 UTC) #43
miu
brianderson: Addressed your comments on the FromQPCValue() test. LGTY now? https://codereview.chromium.org/797893003/diff/240001/base/time/time_win_unittest.cc File base/time/time_win_unittest.cc (right): https://codereview.chromium.org/797893003/diff/240001/base/time/time_win_unittest.cc#newcode241 ...
5 years, 11 months ago (2015-01-14 02:12:25 UTC) #44
brianderson
https://codereview.chromium.org/797893003/diff/240001/base/time/time_win_unittest.cc File base/time/time_win_unittest.cc (right): https://codereview.chromium.org/797893003/diff/240001/base/time/time_win_unittest.cc#newcode246 base/time/time_win_unittest.cc:246: const TimeTicks expected_value = start_time + On 2015/01/14 02:12:24, ...
5 years, 11 months ago (2015-01-14 02:31:42 UTC) #45
darin (slow to review)
LGTM, deferring to the other reviewers
5 years, 11 months ago (2015-01-14 03:39:20 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/797893003/280001
5 years, 11 months ago (2015-01-14 05:12:46 UTC) #48
commit-bot: I haz the power
Committed patchset #12 (id:280001)
5 years, 11 months ago (2015-01-14 05:33:37 UTC) #49
commit-bot: I haz the power
Patchset 12 (id:??) landed as https://crrev.com/5990719bcd1403b5a5b45874ce808727c033e7ec Cr-Commit-Position: refs/heads/master@{#311414}
5 years, 11 months ago (2015-01-14 05:34:20 UTC) #50
Alexander Potapenko
https://codereview.chromium.org/797893003/diff/280001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/797893003/diff/280001/base/time/time_win.cc#newcode396 base/time/time_win.cc:396: ATOMIC_THREAD_FENCE(memory_order_acquire); There's base/atomicops.h with Acquire_Load() and Release_Store() for this.
4 years, 5 months ago (2016-07-05 12:20:29 UTC) #53
Alexander Potapenko
4 years, 5 months ago (2016-07-05 12:20:29 UTC) #54
Message was sent while issue was closed.
https://codereview.chromium.org/797893003/diff/280001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/797893003/diff/280001/base/time/time_win.cc#n...
base/time/time_win.cc:396: ATOMIC_THREAD_FENCE(memory_order_acquire);
There's base/atomicops.h with Acquire_Load() and Release_Store() for this.

Powered by Google App Engine
This is Rietveld 408576698