Fix logic on high Windows resolution timer and have

Fix logic on high Windows resolution timer and have
two possible period values for timeBeginPeriod and timeEndPeriod.

Currently while on battery we disable calls to timeBeginPeriod
which make the windows timers have 15ms resolution.

This change makes it so when EnableHighResolutionTimer(true) which
is on AC power the timer is 1ms and EnableHighResolutionTimer(false)
is 4ms.

This should provide significant power savings while meeting some
timer resolution requirements needed by the GPU compositor.

But also this CL fixes the following:

EnableHighResolutionTimer() and ActivateHighResolutionTimer() are
pretty broken. This CL fixes most issues:

1- The existing logic fails to account that EnableHighResolutionTimer
can be called while the browser is running

2- All related functions need to be thread safe.

3- ActivateHighResolutionTimer was buggy.

BUG=153139
Committed: https://crrev.com/be8f40e67f300e9452cfabb3ad594d907cfaa947
Cr-Commit-Position: refs/heads/master@{#292094}

[cpu_(ooo_6.6-7.5), 2014-08-26 03:28:35]

cpu@chromium.org changed reviewers:
+ jamesr@chromium.org, thakis@chromium.org

[cpu_(ooo_6.6-7.5), 2014-08-26 03:35:46]

ptal.

[jamesr, 2014-08-26 05:03:17]

Can you re-enable HiResTimerManagerTest.DISABLED_ToggleOnOff in
base/timer/hi_res_timer_manager_unittest.cc and close http://crbug.com/114048 as
well?  Or if this doesn't fix the flake just delete that file and close the bug
anyway?

https://codereview.chromium.org/489793003/diff/1/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/489793003/diff/1/base/time/time_win.cc#newcode91
base/time/time_win.cc:91: const int kMinTimerIntervalLowResMs = 4;
since these 2 vars always go into a UINT can we just make the type uint32_t?

https://codereview.chromium.org/489793003/diff/1/base/time/time_win.cc#newcod...
base/time/time_win.cc:182: if (enable) {
let's say we're in high resolution mode with no active high res tasks (so no
pending timeBeginPeriod calls) and have two threads.  Thread A sees we're on
battery now and is coming in to call EnableHRTimer(false).  Thread B has a task
that wants high res timers and is coming in to call ActivateHRTimer(true).

1.) A and B race on the g_high_res_lock and B wins
2.) B executes lines 199-203 under the lock and releases is

now g_high_res_lock is released, g_high_res_timer_enabled is still true,
g_high_res_timer_count is 1.  In thread B, the locals period and high_res_count
are 1 and 1, respectively and activating is true

3.) Thread A aquires the lock and executes lines 175-180

now g_high_res_lock is released, g_high_res_timer_enabled is false,
g_high_res_timer_count is 1.   In thread A, local enable is set to false

4.) Thread A executes lines 182-186  enable is false so we call timeEndPeriod(1)

5.) Thread B executes lines 205-207. activating is true, high_res_count is 1 and
period is 1 so we call to ::timeBeginPeriod(1)

6.) Thread A executes line 187 and calls timeBeginPeriod(4)

now we're a bit buggered.  we've mismatched calls to
timeBeginPeriod/timeEndPeriod by calling timeEndPeriod(1) without a
corresponding timeBeginPeriod() and we've called timeBeginPeriod() twice in a
row.  If things go as planned we'll call timeEndPeriod(4) later on when we
de-activate, but we'll leak a timeBeginPeriod(1)

I think the lock has to surround the time{Begin,End}Period() calls since we're
communicating data into the win32 api and need to make sure that order is
consistent  It's sadface but trueface.

[cpu_(ooo_6.6-7.5), 2014-08-26 19:43:38]

Sadface indeed. Went back to locking the whole call.

Adding the disabled test with some minor rework.

PTAL.

[jamesr, 2014-08-26 19:47:40]

lgtm

https://codereview.chromium.org/489793003/diff/20001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/489793003/diff/20001/base/time/time_win.cc#ne...
base/time/time_win.cc:180: if (enable) {
this bit might be worth a comment explaining the state transitions. rough
attempt:

Since g_high_res_timer_count is nonzero, an earlier call to
ActivateHighResolutionTimer(true) must have called timeBeginPeriod(). If we were
in high resolution mode beforehand, it would have called
timeBeginPeriod(kMinTimerIntervalHighResMs) otherwise it called
timeBeginPeriod(kMinTimerIntervalLowResMs). We need to end that period and start
a new one with the new desired resolution. A future call to
ActivateHighResolutionTimer(false) will end this period.

https://codereview.chromium.org/489793003/diff/20001/base/timer/hi_res_timer_...
File base/timer/hi_res_timer_manager_unittest.cc (right):

https://codereview.chromium.org/489793003/diff/20001/base/timer/hi_res_timer_...
base/timer/hi_res_timer_manager_unittest.cc:18: // The power monitor creates
Window to receive power notifications from
"creates Window" -> "creates a Window"

[Nico, 2014-08-26 20:15:59]

rs-lgtm

https://codereview.chromium.org/489793003/diff/20001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/489793003/diff/20001/base/time/time_win.cc#ne...
base/time/time_win.cc:97: base::LazyInstance<base::Lock> g_high_res_lock =
LAZY_INSTANCE_INITIALIZER;
Can you make this ::Leaky?

[cpu_(ooo_6.6-7.5), 2014-08-27 01:29:32]

The CQ bit was checked by cpu@chromium.org

[commit-bot: I haz the power, 2014-08-27 01:30:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cpu@chromium.org/489793003/40001

[commit-bot: I haz the power, 2014-08-27 03:00:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-27 03:58:55]

Committed patchset #3 (40001) as 3f4fda54db1983799fe12606b0d154f8f4d816eb

[cpu_(ooo_6.6-7.5), 2014-08-28 01:14:33]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2014-09-10 02:49:37]

Patchset 3 (id:??) landed as
https://crrev.com/be8f40e67f300e9452cfabb3ad594d907cfaa947
Cr-Commit-Position: refs/heads/master@{#292094}