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

Issue 395913006: High resolution timer fix for Windows (Closed)

Created:
6 years, 5 months ago by cpu_(ooo_6.6-7.5)
Modified:
6 years, 4 months ago
CC:
chromium-reviews, erikwright+watch_chromium.org, sadrul, tonyg
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : cleanup #

Patch Set 3 : more cleanup #

Total comments: 21

Patch Set 4 : real fixes for review #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+87 lines, -101 lines) Patch
M base/message_loop/incoming_task_queue.h View 1 2 3 2 chunks +10 lines, -10 lines 1 comment Download
M base/message_loop/incoming_task_queue.cc View 1 2 3 4 chunks +25 lines, -52 lines 0 comments Download
M base/message_loop/message_loop.h View 1 2 3 3 chunks +9 lines, -5 lines 0 comments Download
M base/message_loop/message_loop.cc View 1 2 3 7 chunks +27 lines, -5 lines 0 comments Download
M base/message_loop/message_loop_unittest.cc View 1 2 3 1 chunk +7 lines, -17 lines 0 comments Download
M base/message_loop/message_pump.h View 1 2 3 1 chunk +2 lines, -1 line 0 comments Download
M base/pending_task.h View 1 2 2 chunks +3 lines, -3 lines 0 comments Download
M base/pending_task.cc View 1 2 2 chunks +4 lines, -8 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
cpu_(ooo_6.6-7.5)
please take a look.
6 years, 5 months ago (2014-07-18 03:29:03 UTC) #1
darin (slow to review)
https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incoming_task_queue.cc File base/message_loop/incoming_task_queue.cc (right): https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incoming_task_queue.cc#newcode32 base/message_loop/incoming_task_queue.cc:32: // more than 0 and less than 32ms. why ...
6 years, 5 months ago (2014-07-18 04:07:59 UTC) #2
jamesr
https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incoming_task_queue.cc File base/message_loop/incoming_task_queue.cc (right): https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incoming_task_queue.cc#newcode32 base/message_loop/incoming_task_queue.cc:32: // more than 0 and less than 32ms. On ...
6 years, 5 months ago (2014-07-18 04:45:37 UTC) #3
darin (slow to review)
https://codereview.chromium.org/395913006/diff/60001/base/message_loop/message_pump.h File base/message_loop/message_pump.h (right): https://codereview.chromium.org/395913006/diff/60001/base/message_loop/message_pump.h#newcode47 base/message_loop/message_pump.h:47: virtual void UpdateTimerGranularity() {} On 2014/07/18 04:45:37, jamesr wrote: ...
6 years, 5 months ago (2014-07-18 04:55:54 UTC) #4
jamesr
https://codereview.chromium.org/395913006/diff/60001/base/message_loop/message_pump.h File base/message_loop/message_pump.h (right): https://codereview.chromium.org/395913006/diff/60001/base/message_loop/message_pump.h#newcode47 base/message_loop/message_pump.h:47: virtual void UpdateTimerGranularity() {} On 2014/07/18 04:55:54, darin wrote: ...
6 years, 5 months ago (2014-07-18 06:18:33 UTC) #5
cpu_(ooo_6.6-7.5)
code update PTAL. https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incoming_task_queue.cc File base/message_loop/incoming_task_queue.cc (right): https://codereview.chromium.org/395913006/diff/60001/base/message_loop/incoming_task_queue.cc#newcode32 base/message_loop/incoming_task_queue.cc:32: // more than 0 and less ...
6 years, 5 months ago (2014-07-18 22:53:10 UTC) #6
cpu_(ooo_6.6-7.5)
actually hold on the review of a couple of minutes.
6 years, 5 months ago (2014-07-18 23:00:06 UTC) #7
cpu_(ooo_6.6-7.5)
Ok, ready for review, delta from patch 3 is a valid review continuation point.
6 years, 5 months ago (2014-07-18 23:12:03 UTC) #8
darin (slow to review)
LGTM https://codereview.chromium.org/395913006/diff/100001/base/message_loop/incoming_task_queue.h File base/message_loop/incoming_task_queue.h (right): https://codereview.chromium.org/395913006/diff/100001/base/message_loop/incoming_task_queue.h#newcode73 base/message_loop/incoming_task_queue.h:73: // The lock that protects access to the ...
6 years, 5 months ago (2014-07-19 04:06:57 UTC) #9
cpu_(ooo_6.6-7.5)
I am going to have to remove the call to Time::IsHighResolutionTimerInUse from the test. It ...
6 years, 5 months ago (2014-07-20 01:25:55 UTC) #10
darin (slow to review)
On 2014/07/20 01:25:55, cpu wrote: > I am going to have to remove the call ...
6 years, 5 months ago (2014-07-20 18:48:12 UTC) #11
cpu_(ooo_6.6-7.5)
The CQ bit was checked by cpu@chromium.org
6 years, 5 months ago (2014-07-21 22:13:12 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cpu@chromium.org/395913006/100001
6 years, 5 months ago (2014-07-21 22:15:04 UTC) #13
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium ...
6 years, 5 months ago (2014-07-21 23:31:03 UTC) #14
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-22 00:53:02 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/builds/41335)
6 years, 5 months ago (2014-07-22 00:53:03 UTC) #16
cpu_(ooo_6.6-7.5)
The CQ bit was checked by cpu@chromium.org
6 years, 5 months ago (2014-07-22 01:51:47 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cpu@chromium.org/395913006/100001
6 years, 5 months ago (2014-07-22 01:53:21 UTC) #18
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium ...
6 years, 5 months ago (2014-07-22 02:49:25 UTC) #19
commit-bot: I haz the power
Change committed as 284625
6 years, 5 months ago (2014-07-22 04:30:01 UTC) #20
Kunihiko Sakamoto
A revert of this CL has been created in https://codereview.chromium.org/407073004/ by ksakamoto@chromium.org. The reason for ...
6 years, 5 months ago (2014-07-22 11:30:22 UTC) #21
jar (doing other things)
6 years, 4 months ago (2014-08-15 19:09:20 UTC) #22
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698