Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(135)

Issue 2276353002: Remove after wakeup logic and replace PumpTask with Fences (Closed)

Created:
4 years, 3 months ago by alex clarke (OOO till 29th)
Modified:
4 years, 3 months ago
CC:
altimin, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, dglazkov+blink, 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

Remove after wakeup logic and replace PumpTask with Fences Task pumping is a neat concept for throttling but it adds a lot of complexity to the task queues. For example it prevents the WorkQueues from being read only (which they could otherwise be). Fences provide similar functionality but don't preclude various queue optimizations. They are also a more familiar concept which should make the code easier to comprehend. The after wake up logic isn't used and it also adds a lot of complexity. Lets get rid of it :) BUG=638542 Committed: https://crrev.com/c2db8aa78e1130a08bd47f2aa611f322afaf8dc5 Cr-Commit-Position: refs/heads/master@{#416558}

Patch Set 1 #

Patch Set 2 : Fix compile #

Patch Set 3 : Make TestLongIdlePeriodTimeline test quiescence again! #

Total comments: 31

Patch Set 4 : Lets introduce the concept of a fence (replaces pumping) #

Patch Set 5 : Rebased #

Total comments: 11

Patch Set 6 : Changes for Sami #

Patch Set 7 : Fixed nit and made calls to MaybeScheduleImmediateWork more precice for InsertFence & RemoveFence #

Patch Set 8 : Rebased #

Patch Set 9 : Rebased #

Patch Set 10 : Slight simplification #

