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

Issue 394363003: Use __rdtsc on Windows. (Closed)

Created:
6 years, 5 months ago by mtklein_C
Modified:
6 years, 5 months ago
Reviewers:
bungeman-skia, mtklein
CC:
reviews_skia.org, bungeman-skia
Base URL:
https://skia.googlesource.com/skia.git@master
Project:
skia
Visibility:
Public.

Description

Use __rdtsc on Windows. This seems to be ~100x higher resolution than QueryPerformanceCounter. AFAIK, all our Windows perf bots have constant_tsc, so we can be a bit more direct about using rdtsc directly: it'll always tick at the max CPU frequency. Now, the question remains, what is the max CPU frequency to divide through by? It looks like QueryPerformanceFrequency actually gives the CPU frequency in kHz, suspiciously exactly what we need to divide through to get elapsed milliseconds. That was a freebie. I did some before/after comparison on slow benchmarks. Timings look the same. Going to land this without review tonight to see what happens on the bots; happy to review carefully tomorrow. R=mtklein@google.com TBR=bungeman BUG=skia: Committed: https://skia.googlesource.com/skia/+/9129477

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -30 lines) Patch
M bench/nanobench.cpp View 3 chunks +2 lines, -7 lines 0 comments Download
M tools/Stats.h View 2 chunks +1 line, -3 lines 0 comments Download
M tools/timer/SysTimer_windows.h View 1 chunk +1 line, -1 line 0 comments Download
M tools/timer/SysTimer_windows.cpp View 3 chunks +16 lines, -19 lines 0 comments Download

Messages

Total messages: 6 (0 generated)
mtklein
lgtm
6 years, 5 months ago (2014-07-16 23:44:14 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/394363003/1
6 years, 5 months ago (2014-07-16 23:44:35 UTC) #2
mtklein
The CQ bit was unchecked by mtklein@google.com
6 years, 5 months ago (2014-07-16 23:59:26 UTC) #3
mtklein_C
Committed patchset #1 manually as r9129477 (presubmit successful).
6 years, 5 months ago (2014-07-16 23:59:37 UTC) #4
bungeman-skia
Other than being totally magic and stuff, lgtm. Is there any way to know if ...
6 years, 5 months ago (2014-07-17 14:07:14 UTC) #5
mtklein
6 years, 5 months ago (2014-07-17 14:12:39 UTC) #6
Message was sent while issue was closed.
On 2014/07/17 14:07:14, bungeman1 wrote:
> Other than being totally magic and stuff, lgtm. Is there any way to know if
the
> assumptions here hold true, so at least we know if they ever become bogus?

Yeah, there might be.  

As far as I've read up, constant_tsc will always be true for future Intel and
AMD chips, and it's true of every machine we'll ever run Windows performance
tests on (even, run Windows on).  The only part I'm wary about is reading the
processor frequency, but all our numbers seem to line up correctly, so I'd say,
so far so good.  Worst case we can time a long loop of this against a long loop
of a lower-precision timer and assert they're close.

Powered by Google App Engine
This is Rietveld 408576698