Change chrome from statically enabling high resolution timers on windows...

Change chrome from statically enabling high resolution timers on windows
to enabling them dynamically - only when the application really needs
them.

I am working on some test cases for this, and will add them.  But wanted
to send out the concept for review.

In this implementation, I modify the message loop to detect when the
application has requested high resolution timers.  Note that there are
multiple MessageLoops active in a single process.  After a period of
time, we simply shut it off again.    We could have set a timer or
kept a count of active timers, or any number of more complex algorithms.
But I think this algorithm is very simple and good enough.  If an
application continues needing high resolution timers for more than 1s,
we'll turn the high-resolution timers back on again.

One last change - since we've implemented the clamp at 4ms, there isn't
a lot of point to our use of 1ms for timeBeginPeriod.  I've modified
that to 2 (which is half of 4ms, our target minimal interval).

BUG=46531
TEST=MessageLoop.HighResolutionTimers



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

[Mike Belshe, 2010-06-28 00:31:32]

Note:  I'm happy to change the OS_WIN ifdef's.  I did consider deliberately how
to do it best.  Let me know what you think.

Thanks!

[jar (doing other things), 2010-06-28 05:52:35]

I'm not completely clear on the usage scenarios, but I'm suspicious about
possible races.  Adding DCHECKs about what thread might be allowed to call this
could help clarify things.

http://codereview.chromium.org/2822035/diff/8001/9001
File base/message_loop.cc (right):

http://codereview.chromium.org/2822035/diff/8001/9001#newcode292
base/message_loop.cc:292: delay_ms < Time::kMinLowResolutionThresholdMs;
It is interesting that this is turned on when a user requests a short delay...
but I'd alse expect a hi-res timer to be valuable when (for example) a 17ms
delay was consistently requested, but this was being "rounded" to the nearest
15ms interval.  Are all significant uses of hi-res really folks that request a
shorter interval?

http://codereview.chromium.org/2822035/diff/8001/9002
File base/message_loop.h (right):

http://codereview.chromium.org/2822035/diff/8001/9002#newcode419
base/message_loop.h:419: // When we go into high resolution timer mode, we take
a least for 1s before
nit: Typo in sentence?  "...we take a least for 1s ..."

http://codereview.chromium.org/2822035/diff/8001/9002#newcode463
base/message_loop.h:463: base::Time high_resolution_timer_expiration_;
TimeTicks may be preferable, as they don't depend on a user changing the
time-of-day.

http://codereview.chromium.org/2822035/diff/8001/9004
File base/time.h (right):

http://codereview.chromium.org/2822035/diff/8001/9004#newcode397
base/time.h:397: static bool high_resolution_timer_active_;
Given a static, I was wondering how this will deal with two messages loops, each
activating and deactivating the hi-res?  I would expect that the first to
deactivate will will override??  Is this not a concern?

http://codereview.chromium.org/2822035/diff/8001/9005
File base/time_win.cc (right):

http://codereview.chromium.org/2822035/diff/8001/9005#newcode158
base/time_win.cc:158: if (!enable && high_resolution_timer_active_)
Unless you have assurances that the enablement will take place on the same
thread as the activation, this looks a bit racy.  I'm not convinced that when an
attempt is made to disable, that a race won't leave the timer activated
permanently (sometimes).

http://codereview.chromium.org/2822035/diff/8001/9005#newcode173
base/time_win.cc:173: // timer clamp, we use 2ms here.
I don't understand the logic in selecting 2ms (half of the clamp??)

[Mike Belshe, 2010-06-28 18:14:46]

http://codereview.chromium.org/2822035/diff/8001/9001
File base/message_loop.cc (right):

http://codereview.chromium.org/2822035/diff/8001/9001#newcode292
base/message_loop.cc:292: delay_ms < Time::kMinLowResolutionThresholdMs;
You're right, it should be anyone with a resolution < 2 * 15.6ms.  Let me test.

On 2010/06/28 05:52:35, jar wrote:
> It is interesting that this is turned on when a user requests a short delay...
> but I'd alse expect a hi-res timer to be valuable when (for example) a 17ms
> delay was consistently requested, but this was being "rounded" to the nearest
> 15ms interval.  Are all significant uses of hi-res really folks that request a
> shorter interval?

http://codereview.chromium.org/2822035/diff/8001/9002
File base/message_loop.h (right):

http://codereview.chromium.org/2822035/diff/8001/9002#newcode419
base/message_loop.h:419: // When we go into high resolution timer mode, we take
a least for 1s before
On 2010/06/28 05:52:35, jar wrote:
> nit: Typo in sentence?  "...we take a least for 1s ..."

