Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2023033003: scheduler: Throttle timers in out-of-view frames (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 3 months ago by Sami
Modified:
1 year ago
CC:
blink-reviews, chromium-reviews, scheduler-bugs_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

scheduler: Throttle timers in out-of-view frames Throttle timer tasks to 1 Hz in out-of-view or invisible frames. This reduces the power consumption and performance impact of these tasks, which are not likely having any visible effect on the page anyway. BUG=616519 Committed: https://crrev.com/7fec349e59e40dd9beee5f7eafa9e0db5c4ac1bd Cr-Commit-Position: refs/heads/master@{#415278}

Patch Set 1 #

Patch Set 2 : Rebased #

Patch Set 3 : Rebased #

Patch Set 4 : Add cross origin check, tests #

Total comments: 12

Patch Set 5 : Review comments, still looking at test failures #

Patch Set 6 : Fix tests #

Total comments: 1

Patch Set 7 : Resurrected #

Patch Set 8 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+241 lines, -9 lines) Patch
M content/child/runtime_features.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/common/content_features.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_features.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/frame/FrameView.cpp View 1 2 3 4 5 6 7 1 chunk +2 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/blink_platform.gypi View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h View 1 2 3 4 5 6 3 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.cc View 1 2 3 4 5 6 5 chunks +34 lines, -9 lines 0 comments Download
A third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl_unittest.cc View 1 2 3 4 5 6 1 chunk +180 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/WebRuntimeFeatures.cpp View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebFrameScheduler.h View 1 2 3 4 5 6 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebRuntimeFeatures.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 45 (19 generated)
Sami
PTAL.
1 year, 2 months ago (2016-06-28 16:51:41 UTC) #3
alex clarke (OOO till 29th)
Plumbing looks fairly sensible. Would be good to get those test failures fixed before I ...
1 year, 2 months ago (2016-06-28 17:11:28 UTC) #4
Sami
https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/renderer/renderer_web_scheduler_impl.cc File components/scheduler/renderer/renderer_web_scheduler_impl.cc (right): https://codereview.chromium.org/2023033003/diff/60001/components/scheduler/renderer/renderer_web_scheduler_impl.cc#newcode19 components/scheduler/renderer/renderer_web_scheduler_impl.cc:19: const base::Feature kHiddenTimerThrottlingFeature{ On 2016/06/28 17:11:28, alex clarke wrote: ...
1 year, 2 months ago (2016-06-28 17:22:46 UTC) #5
Sami
Okay, I think I've found the problem with the tests: frame visibility updates were getting ...
1 year, 2 months ago (2016-06-28 17:44:46 UTC) #7
haraken
On 2016/06/28 17:44:46, Sami wrote: > Okay, I think I've found the problem with the ...
1 year, 2 months ago (2016-06-29 01:00:23 UTC) #8
ojan
I'm a little worried about the naming here. If it's the "default" then people will ...
1 year, 2 months ago (2016-06-29 04:53:00 UTC) #9
esprehn
https://codereview.chromium.org/2023033003/diff/100001/components/scheduler/renderer/web_frame_scheduler_impl.h File components/scheduler/renderer/web_frame_scheduler_impl.h (right): https://codereview.chromium.org/2023033003/diff/100001/components/scheduler/renderer/web_frame_scheduler_impl.h#newcode71 components/scheduler/renderer/web_frame_scheduler_impl.h:71: const bool allow_hidden_timer_throttling_; I'm sad we keep plumbing more ...
1 year, 2 months ago (2016-06-29 05:06:49 UTC) #11
esprehn
Hmm can you explain this patch? This is moving us away from being able to ...
1 year, 2 months ago (2016-06-29 05:09:32 UTC) #12
Sami
> I'm a little worried about the naming here. If it's the "default" then people ...
1 year, 2 months ago (2016-06-29 10:20:12 UTC) #13
esprehn
On 2016/06/29 at 10:20:12, skyostil wrote: > > I'm a little worried about the naming ...
1 year, 2 months ago (2016-06-30 03:02:39 UTC) #14
haraken
FWIW, the per-iframe scheduler work (https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yjgQ/edit#) needs the unthrottled task runner on WebFrameScheduler. I'm a ...
1 year, 2 months ago (2016-07-01 05:31:27 UTC) #16
haraken
On 2016/07/01 05:31:27, haraken wrote: > FWIW, the per-iframe scheduler work > (https://docs.google.com/document/d/10It1DFRP7H3gev9hQA-7dtTcvcMFhrxuCt3-k21yjgQ/edit) > needs ...
1 year, 2 months ago (2016-07-01 05:32:06 UTC) #17
Sami
On 2016/07/01 05:32:06, haraken wrote: > On 2016/07/01 05:31:27, haraken wrote: > > FWIW, the ...
1 year, 2 months ago (2016-07-01 11:02:38 UTC) #18
Sami
Okay, I've picked this up again now that the scheduler is in Blink. PTAL. FYI, ...
1 year ago (2016-08-25 16:29:06 UTC) #23
alex clarke (OOO till 29th)
lgtm
1 year ago (2016-08-25 16:39:24 UTC) #24
alex clarke (OOO till 29th)
lgtm
1 year ago (2016-08-25 16:39:26 UTC) #25
haraken
LGTM I'm just curious but what metrics are you going to keep track of to ...
1 year ago (2016-08-26 02:10:52 UTC) #26
Sami
On 2016/08/26 02:10:52, haraken wrote: > LGTM Thanks! +jochen@, could you check content/ please? > ...
1 year ago (2016-08-26 10:04:36 UTC) #28
jochen (gone - plz use gerrit)
lgtm
1 year ago (2016-08-26 15:01:58 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2023033003/120001
1 year ago (2016-08-26 15:54:35 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/269574)
1 year ago (2016-08-26 18:33:52 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2023033003/140001
1 year ago (2016-08-30 09:46:46 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/288875)
1 year ago (2016-08-30 11:00:11 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2023033003/140001
1 year ago (2016-08-30 11:09:25 UTC) #41
commit-bot: I haz the power
Committed patchset #8 (id:140001)
1 year ago (2016-08-30 12:44:59 UTC) #43
commit-bot: I haz the power
1 year ago (2016-08-30 12:47:40 UTC) #45
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/7fec349e59e40dd9beee5f7eafa9e0db5c4ac1bd
Cr-Commit-Position: refs/heads/master@{#415278}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b