High resolution timer fix for Windows

This is jamesr@ code I am landing.

On Windows the message pump code tried to manage the systemwide timer resolution to fire delayed tasks with better than 15ms resolution but it was buggy:

1- A short task that was not followed by any other task will leave the systemwide timer pegged to 1ms

2- After we decided to crank up the timer we would 'lease' the timer for 1 second, for no good reason.

Both issues are detrimental to battery power.

The source of both problems is that we tried to decide with incomplete information. This patch solves that by having 1 bit for each pending task that requires a high resolution timer and a sum of the number of tasks that require high res timers.

BUG=153139
TEST=included here, also see the bug for manual testing.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284625

[cpu_(ooo_6.6-7.5), 2014-07-18 03:29:03]

please take a look.

[darin (slow to review), 2014-07-18 04:07:59]

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.cc:32: // more than 0 and less than 32ms.
why 32ms?

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.cc:44: return (high_res_task_count_ > 0);
nit: unnecessary parens.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.h:71: int high_res_task_count_;
can this number be negative? should we use unsigned math?

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.h:73: // The lock that protects access to
|incoming_queue_|, |message_loop_| and
This lock also protects access to high_res_task_count_ now. The comment that
enumerates all other member variables is sort of silly. The comment should just
say that it protects all members of this class or something like that instead.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:172: did_work = DeletePendingTasks();
notice how we delete some pending tasks here... this means we may not fully
decrement the high res task counter, which means we may leave the high res timer
running, right?

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
File base/message_loop/message_loop_unittest.cc (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_loop_unittest.cc:727: TEST(MessageLoopTest,
HighResolutionTimer) {
it'd be good to test that a MessageLoop with pending tasks at destruction time
doesn't leave us with the high res timer activated.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
File base/message_loop/message_pump.h (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_pump.h:47: virtual void UpdateTimerGranularity() {}
why don't you just use DoIdleWork for this? it is always called before sleeping.

[jamesr, 2014-07-18 04:45:37]

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.cc:32: // more than 0 and less than 32ms.
On 2014/07/18 04:07:58, darin wrote:
> why 32ms?

the old comment justified this by saying that this caps the relative error of
the sleep to <50% (i.e. want a sleep of 33 but get one of 33+15=48 ms).  I'm not
super sure about that but this preserves the intended behavior - it'll be a lot
easier to figure out if and how to tweak this number when the logic works

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:130: pending_high_res_tasks_(false),
s/false/0/

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:145: pending_high_res_tasks_(false),
s/false/0/

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:172: did_work = DeletePendingTasks();
On 2014/07/18 04:07:59, darin wrote:
> notice how we delete some pending tasks here... this means we may not fully
> decrement the high res task counter, which means we may leave the high res
timer
> running, right?

Hmm, true. we should probably keep track of whether this message loop as
activated high res and deactivate if so on destruction

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
File base/message_loop/message_pump.h (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_pump.h:47: virtual void UpdateTimerGranularity() {}
On 2014/07/18 04:07:59, darin wrote:
> why don't you just use DoIdleWork for this? it is always called before
sleeping.

it's also called before not sleeping (i.e. we have an idle work task that posts
more tasks) and we really should only do this when we're really about to sleep.
we could do it in MessageLoop::DoIdleWork just before returning false, but imho
that ties this a little too deeply to how the pump works. updating the
granularity changes the behavior of the sleep call so i like having it be as
close to possible as the sleep call

[darin (slow to review), 2014-07-18 04:55:54]

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
File base/message_loop/message_pump.h (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_pump.h:47: virtual void UpdateTimerGranularity() {}
On 2014/07/18 04:45:37, jamesr wrote:
> On 2014/07/18 04:07:59, darin wrote:
> > why don't you just use DoIdleWork for this? it is always called before
> sleeping.
> 
> it's also called before not sleeping (i.e. we have an idle work task that
posts
> more tasks)

I don't think that's quite right. In that case, DoIdleWork returns true,
and we avoid waiting at all. We always completely drain the work queue of
work that can be done before sleeping, and if we do any work, then we
assume we could plausibly have more work to do.


> and we really should only do this when we're really about to sleep.
> we could do it in MessageLoop::DoIdleWork just before returning false

^^^ Yes, just before returning false seems like the right time.


>, but imho
> that ties this a little too deeply to how the pump works. updating the
> granularity changes the behavior of the sleep call so i like having it be as
> close to possible as the sleep call

I don't think it is tying us too closely. Looking at the comment for
DoIdleWork, it seems to describe exactly what we are looking for here...
notification that the pump is about to sleep. The comment describing
UpdateTimerGranularity says almost exactly the same thing.

[jamesr, 2014-07-18 06:18:33]

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
File base/message_loop/message_pump.h (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_pump.h:47: virtual void UpdateTimerGranularity() {}
On 2014/07/18 04:55:54, darin wrote:
> On 2014/07/18 04:45:37, jamesr wrote:
> > On 2014/07/18 04:07:59, darin wrote:
> > > why don't you just use DoIdleWork for this? it is always called before
> > sleeping.
> > 
> > it's also called before not sleeping (i.e. we have an idle work task that
> posts
> > more tasks)
> 
> I don't think that's quite right. In that case, DoIdleWork returns true,
> and we avoid waiting at all. We always completely drain the work queue of
> work that can be done before sleeping, and if we do any work, then we
> assume we could plausibly have more work to do.
> 
> 
> > and we really should only do this when we're really about to sleep.
> > we could do it in MessageLoop::DoIdleWork just before returning false
> 
> ^^^ Yes, just before returning false seems like the right time.
> 
> 
> >, but imho
> > that ties this a little too deeply to how the pump works. updating the
> > granularity changes the behavior of the sleep call so i like having it be as
> > close to possible as the sleep call
> 
> I don't think it is tying us too closely. Looking at the comment for
> DoIdleWork, it seems to describe exactly what we are looking for here...
> notification that the pump is about to sleep. The comment describing
> UpdateTimerGranularity says almost exactly the same thing.

Good points.  I agree doing this at the end of DoIdleWork() would be better.

[cpu_(ooo_6.6-7.5), 2014-07-18 22:53:10]

code update PTAL.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
File base/message_loop/incoming_task_queue.cc (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.cc:32: // more than 0 and less than 32ms.
Added the math example to the comment.

This is an interesting knob to tweak, if we can get appropriate / reliable power
metrics. Audio tight audio timelines are around 6ms and thigh video timelines
are 16ms.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.cc:44: return (high_res_task_count_ > 0);
On 2014/07/18 04:07:58, darin wrote:
> nit: unnecessary parens.

Done.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.h:71: int high_res_task_count_;
On 2014/07/18 04:07:58, darin wrote:
> can this number be negative? should we use unsigned math?

I rather use signed math to make sure we count down to zero correctly, there is
a CHECK() for that on decrement.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incomi...
base/message_loop/incoming_task_queue.h:73: // The lock that protects access to
|incoming_queue_|, |message_loop_| and
On 2014/07/18 04:07:58, darin wrote:
> This lock also protects access to high_res_task_count_ now. The comment that
> enumerates all other member variables is sort of silly. The comment should
just
> say that it protects all members of this class or something like that instead.

Done.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:130: pending_high_res_tasks_(false),
On 2014/07/18 04:45:37, jamesr wrote:
> s/false/0/

Done.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:145: pending_high_res_tasks_(false),
On 2014/07/18 04:45:37, jamesr wrote:
> s/false/0/

Done.

https://codereview.chromium.org/395913006/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:172: did_work = DeletePendingTasks();
added such variable.

[cpu_(ooo_6.6-7.5), 2014-07-18 23:00:06]

actually hold on the review of a couple of minutes.

[cpu_(ooo_6.6-7.5), 2014-07-18 23:12:03]

Ok, ready for review, delta from patch 3 is a valid review continuation point.

[darin (slow to review), 2014-07-19 04:06:57]

LGTM

https://codereview.chromium.org/395913006/diff/100001/base/message_loop/incom...
File base/message_loop/incoming_task_queue.h (right):

https://codereview.chromium.org/395913006/diff/100001/base/message_loop/incom...
base/message_loop/incoming_task_queue.h:73: // The lock that protects access to
the members of this class.
micro-nit: How about listing the lock as the first member variable?

[cpu_(ooo_6.6-7.5), 2014-07-20 01:25:55]

I am going to have to remove the call to
Time::IsHighResolutionTimerInUse  from the test. It is not a reliable signal.

[darin (slow to review), 2014-07-20 18:48:12]

On 2014/07/20 01:25:55, cpu wrote:
> I am going to have to remove the call to
> Time::IsHighResolutionTimerInUse  from the test. It is not a reliable signal.

OK

[cpu_(ooo_6.6-7.5), 2014-07-21 22:13:12]

The CQ bit was checked by cpu@chromium.org

[commit-bot: I haz the power, 2014-07-21 22:15:04]

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

[commit-bot: I haz the power, 2014-07-21 23:31:03]

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

[commit-bot: I haz the power, 2014-07-22 00:53:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 00:53:03]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[cpu_(ooo_6.6-7.5), 2014-07-22 01:51:47]

The CQ bit was checked by cpu@chromium.org

[commit-bot: I haz the power, 2014-07-22 01:53:21]

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

[commit-bot: I haz the power, 2014-07-22 02:49:25]

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

[commit-bot: I haz the power, 2014-07-22 04:30:01]

Change committed as 284625

[Kunihiko Sakamoto, 2014-07-22 11:30:22]

A revert of this CL has been created in
https://codereview.chromium.org/407073004/ by ksakamoto@chromium.org.

The reason for reverting is: This patch seems to make following browser_tests
flakey

PPAPINaClNewlibTest.Graphics2D_FlushOffscreenUpdate
NetInternalsTest.netInternalsHSTSViewAddOverwrite
NetInternalsTest.netInternalsHSTSViewAddDelete
NetInternalsTest.netInternalsHSTSViewAddTwice

http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%282%29/builds...
http://build.chromium.org/p/chromium.win/builders/XP%20Tests%20%281%29/builds...
http://build.chromium.org/p/chromium.win/builders/Vista%20Tests%20%281%29/bui...
.

[jar (doing other things), 2014-08-15 19:09:20]

Per off-line discussion with Carlos, we should get away with *never* setting the
system-wide high res timer.  At the same time, we should still surprisingly be
able to have short timer event durations serviced punctually!  

This should have great system-wide battery saving impact.

The basic trick is to use a fancy "pause thread" instruction that Carlos found
for those "brief sleep periods," and never change the system timer ;-).


tl;dr;

a) Always leave system wide time alone (default 15ms?)

b) When we need to "sleep" in a message loop to wait for a timer event, only use
the current logic for "long" sleep (over 15-30ms? over
2*current_timer_interval?)

c) For "short" sleeps, when a high-res-task will need to fire in under 15-30ms,
use the fancy thread_pause instruction to pause the thread [critical note: The
thread reportedly wouldn't have woken any sooner for an async event anyway, so
don't worry that we'll pause past them!]

d) When deciding long vs short, look to see when the next hi-res-event will fire
(don't worry about low-res-timer-events that may be coming sooner).

...for efficient implementation...

With the high_res boolean added to each task (as in this CL), a second (usually
tiny or empty) priority queue can be maintained (perhaps with time-stamps only)
which holds only high-res-timer tasks.  This will clarify the "proper" sleep
time until the next hi-res task time (no reason to wake early for low-res timer
requests!).

Said second-queue will require (negligible) cost to access the head of the queue
(re: to ask "how long until the next hi-res event??)

There would then be no reason to maintain the counter of "how many hi-res-tasks
are pending," as it is also effectively the size of the second queue.