Issue metadata
Sign in to add a comment
|
MessageLoop::DoIdleWork() keeps high-resolutions timers enabled in more situations than necessary |
||||||||||||||||||||||
Issue descriptiontrunk @ 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
,
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
,
Jun 19 2018
> 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.
,
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
,
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
,
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
,
Jul 11
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
,
Jul 12
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.
,
Jul 17
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).
,
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
,
Jul 18
Drive-by-cleaned-up as much as I could. |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by gab@chromium.org
, Jun 19 2018