Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2393953003:
Go lock-free for RolloverProtectedNow() (Closed)
DescriptionGo 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 #Messages
Total messages: 25 (14 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bcwhite@chromium.org changed reviewers: + brucedawson@chromium.org
Bruce... Got another atomic/lock-free thing for you to look at, if you don't mind.
manzagop@chromium.org changed reviewers: + manzagop@chromium.org
Do you think this could have a measurable performance impact? Can you comment on whether we can measure it and if not whether it's worth trying to?
Just a few comment questions/nits. Reminds me of a __rdtsc type bug in the Xbox 360 CPU. I think it works. Test well. lgtm 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#n... base/time/time_win.cc:339: // A count of the number of detected rollovers. Using this as the upper How can a 16-bit value be the upper half of a 64-bit value? I think I know what you mean, but it's confusing. How about "This can be stored together with the last_8 value in a 32-bit value, while still giving a 48-bit tick counter when combined with the 32-bits returned by timeGetTime" https://codereview.chromium.org/2393953003/diff/20001/base/time/time_win.cc#n... base/time/time_win.cc:354: // 48 days. Why the change to 48 days? Everywhere else it remains either ~49 days or 49 days.
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#n... base/time/time_win.cc:339: // A count of the number of detected rollovers. Using this as the upper On 2016/10/07 21:00:05, brucedawson wrote: > How can a 16-bit value be the upper half of a 64-bit value? I think I know what > you mean, but it's confusing. > > How about "This can be stored together with the last_8 value in a 32-bit value, > while still giving a 48-bit tick counter when combined with the 32-bits returned > by timeGetTime" Done. https://codereview.chromium.org/2393953003/diff/20001/base/time/time_win.cc#n... base/time/time_win.cc:354: // 48 days. On 2016/10/07 21:00:04, brucedawson wrote: > Why the change to 48 days? Everywhere else it remains either ~49 days or 49 > days. It's actually 48.8 days. Because only the top 8 bits are stored for "last", changes in the bottom 24 bits go unnoticed which reduces the "notice" time by 1/256. Comment improved.
bcwhite@chromium.org changed reviewers: + miu@chromium.org
miu@chromium.org: Please review changes in
Nice solution. I wish the getTime() and QPCNow() functions in ntdll.dll were also lock-free. ;-) lgtm % nits: 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" Can this be removed now? (On my cell on an airplane...hard for me to scan for myself.) https://codereview.chromium.org/2393953003/diff/40001/base/time/time_win.cc#n... base/time/time_win.cc:331: int32_t as_opaque_32; nit: s/int32_t/base::subtle::Atomic32/
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/11 16:54:04, miu_OOO_until_17_Oct wrote: > Nice solution. I wish the getTime() and QPCNow() functions in ntdll.dll were > also lock-free. ;-) <rant>I wish that timeGetTime had a 64-bit version. getTickCount64 was added years ago and solves this problem trivially, but it lacks the resolution we need so we have to use timeGetTime. timeGetTime probably maintains a 64-bit counter, it just doesn't bother to share it. Lame.</rant>
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brucedawson@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2393953003/#ps60001 (title: "change opaque field into atomic type")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fdf17e0882099ea5fd62d10577100a203a3d9ec7 Cr-Commit-Position: refs/heads/master@{#424495}
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. |
