Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2557713007:
[scheduler] Delay budget-based throttling for 10s after backgrounded. (Closed)
Description[scheduler] Delay budget-based throttling for 10s after backgrounded.
Delay the start of background budget-based timer throttling for 10s after
page is backgrounded to deal with long navigation-related blocking IPCs.
R=alexclarke@chromium.org,skyostil@chromium.org
BUG=671814
Committed: https://crrev.com/c1fe2cb61120dcfc2eead782a96649f3e22b28c3
Cr-Commit-Position: refs/heads/master@{#437262}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed comments from skyostil@ and alexclarke@ #
Total comments: 6
Patch Set 3 : Address nits #
Total comments: 6
Patch Set 4 : Address comments #
Messages
Total messages: 34 (20 generated)
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Looks good, but how about a test? :) https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:33: constexpr base::TimeDelta kAggresiveThrottlingDelayAfterBackgrounded = typo: aggressive https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:145: if (page_visible_) { I'd suggest unifying this and the other call site to a helper so this logic doesn't get duplicated. https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:321: renderer_scheduler_->ControlTaskRunner()->PostDelayedTask( DCHECK that the callback isn't already pending?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
+1 A test. https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:33: constexpr base::TimeDelta kAggresiveThrottlingDelayAfterBackgrounded = I wonder if kBackgroundBudgetThrottlingGracePeriod is a better name? https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:148: renderer_scheduler_->ControlTaskRunner()->PostDelayedTask( I think this patch is fine for a first pass, but I'm wondering if it's possible to piggyback the logic to enable the budget into the throttling pump function? My motivation would be one less task posted when backgrounded - it's quite possible that optimization isn't worth the added complexity but it would be good to check after this patch has landed.
https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:82: void EnableAggressiveBackgroundThrottling(); I don't think the word aggressive sets the right tone here. Instead I think EnableBudgetThrottling or EnableBackgroundBudgetThrottling is better.
PTAL https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:33: constexpr base::TimeDelta kAggresiveThrottlingDelayAfterBackgrounded = On 2016/12/07 19:02:40, Sami wrote: > typo: aggressive Acknowledged. https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:33: constexpr base::TimeDelta kAggresiveThrottlingDelayAfterBackgrounded = On 2016/12/08 11:18:50, alex clarke wrote: > I wonder if kBackgroundBudgetThrottlingGracePeriod is a better name? Done. https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:145: if (page_visible_) { On 2016/12/07 19:02:40, Sami wrote: > I'd suggest unifying this and the other call site to a helper so this logic > doesn't get duplicated. Done. https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:148: renderer_scheduler_->ControlTaskRunner()->PostDelayedTask( On 2016/12/08 11:18:50, alex clarke wrote: > I think this patch is fine for a first pass, but I'm wondering if it's possible > to piggyback the logic to enable the budget into the throttling pump function? > My motivation would be one less task posted when backgrounded - it's quite > possible that optimization isn't worth the added complexity but it would be good > to check after this patch has landed. Personally I don't like the added complexity, but let's add a todo and see if this optimisation is needed. https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:321: renderer_scheduler_->ControlTaskRunner()->PostDelayedTask( On 2016/12/07 19:02:40, Sami wrote: > DCHECK that the callback isn't already pending? Is there a simple way to do it without maintaining a flag inside WebViewSchedulerImpl? https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2557713007/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:82: void EnableAggressiveBackgroundThrottling(); On 2016/12/08 11:19:45, alex clarke wrote: > I don't think the word aggressive sets the right tone here. Instead I think > EnableBudgetThrottling or EnableBackgroundBudgetThrottling is better. Done.
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...
Description was changed from ========== [scheduler] Delay aggressive throttling for 10s after backgrounded. Delay the start of aggresive background timer throttling for 10s after page is backgrounded to deal with long navigation-related blocking IPCs. R=alexclarke@chromium.org,skyostil@chromium.org BUG=671814 ========== to ========== [scheduler] Delay budget-based throttling for 10s after backgrounded. Delay the start of background budget-based timer throttling for 10s after page is backgrounded to deal with long navigation-related blocking IPCs. R=alexclarke@chromium.org,skyostil@chromium.org BUG=671814 ==========
https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:82: void SetBackgroundBudgetThrottlingState(); Sounds awfully similar to EnableBackgroundBudgetThrottling although it does something quite different. Can you add a comment explaining what it does? https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc:618: TEST_F(WebViewSchedulerImplTest, AggressiveBackgroundThrottling) { Lets change this to BackgroundThrottlingGracePeriod :) https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc:623: I'n not sure all these blank lines help readability. I'd suggest removing lines 631, 628, 625, 623
PTAL https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:82: void SetBackgroundBudgetThrottlingState(); On 2016/12/08 14:14:19, alex clarke wrote: > Sounds awfully similar to EnableBackgroundBudgetThrottling although it does > something quite different. Can you add a comment explaining what it does? Done. https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc:618: TEST_F(WebViewSchedulerImplTest, AggressiveBackgroundThrottling) { On 2016/12/08 14:14:19, alex clarke wrote: > Lets change this to BackgroundThrottlingGracePeriod :) Done. https://codereview.chromium.org/2557713007/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl_unittest.cc:623: On 2016/12/08 14:14:19, alex clarke wrote: > I'n not sure all these blank lines help readability. I'd suggest removing lines > 631, 628, 625, 623 Done.
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/2557713007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:141: delayed_background_budget_throttling_enabler_.Cancel(); Can we move this inside SetBackgroundBudgetThrottlingState? If we use it elsewhere we might forget to cancel any existing timer. https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:82: // Depending on page visibility turns budget throttling off or schedules nit: Depending on page visibility, either turns budget throttling off, or ...
lgtm once Alex is happy. https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:84: void SetBackgroundBudgetThrottlingState(); nit: /Set/Update/
PTAL https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc (right): https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.cc:141: delayed_background_budget_throttling_enabler_.Cancel(); On 2016/12/08 14:23:51, alex clarke wrote: > Can we move this inside SetBackgroundBudgetThrottlingState? If we use it > elsewhere we might forget to cancel any existing timer. Done. https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h (right): https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:82: // Depending on page visibility turns budget throttling off or schedules On 2016/12/08 14:23:51, alex clarke wrote: > nit: Depending on page visibility, either turns budget throttling off, or ... Done. https://codereview.chromium.org/2557713007/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/web_view_scheduler_impl.h:84: void SetBackgroundBudgetThrottlingState(); On 2016/12/08 14:32:10, Sami wrote: > nit: /Set/Update/ Done.
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...
lgtm
The CQ bit was unchecked by altimin@chromium.org
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2557713007/#ps60001 (title: "Address comments")
The CQ bit was checked by altimin@chromium.org
The CQ bit was unchecked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2557713007/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481214381129640,
"parent_rev": "25f5d66d840285733975cf4d250e7f1d8a028754", "commit_rev":
"5e28726bc929b4b28a99f2c2f60d1c283f952697"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Delay budget-based throttling for 10s after backgrounded. Delay the start of background budget-based timer throttling for 10s after page is backgrounded to deal with long navigation-related blocking IPCs. R=alexclarke@chromium.org,skyostil@chromium.org BUG=671814 ========== to ========== [scheduler] Delay budget-based throttling for 10s after backgrounded. Delay the start of background budget-based timer throttling for 10s after page is backgrounded to deal with long navigation-related blocking IPCs. R=alexclarke@chromium.org,skyostil@chromium.org BUG=671814 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Delay budget-based throttling for 10s after backgrounded. Delay the start of background budget-based timer throttling for 10s after page is backgrounded to deal with long navigation-related blocking IPCs. R=alexclarke@chromium.org,skyostil@chromium.org BUG=671814 ========== to ========== [scheduler] Delay budget-based throttling for 10s after backgrounded. Delay the start of background budget-based timer throttling for 10s after page is backgrounded to deal with long navigation-related blocking IPCs. R=alexclarke@chromium.org,skyostil@chromium.org BUG=671814 Committed: https://crrev.com/c1fe2cb61120dcfc2eead782a96649f3e22b28c3 Cr-Commit-Position: refs/heads/master@{#437262} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c1fe2cb61120dcfc2eead782a96649f3e22b28c3 Cr-Commit-Position: refs/heads/master@{#437262} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