Done.

http://codereview.chromium.org/2822035/diff/8001/9002#newcode463
base/message_loop.h:463: base::Time high_resolution_timer_expiration_;
On 2010/06/28 05:52:35, jar wrote:
> TimeTicks may be preferable, as they don't depend on a user changing the
> time-of-day.

Done.

http://codereview.chromium.org/2822035/diff/8001/9004
File base/time.h (right):

http://codereview.chromium.org/2822035/diff/8001/9004#newcode397
base/time.h:397: static bool high_resolution_timer_active_;
On 2010/06/28 05:52:35, jar wrote:
> Given a static, I was wondering how this will deal with two messages loops,
each
> activating and deactivating the hi-res?  I would expect that the first to
> deactivate will will override??  Is this not a concern?

I've switched this to be more like the windows api.  In windows each call to
timeBeginPeriod(x) must have its own corresponding call to timeEndPeriod(x). 
So, if 3 threads each enable, 3 threads must each disable.  This eliminated both
the race condition and also this concern by making each thread always enable.  I
changed the Enable API to not deactivate automatically so that this still works.

http://codereview.chromium.org/2822035/diff/8001/9005
File base/time_win.cc (right):

http://codereview.chromium.org/2822035/diff/8001/9005#newcode158
base/time_win.cc:158: if (!enable && high_resolution_timer_active_)
OK - thanks.  See previous comment.  

On 2010/06/28 05:52:35, jar wrote:
> Unless you have assurances that the enablement will take place on the same
> thread as the activation, this looks a bit racy.  I'm not convinced that when
an
> attempt is made to disable, that a race won't leave the timer activated
> permanently (sometimes).

http://codereview.chromium.org/2822035/diff/8001/9005#newcode173
base/time_win.cc:173: // timer clamp, we use 2ms here.
I played with this more, and yes, I don't like the granularity of it.  Switched
back to 1.

On 2010/06/28 05:52:35, jar wrote:
> I don't understand the logic in selecting 2ms (half of the clamp??)

[jar (doing other things), 2010-06-29 00:37:05]

I like the logic change.  

I had a few more thoughts and fears listed below.

http://codereview.chromium.org/2822035/diff/4002/10
File base/message_loop.cc (right):