Unified diffs Side-by-side diffs Delta from patch set Stats (+961 lines, -1073 lines) Patch
M third_party/WebKit/Source/platform/BUILD.gn View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/WebScheduler.cpp View 1 chunk +0 lines, -5 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h View 1 2 3 4 5 6 7 8 9 8 chunks +13 lines, -49 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc View 1 2 3 4 5 6 7 8 9 12 chunks +118 lines, -177 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -11 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc View 1 2 3 4 5 6 7 8 10 chunks +22 lines, -40 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc View 1 2 3 4 5 6 7 8 16 chunks +125 lines, -304 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/time_domain.h View 2 chunks +4 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/time_domain.cc View 4 chunks +5 lines, -12 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/time_domain_unittest.cc View 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/work_queue.h View 1 2 3 4 5 6 4 chunks +43 lines, -15 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/work_queue.cc View 1 2 3 4 5 6 chunks +54 lines, -7 lines 0 comments Download
A third_party/WebKit/Source/platform/scheduler/base/work_queue_unittest.cc View 1 2 3 4 5 1 chunk +362 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc View 1 2 3 4 chunks +10 lines, -16 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/idle_helper.h View 1 2 3 1 chunk +19 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/idle_helper.cc View 1 2 3 5 chunks +9 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/idle_helper_unittest.cc View 2 chunks +0 lines, -112 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h View 1 2 3 4 5 6 7 8 2 chunks +0 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.cc View 1 2 3 4 5 6 7 8 3 chunks +1 line, -13 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/scheduler_helper_unittest.cc View 1 2 3 2 chunks +5 lines, -15 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/single_thread_idle_task_runner.cc View 1 2 3 2 chunks +0 lines, -14 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/web_scheduler_impl.h View 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/web_scheduler_impl.cc View 1 chunk +0 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/webthread_base.cc View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/worker_scheduler_impl_unittest.cc View 1 2 2 chunks +38 lines, -16 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl_unittest.cc View 1 2 3 4 3 chunks +19 lines, -105 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/throttling_helper.h View 1 2 3 4 5 2 chunks +19 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/throttling_helper.cc View 1 2 3 5 chunks +11 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/throttling_helper_unittest.cc View 1 2 3 9 chunks +30 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/web_frame_scheduler_impl.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/testing/TestingPlatformSupport.h View 1 2 3 4 5 6 7 1 chunk +29 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebScheduler.h View 2 chunks +0 lines, -7 lines 0 comments Download
M third_party/WebKit/public/platform/scheduler/base/task_queue.h View 1 2 3 6 chunks +17 lines, -78 lines 0 comments Download
M third_party/WebKit/public/platform/scheduler/child/single_thread_idle_task_runner.h View 1 2 3 3 chunks +0 lines, -6 lines 0 comments Download
M third_party/WebKit/public/platform/scheduler/child/webthread_base.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 57 (41 generated)
alex clarke (OOO till 29th)
PTAL
4 years, 3 months ago (2016-08-25 13:24:32 UTC) #4
Sami
This is an awesome cleanup, assuming the remaining code still works :) https://codereview.chromium.org/2276353002/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc ...
4 years, 3 months ago (2016-08-25 15:54:13 UTC) #11
Sami
+FYI altimin@ in case this has some implications for cpu time throttling.
4 years, 3 months ago (2016-08-25 16:35:19 UTC) #13
alex clarke (OOO till 29th)
PTAL :) https://codereview.chromium.org/2276353002/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2276353002/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc#newcode444 third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:444: if (any_thread().immediate_incoming_queue.empty()) { On 2016/08/25 15:54:12, Sami ...
4 years, 3 months ago (2016-08-26 13:29:09 UTC) #20
Sami
The fence abstraction seems to fit nicely with what the throttling and idle helpers want ...
4 years, 3 months ago (2016-08-26 14:37:03 UTC) #23
alex clarke (OOO till 29th)
PTAL https://codereview.chromium.org/2276353002/diff/80001/third_party/WebKit/Source/platform/scheduler/base/work_queue.cc File third_party/WebKit/Source/platform/scheduler/base/work_queue.cc (left): https://codereview.chromium.org/2276353002/diff/80001/third_party/WebKit/Source/platform/scheduler/base/work_queue.cc#oldcode106 third_party/WebKit/Source/platform/scheduler/base/work_queue.cc:106: task_queue_->TraceQueueSize(true); On 2016/08/26 14:37:02, Sami wrote: > Did ...
4 years, 3 months ago (2016-08-26 16:33:14 UTC) #27
Sami
lgtm! https://codereview.chromium.org/2276353002/diff/80001/third_party/WebKit/Source/platform/scheduler/base/work_queue.h File third_party/WebKit/Source/platform/scheduler/base/work_queue.h (right): https://codereview.chromium.org/2276353002/diff/80001/third_party/WebKit/Source/platform/scheduler/base/work_queue.h#newcode85 third_party/WebKit/Source/platform/scheduler/base/work_queue.h:85: // task removed had an enqueue order >= ...
4 years, 3 months ago (2016-08-26 16:44:01 UTC) #29
alex clarke (OOO till 29th)
https://codereview.chromium.org/2276353002/diff/80001/third_party/WebKit/Source/platform/scheduler/base/work_queue.h File third_party/WebKit/Source/platform/scheduler/base/work_queue.h (right): https://codereview.chromium.org/2276353002/diff/80001/third_party/WebKit/Source/platform/scheduler/base/work_queue.h#newcode85 third_party/WebKit/Source/platform/scheduler/base/work_queue.h:85: // task removed had an enqueue order >= the ...
4 years, 3 months ago (2016-08-26 16:47:38 UTC) #30
alex clarke (OOO till 29th)
Jochen can you please do an owners review?
4 years, 3 months ago (2016-08-26 17:07:32 UTC) #34
jochen (gone - plz use gerrit)
stampety-stamp lgtm
4 years, 3 months ago (2016-08-26 17:08:59 UTC) #35
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/2276353002/140001
4 years, 3 months ago (2016-08-26 23:27:35 UTC) #40
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/287670)
4 years, 3 months ago (2016-08-27 00:41:33 UTC) #42
Sami
Judging from the layout tests there might be some service worker related breakage here.
4 years, 3 months ago (2016-08-30 14:11:03 UTC) #43
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/2276353002/180001
4 years, 3 months ago (2016-09-05 18:00:13 UTC) #53
commit-bot: I haz the power
Committed patchset #10 (id:180001)
4 years, 3 months ago (2016-09-05 18:04:37 UTC) #55
commit-bot: I haz the power
4 years, 3 months ago (2016-09-05 18:06:21 UTC) #57
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/c2db8aa78e1130a08bd47f2aa611f322afaf8dc5
Cr-Commit-Position: refs/heads/master@{#416558}

Powered by Google App Engine
This is Rietveld 408576698