Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1977983003:
[base] Implement CPU time on Windows. (Closed)
Description[base] Implement CPU time on Windows.
We already implemented CPU time for OS X and POSIX, this path is a
follow up for the implementation on Windows.
BUG=v8:5000
LOG=n
Committed: https://crrev.com/ee43805a66440d78d20310a9f822dffe87237855
Cr-Commit-Position: refs/heads/master@{#36656}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address fadi's comment #
Total comments: 6
Patch Set 3 : Address fadi's comment #Patch Set 4 : #Patch Set 5 : #
Total comments: 4
Patch Set 6 : Add comments #
Total comments: 6
Patch Set 7 : #
Messages
Total messages: 28 (6 generated)
lpy@chromium.org changed reviewers: + bmeurer@chromium.org, fmeawad@chromium.org, jochen@chromium.org
PTAL.
Note that, with this patch landed, I think we will drop the support of Windows XP, please see https://bugs.chromium.org/p/v8/issues/detail?id=5000 for details. Thanks.
https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc#n... src/base/platform/time.cc:77: uint64_t QPCNowRaw() { If I am reading the correctly, we do not use QPC for HighRes time on windows in V8, therefore I also believe that V8 does not have High Res time on Windows. If we make the ThreadTime use a higher res clock than the one used by Time, it might lead to some confusion. Can you verify that is not the case? If it is the case, then we should provide a windows HighRes clock for V8 and use it for the wall clock as well (before landing the CPU time)
https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc#n... src/base/platform/time.cc:73: return strcmp(cpu.vendor(), "AuthenticAMD") == 0 && cpu.family() == 15; I commented on this in the other CL but I think it catches more than just X2's now. Working from (flawed) memory but I believe X2's have their extended family set to 0 or 1.
On 2016/05/15 09:06:46, noordhuis wrote: > https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc > File src/base/platform/time.cc (right): > > https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc#n... > src/base/platform/time.cc:73: return strcmp(cpu.vendor(), "AuthenticAMD") == 0 > && cpu.family() == 15; > I commented on this in the other CL but I think it catches more than just X2's > now. Working from (flawed) memory but I believe X2's have their extended family > set to 0 or 1. When you said X2's, do you mean AMD Athlon 64 X2? If yes, any suggestion about how to verify their ext_family value? Thanks
On 2016/05/15 09:16:17, lpy wrote: > When you said X2's, do you mean AMD Athlon 64 X2? If yes, any suggestion about > how to verify their ext_family value? > Thanks I believe the Athlon 64 X2 and Turion 64 X2 report their ext_family as 0; the Athlon II X2 reports 1. I don't know if that last one is affected by the QPC bug, though.
https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc#n... src/base/platform/time.cc:73: return strcmp(cpu.vendor(), "AuthenticAMD") == 0 && cpu.family() == 15; On 2016/05/15 09:06:46, noordhuis wrote: > I commented on this in the other CL but I think it catches more than just X2's > now. Working from (flawed) memory but I believe X2's have their extended family > set to 0 or 1. I will leave this as it is for now, since we are using the same implementation as Chrome does https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time_win... and I believe Mozilla also has the same implementation. https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc#n... src/base/platform/time.cc:77: uint64_t QPCNowRaw() { On 2016/05/14 01:04:07, fmeawad wrote: > If I am reading the correctly, we do not use QPC for HighRes time on windows in > V8, > therefore I also believe that V8 does not have High Res time on Windows. > > If we make the ThreadTime use a higher res clock than the one used by Time, it > might lead to some confusion. > > Can you verify that is not the case? > > If it is the case, then we should provide a windows HighRes clock for V8 and use > it for the wall clock as well (before landing the CPU time) It depends. V8 may fall back to low resolution time. This construct trait: https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/platfo... shows that V8 will fall back to low res time when QueryPerformanceFrequency fails or on a buggy CPU.
> https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc#n... > src/base/platform/time.cc:77: uint64_t QPCNowRaw() { > On 2016/05/14 01:04:07, fmeawad wrote: > > If I am reading the correctly, we do not use QPC for HighRes time on windows > in > > V8, > > therefore I also believe that V8 does not have High Res time on Windows. > > > > If we make the ThreadTime use a higher res clock than the one used by Time, it > > might lead to some confusion. > > > > Can you verify that is not the case? > > > > If it is the case, then we should provide a windows HighRes clock for V8 and > use > > it for the wall clock as well (before landing the CPU time) > > It depends. V8 may fall back to low resolution time. > > This construct trait: > https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/base/platfo... > shows that V8 will fall back to low res time when QueryPerformanceFrequency > fails or on a buggy CPU. Also, note that on systems that run Windows XP or later, QueryPerformanceFrequency will never fail.
On 2016/05/24 22:21:46, lpy wrote: > https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc > File src/base/platform/time.cc (right): > > https://codereview.chromium.org/1977983003/diff/1/src/base/platform/time.cc#n... > src/base/platform/time.cc:73: return strcmp(cpu.vendor(), "AuthenticAMD") == 0 > && cpu.family() == 15; > On 2016/05/15 09:06:46, noordhuis wrote: > > I commented on this in the other CL but I think it catches more than just X2's > > now. Working from (flawed) memory but I believe X2's have their extended > family > > set to 0 or 1. > > I will leave this as it is for now, since we are using the same implementation > as Chrome does > https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time_win... > and I believe Mozilla also has the same implementation. Sorry, Mozilla doesn't have the same implementation, I think they are using a loop to check if QPC is reliable. https://dxr.mozilla.org/mozilla-central/source/mozglue/misc/TimeStamp_windows...
I made a few changes, PTAL.
https://codereview.chromium.org/1977983003/diff/20001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1977983003/diff/20001/src/base/platform/time.... src/base/platform/time.cc:71: inline bool IsBuggyAthlon(const v8::base::CPU& cpu) { nit:Rename to IsQPCReliable https://codereview.chromium.org/1977983003/diff/20001/src/base/platform/time.... src/base/platform/time.cc:547: // where QueryPerformanceCounter is unreliable. Change comment to: If QPC not reliable, fallback to low-resolution tick clock. https://codereview.chromium.org/1977983003/diff/20001/src/base/platform/time.... src/base/platform/time.cc:548: CPU cpu; CPU cpu should move inside IsBuggyAthlon since it is not used anywhere else?
I addressed the comments, PTAL. https://codereview.chromium.org/1977983003/diff/20001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1977983003/diff/20001/src/base/platform/time.... src/base/platform/time.cc:71: inline bool IsBuggyAthlon(const v8::base::CPU& cpu) { On 2016/05/24 22:56:59, fmeawad wrote: > nit:Rename to IsQPCReliable Done. https://codereview.chromium.org/1977983003/diff/20001/src/base/platform/time.... src/base/platform/time.cc:547: // where QueryPerformanceCounter is unreliable. On 2016/05/24 22:56:59, fmeawad wrote: > Change comment to: If QPC not reliable, fallback to low-resolution tick clock. Done. https://codereview.chromium.org/1977983003/diff/20001/src/base/platform/time.... src/base/platform/time.cc:548: CPU cpu; On 2016/05/24 22:56:59, fmeawad wrote: > CPU cpu should move inside IsBuggyAthlon since it is not used anywhere else? Done.
lgtm https://codereview.chromium.org/1977983003/diff/80001/src/base/cpu.cc File src/base/cpu.cc (right): https://codereview.chromium.org/1977983003/diff/80001/src/base/cpu.cc#newcode422 src/base/cpu.cc:422: nit: Add a small comment to explain what happens here. https://codereview.chromium.org/1977983003/diff/80001/src/base/win32-headers.h File src/base/win32-headers.h (right): https://codereview.chromium.org/1977983003/diff/80001/src/base/win32-headers.... src/base/win32-headers.h:31: // function to be present). nit: Update the comment
bmeurer@, PTAL. https://codereview.chromium.org/1977983003/diff/80001/src/base/cpu.cc File src/base/cpu.cc (right): https://codereview.chromium.org/1977983003/diff/80001/src/base/cpu.cc#newcode422 src/base/cpu.cc:422: On 2016/05/25 23:40:31, fmeawad wrote: > nit: Add a small comment to explain what happens here. Done. https://codereview.chromium.org/1977983003/diff/80001/src/base/win32-headers.h File src/base/win32-headers.h (right): https://codereview.chromium.org/1977983003/diff/80001/src/base/win32-headers.... src/base/win32-headers.h:31: // function to be present). On 2016/05/25 23:40:31, fmeawad wrote: > nit: Update the comment Done.
https://codereview.chromium.org/1977983003/diff/100001/src/base/cpu.cc File src/base/cpu.cc (right): https://codereview.chromium.org/1977983003/diff/100001/src/base/cpu.cc#newcod... src/base/cpu.cc:423: // Check if CPU has non stoppable Time Stamp Counter. nit: time stamp counter https://codereview.chromium.org/1977983003/diff/100001/src/base/win32-headers.h File src/base/win32-headers.h (right): https://codereview.chromium.org/1977983003/diff/100001/src/base/win32-headers... src/base/win32-headers.h:30: // Require Windows Vista or higher (this is required for the RtlCaptureContext nit: Still needs updating, it is required to read the CPU time https://codereview.chromium.org/1977983003/diff/100001/src/base/win32-headers... src/base/win32-headers.h:42: // Require Windows Vista or higher when compiling with MinGW. This is for MinGW nit: I do not thing the compiling with MinGW does require Vista which is what the comment states
ptal. https://codereview.chromium.org/1977983003/diff/100001/src/base/cpu.cc File src/base/cpu.cc (right): https://codereview.chromium.org/1977983003/diff/100001/src/base/cpu.cc#newcod... src/base/cpu.cc:423: // Check if CPU has non stoppable Time Stamp Counter. On 2016/05/26 16:40:34, fmeawad wrote: > nit: time stamp counter Done. https://codereview.chromium.org/1977983003/diff/100001/src/base/win32-headers.h File src/base/win32-headers.h (right): https://codereview.chromium.org/1977983003/diff/100001/src/base/win32-headers... src/base/win32-headers.h:30: // Require Windows Vista or higher (this is required for the RtlCaptureContext On 2016/05/26 16:40:34, fmeawad wrote: > nit: Still needs updating, it is required to read the CPU time Done. https://codereview.chromium.org/1977983003/diff/100001/src/base/win32-headers... src/base/win32-headers.h:42: // Require Windows Vista or higher when compiling with MinGW. This is for MinGW On 2016/05/26 16:40:34, fmeawad wrote: > nit: I do not thing the compiling with MinGW does require Vista which is what > the comment states Done.
lgtm
Description was changed from ========== Implement CPU time on Windows. We already implemented CPU time for OS X and POSIX, this path is a follow up for the implementation on Windows. BUG=v8:5000 LOG=n ========== to ========== [base] Implement CPU time on Windows. We already implemented CPU time for OS X and POSIX, this path is a follow up for the implementation on Windows. BUG=v8:5000 LOG=n ==========
Description was changed from ========== [base] Implement CPU time on Windows. We already implemented CPU time for OS X and POSIX, this path is a follow up for the implementation on Windows. BUG=v8:5000 LOG=n ========== to ========== [base] Implement CPU time on Windows. We already implemented CPU time for OS X and POSIX, this path is a follow up for the implementation on Windows. BUG=v8:5000 LOG=n ==========
LGTM (rubber-stamped)
lgtm
The CQ bit was checked by lpy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977983003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977983003/120001
Message was sent while issue was closed.
Description was changed from ========== [base] Implement CPU time on Windows. We already implemented CPU time for OS X and POSIX, this path is a follow up for the implementation on Windows. BUG=v8:5000 LOG=n ========== to ========== [base] Implement CPU time on Windows. We already implemented CPU time for OS X and POSIX, this path is a follow up for the implementation on Windows. BUG=v8:5000 LOG=n ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [base] Implement CPU time on Windows. We already implemented CPU time for OS X and POSIX, this path is a follow up for the implementation on Windows. BUG=v8:5000 LOG=n ========== to ========== [base] Implement CPU time on Windows. We already implemented CPU time for OS X and POSIX, this path is a follow up for the implementation on Windows. BUG=v8:5000 LOG=n Committed: https://crrev.com/ee43805a66440d78d20310a9f822dffe87237855 Cr-Commit-Position: refs/heads/master@{#36656} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/ee43805a66440d78d20310a9f822dffe87237855 Cr-Commit-Position: refs/heads/master@{#36656} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
