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

Issue 2393953003: Go lock-free for RolloverProtectedNow() (Closed)

Created:
3 years ago by bcwhite
Modified:
3 years ago
CC:
chromium-reviews, manzagop (departed)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Go lock-free for RolloverProtectedNow() This turns a global lock acquisition with every TimeTicks::Now() into a single 32-bit atomic load in most cases, with a 32-bit CAS about once every 4.5 hours. It also allows code that tracks locks (for hang detection) to not cause a recursive loop when recording the time at which a lock is being acquired. BUG=652432 Committed: https://crrev.com/fdf17e0882099ea5fd62d10577100a203a3d9ec7 Cr-Commit-Position: refs/heads/master@{#424495}

Patch Set 1 #

Patch Set 2 : comment fixes #

Total comments: 4

Patch Set 3 : comment improvements #

Total comments: 4

Patch Set 4 : change opaque field into atomic type #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -25 lines) Patch
M base/time/time_win.cc View 1 2 3 3 chunks +59 lines, -25 lines 0 comments Download

Messages

Total messages: 25 (14 generated)
bcwhite
Bruce... Got another atomic/lock-free thing for you to look at, if you don't mind.
3 years ago (2016-10-06 12:25:08 UTC) #6
manzagop (departed)
Do you think this could have a measurable performance impact? Can you comment on whether ...
3 years ago (2016-10-06 13:46:36 UTC) #8
brucedawson
Just a few comment questions/nits. Reminds me of a __rdtsc type bug in the Xbox ...
3 years ago (2016-10-07 21:00:05 UTC) #9
bcwhite
https://codereview.chromium.org/2393953003/diff/20001/base/time/time_win.cc File base/time/time_win.cc (right): https://codereview.chromium.org/2393953003/diff/20001/base/time/time_win.cc#newcode339 base/time/time_win.cc:339: // A count of the number of detected rollovers. ...
3 years ago (2016-10-11 15:27:52 UTC) #10
bcwhite
miu@chromium.org: Please review changes in
3 years ago (2016-10-11 15:32:53 UTC) #12
miu
Nice solution. I wish the getTime() and QPCNow() functions in ntdll.dll were also lock-free. ;-) ...
3 years ago (2016-10-11 16:54:04 UTC) #13
brucedawson
On 2016/10/11 16:54:04, miu_OOO_until_17_Oct wrote: > Nice solution. I wish the getTime() and QPCNow() functions ...
3 years ago (2016-10-11 17:37:30 UTC) #16
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/2393953003/60001
3 years ago (2016-10-11 18:30:03 UTC) #21
commit-bot: I haz the power
Committed patchset #4 (id:60001)
3 years ago (2016-10-11 18:34:30 UTC) #22
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/fdf17e0882099ea5fd62d10577100a203a3d9ec7 Cr-Commit-Position: refs/heads/master@{#424495}
3 years ago (2016-10-11 18:36:56 UTC) #24
bcwhite
3 years ago (2016-10-12 14:17:50 UTC) #25
Message was sent while issue was closed.
https://codereview.chromium.org/2393953003/diff/40001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/2393953003/diff/40001/base/time/time_win.cc#n...
base/time/time_win.cc:45: #include "base/synchronization/lock.h"
On 2016/10/11 16:54:04, miu_OOO_until_17_Oct wrote:
> Can this be removed now? (On my cell on an airplane...hard for me to scan for
> myself.)

No, there is still a lock when doing updates to the high-frequency timer.

https://codereview.chromium.org/2393953003/diff/40001/base/time/time_win.cc#n...
base/time/time_win.cc:331: int32_t as_opaque_32;
On 2016/10/11 16:54:04, miu_OOO_until_17_Oct wrote:
> nit: s/int32_t/base::subtle::Atomic32/

Okay.  Though this particular field is never accessed using atomic methods.

Powered by Google App Engine
This is Rietveld 408576698