http://codereview.chromium.org/2822035/diff/4002/10#newcode287
base/message_loop.cc:287: if (high_resolution_timer_expiration_.is_null()) {
This logic will (when we regularly need hi-res) cycle in-and-out of hi-res mode.
 IMO, it would be nicer to stay in hi-res mode by extending the expiration
whenever we need_high_res_timers, even if we already have an expiration.
WDYT?

To be precise.... you might not need to extend it (with the costly call to
Now()), but you could use a boolean recently_needed_hi_res that would be checked
when we finally contemplate exiting hi-res.

http://codereview.chromium.org/2822035/diff/4002/10#newcode303
base/message_loop.cc:303: if (base::TimeTicks::Now() >
high_resolution_timer_expiration_) {
It *might* be nice to schedule an event to disable hi-res, or at least consider
disabling it, rather than polling each time we post a delayed task.  There is a
tiny chance that this polling would have a perf impact.

It is also interesting that we only disable hi-res when we schedule another
timer event.  Maybe this happens more than often enough on some threads.... but
a lightly used thread could leave hi-res enabled for a loooong time.

http://codereview.chromium.org/2822035/diff/4002/13
File base/time.h (right):

http://codereview.chromium.org/2822035/diff/4002/13#newcode266
base/time.h:266: // activations.
This is nice, and avoids all my race concerns (assuming we eventually turn off
hi-res mode after a second).

http://codereview.chromium.org/2822035/diff/4002/13#newcode392
base/time.h:392: // Flags about the current state of the high resolution timer.
I think you just track one state variable now.

[Mike Belshe, 2010-06-29 01:53:30]

http://codereview.chromium.org/2822035/diff/4002/10
File base/message_loop.cc (right):

http://codereview.chromium.org/2822035/diff/4002/10#newcode287
base/message_loop.cc:287: if (high_resolution_timer_expiration_.is_null()) {
On 2010/06/29 00:37:05, jar wrote:
> This logic will (when we regularly need hi-res) cycle in-and-out of hi-res
mode.
>  IMO, it would be nicer to stay in hi-res mode by extending the expiration
> whenever we need_high_res_timers, even if we already have an expiration.
> WDYT?

Of course, that is what I wanted to do.  But we're deep in the guts of the
message loop.  Doing something computationally cheap (which is what we have
here) seems preferable.

Also - I'm optimizing for the case where we don't need hi-res timers, which is
the common case.
> 
> To be precise.... you might not need to extend it (with the costly call to
> Now()), but you could use a boolean recently_needed_hi_res that would be
checked
> when we finally contemplate exiting hi-res.
>

http://codereview.chromium.org/2822035/diff/4002/10#newcode303
base/message_loop.cc:303: if (base::TimeTicks::Now() >
high_resolution_timer_expiration_) {
On 2010/06/29 00:37:05, jar wrote:
> It *might* be nice to schedule an event to disable hi-res, or at least
consider
> disabling it, rather than polling each time we post a delayed task.  There is
a
> tiny chance that this polling would have a perf impact.

Scheduling a task adds a bunch of complexity, but the real question is the
lifecycle of the new task.  To keep it simple, I thought this approach is far
better.  I've reduced the per-timer overhead to 1 check (
high_resolution_timer_expiration.is_null()) and that seems pretty cheap.
> 
> It is also interesting that we only disable hi-res when we schedule another
> timer event.  Maybe this happens more than often enough on some threads....
but
> a lightly used thread could leave hi-res enabled for a loooong time.

Our app simply doesn't work that way.  But yes, it is theoretically possible.

http://codereview.chromium.org/2822035/diff/4002/13
File base/time.h (right):

http://codereview.chromium.org/2822035/diff/4002/13#newcode266
base/time.h:266: // activations.
On 2010/06/29 00:37:05, jar wrote:
> This is nice, and avoids all my race concerns (assuming we eventually turn off
> hi-res mode after a second).

Cool.

http://codereview.chromium.org/2822035/diff/4002/13#newcode392
base/time.h:392: // Flags about the current state of the high resolution timer.
On 2010/06/29 00:37:05, jar wrote:
> I think you just track one state variable now.

Good catch, thanks.

[jar (doing other things), 2010-06-29 04:19:56]

I have some mild concerns, listed below, but expect that this is probably an
excellent improvement over current status quo. and I know you're about to go on
vacation  ...soooo....

LGTM

One last thing you might comment on is how this acts during thread shutdown.  I
don't think I saw code in the message loop destructor.... but perchance by the
time threads of this sort are shutting down, the whole process is soon to
terminate, and that will handle restoring low-res timers.
If you think this might be an issue, you're welcome to file a bug and deal with
it when you get back.

http://codereview.chromium.org/2822035/diff/4002/10
File base/message_loop.cc (right):

http://codereview.chromium.org/2822035/diff/4002/10#newcode287
base/message_loop.cc:287: if (high_resolution_timer_expiration_.is_null()) {
I don't know if it makes sense... but when the high-res timer is active, then
I'm more concerned with being more efficient, perhaps because I *imagine* that
the CPU will be stressed to so do all its work.  I don't know if we have per
tests that really exercise this question... and I'm not (to be honest) sure of
which case is better served by optimizing.

On 2010/06/29 01:53:30, Mike Belshe wrote:
> On 2010/06/29 00:37:05, jar wrote:
> > This logic will (when we regularly need hi-res) cycle in-and-out of hi-res
> mode.
> >  IMO, it would be nicer to stay in hi-res mode by extending the expiration
> > whenever we need_high_res_timers, even if we already have an expiration.
> > WDYT?
> 
> Of course, that is what I wanted to do.  But we're deep in the guts of the
> message loop.  Doing something computationally cheap (which is what we have
> here) seems preferable.
> 
> Also - I'm optimizing for the case where we don't need hi-res timers, which is
> the common case.
> > 
> > To be precise.... you might not need to extend it (with the costly call to
> > Now()), but you could use a boolean recently_needed_hi_res that would be
> checked
> > when we finally contemplate exiting hi-res.
> > 
> 
>

http://codereview.chromium.org/2822035/diff/4002/10#newcode303
base/message_loop.cc:303: if (base::TimeTicks::Now() >
high_resolution_timer_expiration_) {
On 2010/06/29 01:53:30, Mike Belshe wrote:
> On 2010/06/29 00:37:05, jar wrote:
> > It *might* be nice to schedule an event to disable hi-res, or at least
> consider
> > disabling it, rather than polling each time we post a delayed task.  There
is
> a
> > tiny chance that this polling would have a perf impact.
> 
> Scheduling a task adds a bunch of complexity, but the real question is the
> lifecycle of the new task.  To keep it simple, I thought this approach is far
> better.  I've reduced the per-timer overhead to 1 check (
> high_resolution_timer_expiration.is_null()) and that seems pretty cheap.
> > 
> > It is also interesting that we only disable hi-res when we schedule another
> > timer event.  Maybe this happens more than often enough on some threads....
> but
> > a lightly used thread could leave hi-res enabled for a loooong time.
> 
> Our app simply doesn't work that way.  But yes, it is theoretically possible.

I'm not sure if this is correct... but it is plausible that JS code could
request a short interval, and then that main renderer thread *might* not have to
deal with any events fro a long while.  That *might* be a plausible example of
this issue.  Then again, perhaps all such threads regularly have timer events
for "other stuff."  I'm not sure (but you probably know this far better than I
do).
>

[Mike Belshe, 2010-06-29 04:29:14]

Thanks, Jim!

On 2010/06/29 04:19:56, jar wrote:
> I have some mild concerns, listed below, but expect that this is probably an
> excellent improvement over current status quo. and I know you're about to go
on
> vacation  ...soooo....
> 
> LGTM
> 
> One last thing you might comment on is how this acts during thread shutdown. 
I
> don't think I saw code in the message loop destructor.... but perchance by the
> time threads of this sort are shutting down, the whole process is soon to
> terminate, and that will handle restoring low-res timers.
> If you think this might be an issue, you're welcome to file a bug and deal
with
> it when you get back.

On process shutdown, the OS takes care of the rest.  I've verified this many
times, so I think it is okay.



> 
> http://codereview.chromium.org/2822035/diff/4002/10
> File base/message_loop.cc (right):
> 
> http://codereview.chromium.org/2822035/diff/4002/10#newcode287
> base/message_loop.cc:287: if (high_resolution_timer_expiration_.is_null()) {
> I don't know if it makes sense... but when the high-res timer is active, then
> I'm more concerned with being more efficient, perhaps because I *imagine* that
> the CPU will be stressed to so do all its work.  I don't know if we have per
> tests that really exercise this question... and I'm not (to be honest) sure of
> which case is better served by optimizing.
> 
> On 2010/06/29 01:53:30, Mike Belshe wrote:
> > On 2010/06/29 00:37:05, jar wrote:
> > > This logic will (when we regularly need hi-res) cycle in-and-out of hi-res
> > mode.
> > >  IMO, it would be nicer to stay in hi-res mode by extending the expiration
> > > whenever we need_high_res_timers, even if we already have an expiration.
> > > WDYT?
> > 
> > Of course, that is what I wanted to do.  But we're deep in the guts of the
> > message loop.  Doing something computationally cheap (which is what we have
> > here) seems preferable.
> > 
> > Also - I'm optimizing for the case where we don't need hi-res timers, which
is
> > the common case.
> > > 
> > > To be precise.... you might not need to extend it (with the costly call to
> > > Now()), but you could use a boolean recently_needed_hi_res that would be
> > checked
> > > when we finally contemplate exiting hi-res.
> > > 
> > 
> >
> 
> http://codereview.chromium.org/2822035/diff/4002/10#newcode303
> base/message_loop.cc:303: if (base::TimeTicks::Now() >
> high_resolution_timer_expiration_) {
> On 2010/06/29 01:53:30, Mike Belshe wrote:
> > On 2010/06/29 00:37:05, jar wrote:
> > > It *might* be nice to schedule an event to disable hi-res, or at least
> > consider
> > > disabling it, rather than polling each time we post a delayed task.  There
> is
> > a
> > > tiny chance that this polling would have a perf impact.
> > 
> > Scheduling a task adds a bunch of complexity, but the real question is the
> > lifecycle of the new task.  To keep it simple, I thought this approach is
far
> > better.  I've reduced the per-timer overhead to 1 check (
> > high_resolution_timer_expiration.is_null()) and that seems pretty cheap.
> > > 
> > > It is also interesting that we only disable hi-res when we schedule
another
> > > timer event.  Maybe this happens more than often enough on some
threads....
> > but
> > > a lightly used thread could leave hi-res enabled for a loooong time.
> > 
> > Our app simply doesn't work that way.  But yes, it is theoretically
possible.
> 
> I'm not sure if this is correct... but it is plausible that JS code could
> request a short interval, and then that main renderer thread *might* not have
to
> deal with any events fro a long while.  That *might* be a plausible example of
> this issue.  Then again, perhaps all such threads regularly have timer events
> for "other stuff."  I'm not sure (but you probably know this far better than I
> do).

The theory is possible.  But I don't see that happening in testing.