New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 854237 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 18
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-07-16
OS: Windows
Pri: 3
Type: Bug

Blocked on:
issue 863938



Sign in to add a comment

MessageLoop::DoIdleWork() keeps high-resolutions timers enabled in more situations than necessary

Project Member Reported by gab@chromium.org, Jun 19 2018

Issue description

trunk @ r566015

I noticed today that MessageLoop::DoIdleWork() will enable hi-res timers on Windows unconditionally if incoming_task_queue_->HasPendingHighResolutionTasks().

Nested loops however end up in a |!task_execution_allowed_| state by default (i.e. if not explicitly of RunLoop::Type::kNestableTasksAllowed).

In such a situation, DoIdleWork() incorrectly leaves hi-res timers enabled while DoDelayedWork() refuses to process any task.

This is also currently the case for deferred non-nestable tasks (but https://chromium-review.googlesource.com/c/chromium/src/+/1103120 is about to address this by no longer considering hi-res delays which aren't in the DelayedQueue).

Cleaning these two things will be generally cleaner and may slightly reduce time spent in hi-res mode (although I don't expect nested loops to be common enough to make much of a dent in the "Windows.HighResolutionTimerUsage" metric). =D
 

Comment 1 by gab@chromium.org, Jun 19 2018

Summary: MessageLoop::DoIdleWork() keeps high-resolutions timers enabled in more situations than necessary (was: MessageLoop::DoIdleWork() will keep high-resolutions timers incorrectly enabled in default nested loops)
I also notice that we keep hi-res timers enabled while a thread is active and since this is a per-process setting (any thread being on means it's enabled for the process) it means other threads are potentially using hi-res timers unnecessarily. I'll try to disable hi-res timers while a thread is active to see.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 19 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6a1da98dc56f11d5abb93a9762b705b76c879e05

commit 6a1da98dc56f11d5abb93a9762b705b76c879e05
Author: Gabriel Charette <gab@chromium.org>
Date: Tue Jun 19 20:14:54 2018

[MessageLoop] Only count tasks actually in the DelayedQueue as pending hi-res tasks.

This count only matters when reaching MessageLoop::DoIdleWork() at
which point the TriageQueue is empty by definition (and tasks in the
DeferredQueue do not matter). This CL is therefore a no-op behavior
wise.

In an upcoming change, the thread-safe |incoming_queue_| will move to
MessageLoopTaskRunner and dropping unecessary hi-res counts now will
make this transition simpler.

This also allows disconnecting DelayedQueue and DeferredQueue from their
outer class.

R=danakj@chromium.org, kylechar@chromium.org

Bug: 708584,  854237 
Change-Id: Icf36f1e425a8320fb14a4c561c529a9ea2389e0f
Reviewed-on: https://chromium-review.googlesource.com/1103120
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568586}
[modify] https://crrev.com/6a1da98dc56f11d5abb93a9762b705b76c879e05/base/message_loop/incoming_task_queue.cc
[modify] https://crrev.com/6a1da98dc56f11d5abb93a9762b705b76c879e05/base/message_loop/incoming_task_queue.h

> since this is a per-process setting

Oh it's way worse than that. Hi-res timers are a system-wide setting. When we raise the timer frequency we may cause every program on the system to wake up more frequently. What is actually being changed is how frequently the system timer interrupt is fired, which then affects when all threads on the system can possibly be woken.

The actual power impact is hard to estimate. Microsoft has been making OS changes that reduce the impact of having the timer frequency raised (only firing the interrupt when it is actually needed, and only on some cores). And, perversely enough, one thing that reduces the power cost of us raising the timer frequency is that often some other program has already raised it, so us raising it makes no difference. For instance, last I checked all Go programs leave the timer frequency permanently raised - see https://github.com/golang/go/issues/8687.

That said, thank you for making this change. It will help.

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f93ca115caf2976f5a799b1782aaddfd6544102e

commit f93ca115caf2976f5a799b1782aaddfd6544102e
Author: Gabriel Charette <gab@chromium.org>
Date: Sat Jun 30 05:35:30 2018

[MessageLoop] Add MessageLoop.ScheduledSleep.* metrics.

These metric help evaluate whether scheduled sleeps (especially long
ones) tend to be the cause of the next wakeup or not. This will
influence the upcoming redesign of delayed tasks management.

The time histogram was extended to 14 days since that's the maximum
possible cap we will enforce @  crbug.com/850450#c3 . With 50 buckets
that still yields 7 buckets under 16ms so we get proper resolution
in the fast range too.

This will also, as a side-effect of not doing delayed tasks logic
when ShouldQuitWhenIdle(), result in disabling hi-resolution timers
when the loop is QuitWhenIdle(). Not that this will matter much for power
per this mostly being a test-only operation but always nice to reduce
time spent under hi-res timers.

Bug:  850450 , 786597,  854237 
Change-Id: Ic552a512f534fc04162a3d6b53c6043621bb3fcf
Reviewed-on: https://chromium-review.googlesource.com/1094301
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571763}
[modify] https://crrev.com/f93ca115caf2976f5a799b1782aaddfd6544102e/base/message_loop/message_loop.cc
[modify] https://crrev.com/f93ca115caf2976f5a799b1782aaddfd6544102e/base/message_loop/message_loop.h
[modify] https://crrev.com/f93ca115caf2976f5a799b1782aaddfd6544102e/tools/metrics/histograms/histograms.xml

Project Member

Comment 5 by bugdroid1@chromium.org, Jul 4

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/623e3241cee548d0200492a43451fa5cfa9ccd62

commit 623e3241cee548d0200492a43451fa5cfa9ccd62
Author: Gabriel Charette <gab@chromium.org>
Date: Wed Jul 04 18:34:19 2018

[MessageLoop] Disable hi-res timers while in nested loops that don't process application tasks

(also disable "on idle" metrics in that state as they pertain to pending
delayed tasks and don't make sense when those are intentionally being
ignored)

R=danakj@chromium.org, kylechar@chromium.org

Bug:  854237 
Change-Id: I8b2201c8d8c3508878fcf7b5fef6b0ea7e857ba3
Reviewed-on: https://chromium-review.googlesource.com/1106434
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572626}
[modify] https://crrev.com/623e3241cee548d0200492a43451fa5cfa9ccd62/base/message_loop/message_loop.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jul 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/62973d1b3f85cbc12f3f6e255e8d5dd1f91225b4

