Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2555623004:
[scheduler] Throttle default timer queue when renderer is backgrounded. (Closed)
Description[scheduler] Throttle default timer queue when renderer is backgrounded.
BUG=671639
Committed: https://crrev.com/2995e21f5f0d5b0dd6bbacdc17a39cf0c0b0ae4b
Cr-Commit-Position: refs/heads/master@{#437036}
Patch Set 1 #Patch Set 2 : Ooops #
Total comments: 1
Patch Set 3 : Integrate throttling into policy #
Total comments: 5
Patch Set 4 : Address Sami's comments #Patch Set 5 : Rebased #
Messages
Total messages: 22 (9 generated)
altimin@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
PTAL
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2555623004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2555623004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:494: task_queue_throttler()->IncreaseThrottleRefCount( This is going to fight with UpdatePolicyLocked. What you need to do (assuming Sami agrees this is a good idea - IIRC we discussed this in the past and didn't do it for some reason) is: 1. Add this clause near the bottom of UpdatePolicyLocked: if (MainThreadOnly().renderer_backgrounded) { new_policy.timer_queue_policy.time_domain_type = TimeDomainType::THROTTLED; } 2. Call ForceUpdatePolicy() in here and in OnRendererForegrounded Note this needs to place nicely (i.e. not call ForceUpdatePolicy too often) with SuspendTimerQueueWhenBackgrounded / ResumeTimerQueueWhenForegroundedOrResumed which Jared added to suspend timers completely after 5 mins on android.
PTAL
Good catch! https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1081: new_policy.timer_queue_policy.time_domain_type = TimeDomainType::THROTTLED; Should we have a TODO around making this also use the background time budget pool? Ideally we'd get rid of this queue completely, but that might take awhile. (Aside: we might also make it so that there's only one background budget pool per renderer instead of one per WebView.) https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:181: class AutoAdvanceNowEnabler { bikeshed: ScopedAutoAdvanceNow is what I'd expect this to be called :)
PTAL https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1081: new_policy.timer_queue_policy.time_domain_type = TimeDomainType::THROTTLED; On 2016/12/06 19:09:10, Sami wrote: > Should we have a TODO around making this also use the background time budget > pool? Ideally we'd get rid of this queue completely, but that might take awhile. > > (Aside: we might also make it so that there's only one background budget pool > per renderer instead of one per WebView.) Done. (And I want to state that I don't really like the idea of having one time budget pool per renderer - it will prevent throttling when one of the pages is foregrounded and it's against our frame performance isolation policy). https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc:181: class AutoAdvanceNowEnabler { On 2016/12/06 19:09:10, Sami wrote: > bikeshed: ScopedAutoAdvanceNow is what I'd expect this to be called :) Done.
lgtm
The CQ bit was checked by altimin@chromium.org
On 2016/12/07 16:53:21, alex clarke wrote: > lgtm Do we want to merge it back into M56?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2555623004/#ps80001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm! https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2555623004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1081: new_policy.timer_queue_policy.time_domain_type = TimeDomainType::THROTTLED; On 2016/12/07 11:20:09, altimin wrote: > On 2016/12/06 19:09:10, Sami wrote: > > Should we have a TODO around making this also use the background time budget > > pool? Ideally we'd get rid of this queue completely, but that might take > awhile. > > > > (Aside: we might also make it so that there's only one background budget pool > > per renderer instead of one per WebView.) > > Done. (And I want to state that I don't really like the idea of having one time > budget pool per renderer - it will prevent throttling when one of the pages is > foregrounded and it's against our frame performance isolation policy). (Why would it prevent throttling? The foreground pages just wouldn't participate in the budget. What I was mainly going for is that a renderer with 100 tabs shouldn't necessarily be allowed to use more CPU than a renderer with 1 tab. Later we can coalesce this limit across renderers too.)
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481134440902980,
"parent_rev": "c879dd4e606da8e4d8dbf59c8a9822a40d379414", "commit_rev":
"34603be8c820eca84eb7d77bd523a3579c9ff89b"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Throttle default timer queue when renderer is backgrounded. BUG=671639 ========== to ========== [scheduler] Throttle default timer queue when renderer is backgrounded. BUG=671639 Committed: https://crrev.com/2995e21f5f0d5b0dd6bbacdc17a39cf0c0b0ae4b Cr-Commit-Position: refs/heads/master@{#437036} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/2995e21f5f0d5b0dd6bbacdc17a39cf0c0b0ae4b Cr-Commit-Position: refs/heads/master@{#437036} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
