Change to Hi Res timers on Windows....

Change to Hi Res timers on Windows.

There are two parts of this:

1) TimeTicks:Now()
Don't call timeBeginPeriod() in all cases.
Use the new SystemMonitor class to watch battery on/off
transitions and use the timeBeginPeriod() only when we're
using the battery.

2) TimeTicks::UnreliableHiResNow()
Change this function from "UnreliableHiResNow()" to 
"HiResNow()".   We still use QPC, but we detect if we're 
on AMD Athlon XP machines which fail on QPC.  For those
systems, we fall back to TimeTicks::Now().

Updated tests to detect hardware specifics of timers.
Output of the test will contain lines such as these:

[ RUN      ] TimeTicks.SubMillisecondTimers
Min timer is: 1us
[       OK ] TimeTicks.SubMillisecondTimers
[ RUN      ] TimeTicks.TimeGetTimeCaps
timeGetTime range is 1 to 1000000ms
[       OK ] TimeTicks.TimeGetTimeCaps
[ RUN      ] TimeTicks.QueryPerformanceFrequency
QueryPerformanceFrequency is 2394.18MHz
[       OK ] TimeTicks.QueryPerformanceFrequency
[ RUN      ] TimeTicks.TimerPerformance
Time::Now: 0.11us per call
TimeTicks::Now: 0.09us per call
TimeTicks::HighResNow: 0.26us per call
[       OK ] TimeTicks.TimerPerformance




Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=2625

[darin (slow to review), 2008-09-25 22:54:51]

LG overall.  Here's some minor comments and a couple questions:

http://codereview.chromium.org/4092/diff/1/8
File base/time.h (right):

http://codereview.chromium.org/4092/diff/1/8#newcode358
Line 358: // be used when it is really needed.
The name of this method may make it a bit too attractive to casual coders. 
Perhaps you last sentence above can be changed to ALL CAPS or in some way made
more bold to help people understand that they should use HighResNow carefully?

http://codereview.chromium.org/4092/diff/1/3
File base/time_unittest.cc (right):