commit 62973d1b3f85cbc12f3f6e255e8d5dd1f91225b4
Author: Gabriel Charette <gab@chromium.org>
Date: Wed Jul 11 18:40:38 2018

[MessageLoop] Disable hi-res timers when not sleeping

Time::ActivateHighResolutionTimer(bool activating) is a per-thread vote
for a system-wide side-effect. For a given thread, hi-res timers are
only useful when going to sleep (if it has pending hi-res tasks).

Deactivating a thread's vote while it's active will prevent other
threads on the system which do not have hi-res requirements from
being forced to use hi-res timers in that period.

Bug:  854237 
Change-Id: I1393e184cac6c9321d13b92b6077a38c62b1f590
Reviewed-on: https://chromium-review.googlesource.com/1107110
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574259}
[modify] https://crrev.com/62973d1b3f85cbc12f3f6e255e8d5dd1f91225b4/base/message_loop/message_loop.cc
[modify] https://crrev.com/62973d1b3f85cbc12f3f6e255e8d5dd1f91225b4/base/message_loop/message_loop.h
[modify] https://crrev.com/62973d1b3f85cbc12f3f6e255e8d5dd1f91225b4/base/message_loop/message_loop_unittest.cc

NextAction: 2018-07-16
All intended changes landed, I'll loop back next week to see if we were lucky enough for this to have an impact on Windows.HighResolutionTimerUsage
Note that crbug.com/862641 shows that on some users' machines we are repeatedly toggling the timer interrupt frequency which leads to non-trivial CPU time spent in NtSetTimerResolution.
Blockedon: 863938
 Issue 863938  highlights that r574259 was more detrimental than anything.

It did result in the 97th and 98th percentiles of Windows.HighResolutionTimerUsage going down from 100% active to 97% and 99% active [1]. But that came at the cost of contention in base::Time::ActivateHighResolutionTimer() (specifically the timeEndPeriod() kernel call) [2].

[1] https://uma.googleplex.com/p/chrome/timeline_v2/?sid=6e586de84b5e84362bdfc4ff24968c75

[2] https://uma.googleplex.com/p/chrome/callstacks?sid=3aba0fb9c8f865a26a958c7ea308c043

I'll revert the last change and leave it at that (there's probably still room for improvement but I was just making drive-by tweaks and won't be able to own this further).
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 18

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/050feaa75927b7216306c6a12084cb64920f9e78

commit 050feaa75927b7216306c6a12084cb64920f9e78
Author: Gabriel Charette <gab@chromium.org>
Date: Wed Jul 18 19:16:12 2018

Revert "[MessageLoop] Disable hi-res timers when not sleeping"

This reverts commit 62973d1b3f85cbc12f3f6e255e8d5dd1f91225b4.

Reason for revert: results in more churn in
base::Time::ActivateHighResolutionTimer()
without much benefits (decreased 97th and 98th percentile of
Windows.HighResolutionTimerUsage from 100% to 97% and 99%
respectively). See  crbug.com/863938 

Original change's description:
> [MessageLoop] Disable hi-res timers when not sleeping
> 
> Time::ActivateHighResolutionTimer(bool activating) is a per-thread vote
> for a system-wide side-effect. For a given thread, hi-res timers are
> only useful when going to sleep (if it has pending hi-res tasks).
> 
> Deactivating a thread's vote while it's active will prevent other
> threads on the system which do not have hi-res requirements from
> being forced to use hi-res timers in that period.
> 
> Bug:  854237 
> Change-Id: I1393e184cac6c9321d13b92b6077a38c62b1f590
> Reviewed-on: https://chromium-review.googlesource.com/1107110
> Reviewed-by: danakj <danakj@chromium.org>
> Reviewed-by: kylechar <kylechar@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#574259}

TBR=danakj@chromium.org,gab@chromium.org,kylechar@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  854237 ,  863938 
Change-Id: I7c26646cac8548ae7b02c90e045bc857ae890ce7
Reviewed-on: https://chromium-review.googlesource.com/1140753
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576157}
[modify] https://crrev.com/050feaa75927b7216306c6a12084cb64920f9e78/base/message_loop/message_loop.cc
[modify] https://crrev.com/050feaa75927b7216306c6a12084cb64920f9e78/base/message_loop/message_loop.h
[modify] https://crrev.com/050feaa75927b7216306c6a12084cb64920f9e78/base/message_loop/message_loop_unittest.cc

Status: Fixed (was: Started)
Drive-by-cleaned-up as much as I could.

Sign in to add a comment