http://codereview.chromium.org/4092/diff/1/3#newcode89
Line 89: for (int index=0; index < 500; index++) {
nit: "int index = 0;"

http://codereview.chromium.org/4092/diff/1/3#newcode108
Line 108: // so that our averages come out better.
agreed!  we have a lot of code doing custom rounding.  would be nice if
InMilliseconds just did that.

http://codereview.chromium.org/4092/diff/1/3#newcode110
Line 110: EXPECT_GE(delta.InMicroseconds(), 9000);
can't we expect better precision in the microseconds case?

http://codereview.chromium.org/4092/diff/1/3#newcode120
Line 120: EXPECT_GE(delta.InMicroseconds(), 10000);
the problem described above for TimeTicks::Deltas doesn't exist here as well?  i
would have thought that it could since HighResNow == Now on some architectures.

http://codereview.chromium.org/4092/diff/1/4
File base/time_unittest_win.cc (right):

http://codereview.chromium.org/4092/diff/1/4#newcode7
Line 7: #include <mmsystem.h>
can these headers be sorted alphabetically per the style guide?

http://codereview.chromium.org/4092/diff/1/4#newcode48
Line 48: EXPECT_GE(milliseconds, 0);
I assume that this change was done because InMilliseconds rounds down?  Might be
worth a comment here.

http://codereview.chromium.org/4092/diff/1/4#newcode111
Line 111: for (int i=0; i < 1000; i++) {
nit: "int i = 0;"

http://codereview.chromium.org/4092/diff/1/4#newcode115
Line 115: delta.InMicroseconds() < 1000) {
nit: indent 2 more spaces so that the "delta" lines up with the "delta" above
it?

http://codereview.chromium.org/4092/diff/1/4#newcode116
Line 116: if (min_timer > delta.InMicroseconds())
this block is over indented

http://codereview.chromium.org/4092/diff/1/4#newcode157
Line 157: // Verify that various timer mechanisms can always complete in
quickly.
this sentence reads funny... "in quickly"?  maybe kill the "in"

http://codereview.chromium.org/4092/diff/1/4#newcode168
Line 168: { reinterpret_cast<TestFunc>(Time::Now), "Time::Now" },
ugly, but ok.  sizeof(Time) == sizeof(TimeTicks).  can you use COMPILE_ASSERT to
ensure that that is true?

http://codereview.chromium.org/4092/diff/1/4#newcode177
Line 177: for (int index=0; index < kLoops; index++) {
nit: "int index = 0;"

http://codereview.chromium.org/4092/diff/1/2
File base/time_win.cc (right):

http://codereview.chromium.org/4092/diff/1/2#newcode33
Line 33: // message loop waits be have the same resolution that our time
measurements
"...message loop waits [..snip..] have the same resolution that..." right?

http://codereview.chromium.org/4092/diff/1/2#newcode34
Line 34: // do.  Otherwise, WaitForSingleObject(..., 1) will wait up to 15ms.
instead of "wait up to" don't you mean "wait no less than" ?

http://codereview.chromium.org/4092/diff/1/2#newcode247
Line 247: void OnPowerStateChange(base::SystemMonitor* system) {
it looks like we don't have any multi-threading concerns here.  is that right?

http://codereview.chromium.org/4092/diff/1/2#newcode258
Line 258: return;
fix indentation

http://codereview.chromium.org/4092/diff/1/2#newcode313
Line 313: DisableHighResClock();
indentation

http://codereview.chromium.org/4092/diff/1/2#newcode357
Line 357: return static_cast<int64>(now.QuadPart / ticks_per_microsecond_);
so i thought part of the "problem" with QPC was that ticks_per_microsecond_ can
actually change?  e.g., intel speed step.  is that not something to worry about?

http://codereview.chromium.org/4092/diff/1/5
File base/timer.cc (right):

http://codereview.chromium.org/4092/diff/1/5#newcode26
Line 26: counter.Add((int)timer_task->delay_.InMilliseconds());
nit: static_cast<int>(...)

[Mike Belshe, 2008-09-25 23:56:30]

http://codereview.chromium.org/4092/diff/1/3
File base/time_unittest.cc (right):

http://codereview.chromium.org/4092/diff/1/3#newcode110
Line 110: EXPECT_GE(delta.InMicroseconds(), 9000);
On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> can't we expect better precision in the microseconds case?

Yeah, we might be able to get to 9500us.  But I didn't think it was worth it.

Actually, I could undo  this change, since I didn't end up changing the timer
here.

http://codereview.chromium.org/4092/diff/1/3#newcode120
Line 120: EXPECT_GE(delta.InMicroseconds(), 10000);
On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> the problem described above for TimeTicks::Deltas doesn't exist here as well? 
i
> would have thought that it could since HighResNow == Now on some
architectures.

Yes, you're right.  I had it that way, but erroneously convinced myself I didn't
need it.  I just reproed this.

http://codereview.chromium.org/4092/diff/1/4
File base/time_unittest_win.cc (right):

http://codereview.chromium.org/4092/diff/1/4#newcode7
Line 7: #include <mmsystem.h>
On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> can these headers be sorted alphabetically per the style guide?

Not quite; windows.h needs to be first.  I moved up mmsystem.h

http://codereview.chromium.org/4092/diff/1/4#newcode48
Line 48: EXPECT_GE(milliseconds, 0);
I'll add the comment.

Actually, it is because its such a tight loop it can be measured
sub-millisecond.  I'm not sure why this hasn't hit us in the past, actually.

On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> I assume that this change was done because InMilliseconds rounds down?  Might
be
> worth a comment here.

http://codereview.chromium.org/4092/diff/1/4#newcode168
Line 168: { reinterpret_cast<TestFunc>(Time::Now), "Time::Now" },
Heh.  You have good eyes on my cheat.  The compile assert is a good one.

On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> ugly, but ok.  sizeof(Time) == sizeof(TimeTicks).  can you use COMPILE_ASSERT
to
> ensure that that is true?

http://codereview.chromium.org/4092/diff/1/2
File base/time_win.cc (right):

http://codereview.chromium.org/4092/diff/1/2#newcode247
Line 247: void OnPowerStateChange(base::SystemMonitor* system) {
On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> it looks like we don't have any multi-threading concerns here.  is that right?

I don't think so.  I have a separate CL to make accesses to the observer_list
(AddObserver, RemoveObserver, and Notifications) thread safe though.

http://codereview.chromium.org/4092/diff/1/2#newcode357
Line 357: return static_cast<int64>(now.QuadPart / ticks_per_microsecond_);
On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> so i thought part of the "problem" with QPC was that ticks_per_microsecond_
can
> actually change?  e.g., intel speed step.  is that not something to worry
about?

I don't think so:
http://www.gamedev.net/community/forums/topic.asp?topic_id=453522
http://msdn.microsoft.com/en-us/library/ms644905(VS.85).aspx

"The frequency cannot change while the system is running"

http://codereview.chromium.org/4092/diff/1/5
File base/timer.cc (right):

http://codereview.chromium.org/4092/diff/1/5#newcode26
Line 26: counter.Add((int)timer_task->delay_.InMilliseconds());
On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> nit: static_cast<int>(...)

Sorry - this is junk.  Removing this file entirely from the CL.

[Dean McNamee, 2008-09-26 08:22:39]

I didn't do a real review yet, will do it on Monday.  In general I think the
idea looks ok... I don't want to say I'm skeptical, I believe in the interrupt
battery problem.  But on the other hand, it would be nice to have like, data and
stuff?  How much difference is this really making?  How do we know there isn't
another part of Chrome that is effectively voiding out these changes?  Can we
hook up a WattsUp to a laptop and actual measure a power difference?  Can we
time on long a battery lasts or something?

I mean, you know, I don't want to make things overly difficult, but on the other
hand, we have no validation that this actually helps, or how big the problem was
in the first place?

http://codereview.chromium.org/4092/diff/1/4
File base/time_unittest_win.cc (right):

http://codereview.chromium.org/4092/diff/1/4#newcode183
Line 183: (stop - start).InMillisecondsF() * 1000 / kLoops);
Do we really just want printfs in our unit tests?  Seems like a LOG would be
more appropriate.

http://codereview.chromium.org/4092/diff/1/2
File base/time_win.cc (right):

http://codereview.chromium.org/4092/diff/1/2#newcode311
Line 311: base::CPU cpu;
I realize not your job, but I think CPU should move into sys_info.

http://codereview.chromium.org/4092/diff/1/2#newcode357
Line 357: return static_cast<int64>(now.QuadPart / ticks_per_microsecond_);
On 2008/09/25 23:56:30, mbelshe wrote:
> On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> > so i thought part of the "problem" with QPC was that ticks_per_microsecond_
> can
> > actually change?  e.g., intel speed step.  is that not something to worry
> about?
> 
> I don't think so:
> http://www.gamedev.net/community/forums/topic.asp?topic_id=453522
> http://msdn.microsoft.com/en-us/library/ms644905(VS.85).aspx
> 
> "The frequency cannot change while the system is running"
> 
> 
> 
> 


I think this also means that they adjust for the worse case.  So if you have
speed step, take the slowest frequency?  I know oh my laptop, I was 3.5mhz,
while Mike's desktop was like 1000 times that?

So while I don't think the result of this function call will ever change, I
think there could still be a noticeable effect on the result of QPC during
frequency scaling.

http://codereview.chromium.org/4092/diff/1/2#newcode367
Line 367: float ticks_per_microsecond_;  // 0 indicates QPF failed and we're
broken.
We talked about this before, and I suggested maybe a float.  I thin it would be
simpler / faster / more accurate to avoid floating point.  Is there a way we can
just keep the original int64 returned by QPF, instead of scaling it down to
ticks_per_microsecond, leave it in 100 nanos and do the math so we aren't
truncating like before?

http://codereview.chromium.org/4092/diff/1/2#newcode368
Line 368: int64 skew_;  // Skew between lo-res and hi-res clocks (for
debugging).
If it's for debugging, we should only have it when DEBUG?

[Mike Belshe, 2008-09-29 17:46:13]

On 2008/09/26 08:22:39, Dean McNamee wrote:
> I didn't do a real review yet, will do it on Monday.  In general I think the
> idea looks ok... I don't want to say I'm skeptical, I believe in the interrupt
> battery problem.  But on the other hand, it would be nice to have like, data
and
> stuff?  How much difference is this really making?  How do we know there isn't
> another part of Chrome that is effectively voiding out these changes?  Can we
> hook up a WattsUp to a laptop and actual measure a power difference?  Can we
> time on long a battery lasts or something?

I don't have a good way to measure this, but if we trust the Intel report, here
are some thoughts.
http://software.intel.com/en-us/articles/cpu-power-utilization-on-intel-archi...

In these tests, they measured a 11% to 28% increase in CPU usage when increasing
the timer rate from 10ms to 1ms.

My laptop battery is rated for 10.8V/7.2A*h.  If the CPU were the only drain on 
the battery, Intel says it uses 1.1W @10ms and 1.24W @1ms timers.  This 11%
difference would equate to just about 8 hours of battery life.  Of course, there
are additional drains on battery which are much larger....

> I mean, you know, I don't want to make things overly difficult, but on the
other
> hand, we have no validation that this actually helps, or how big the problem
was
> in the first place?

I think the Intel report is good?

> 
> http://codereview.chromium.org/4092/diff/1/4
> File base/time_unittest_win.cc (right):
> 
> http://codereview.chromium.org/4092/diff/1/4#newcode183
> Line 183: (stop - start).InMillisecondsF() * 1000 / kLoops);
> Do we really just want printfs in our unit tests?  Seems like a LOG would be
> more appropriate.

I thought the printfs were nice because you can run these tests on any machine
and see some characteristics of the machine.  I have seen other tests which use
stdio, but I don't know if we have a policy on this?

> 
> http://codereview.chromium.org/4092/diff/1/2
> File base/time_win.cc (right):
> 
> http://codereview.chromium.org/4092/diff/1/2#newcode311
> Line 311: base::CPU cpu;
> I realize not your job, but I think CPU should move into sys_info.

Yeah, maybe that would be a good idea.  I didn't think of this when I was doing
it...  I filed a bug (2956) on that.  I hope you agree its not hi pri?

 
> http://codereview.chromium.org/4092/diff/1/2#newcode357
> Line 357: return static_cast<int64>(now.QuadPart / ticks_per_microsecond_);
> On 2008/09/25 23:56:30, mbelshe wrote:
> > On 2008/09/25 22:54:51, Darin Fisher (Google) wrote:
> > > so i thought part of the "problem" with QPC was that
ticks_per_microsecond_
> > can
> > > actually change?  e.g., intel speed step.  is that not something to worry
> > about?
> > 
> > I don't think so:
> > http://www.gamedev.net/community/forums/topic.asp?topic_id=453522
> > http://msdn.microsoft.com/en-us/library/ms644905(VS.85).aspx
> > 
> > "The frequency cannot change while the system is running"
> > 
> > 
> > 
> > 
> 
> 
> I think this also means that they adjust for the worse case.  So if you have
> speed step, take the slowest frequency?  I know oh my laptop, I was 3.5mhz,
> while Mike's desktop was like 1000 times that?

Yup.  BTW - you can run the tests on any machine and it will print the 
frequency for you :-) (I confirmed your 3.5MHz on my laptop too)


> So while I don't think the result of this function call will ever change, I
> think there could still be a noticeable effect on the result of QPC during
> frequency scaling.

Yes; different machines will have vastly different rollovers and resolution.

> 
> http://codereview.chromium.org/4092/diff/1/2#newcode367
> Line 367: float ticks_per_microsecond_;  // 0 indicates QPF failed and we're
> broken.
> We talked about this before, and I suggested maybe a float.  I thin it would
be
> simpler / faster / more accurate to avoid floating point.  Is there a way we
can
> just keep the original int64 returned by QPF, instead of scaling it down to
> ticks_per_microsecond, leave it in 100 nanos and do the math so we aren't
> truncating like before?

I actually tested this; I really was trying to make QPC viable; but the effect
on
performance due to float math was radically dwarfed by the time to call QPC
itself.
So, we might be able to come up with something here; but the QPC perf will still
be very slow compared to timeGetTime().

> 
> http://codereview.chromium.org/4092/diff/1/2#newcode368
> Line 368: int64 skew_;  // Skew between lo-res and hi-res clocks (for
> debugging).
> If it's for debugging, we should only have it when DEBUG?

Yes, you are right.