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

Issue 2778123003: [scheduler] Add WakeupBudgetPool. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 months, 3 weeks ago by altimin
Modified:
3 months, 2 weeks ago
CC:
blink-reviews, chromium-reviews, kinuko+watch, scheduler-bugs_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[scheduler] Add WakeupBudgetPool. WakeupBudgetPool allows a short chain of fast continuation tasks to when immediately even when throttled (old throttling mechanism introduced a second delay between each pair). Please note that full task queue throttler integration is still missing (task queue throttler still inserts fences unconditionally) and will be added in a later patch. R=skyostil@chromium.org,alexclarke@chromium.org BUG=699541 Review-Url: https://codereview.chromium.org/2778123003 Cr-Commit-Position: refs/heads/master@{#469736} Committed: https://chromium.googlesource.com/chromium/src/+/db65a6534a5e4ee5bdc0e5d31bdec2d95bc65ca9

Patch Set 1 #

Patch Set 2 : WIP #

Patch Set 3 : First meaningful version #

Total comments: 51

Patch Set 4 : Rebased #

Patch Set 5 : Second iteration. #

Total comments: 38

Patch Set 6 : addressed comments #

Total comments: 10

Patch Set 7 : Addressed comments from alexclarke@ #

Total comments: 12

Patch Set 8 : Rebased & addressed comments from skyostil@ and alexclarke@ #

Total comments: 8

Patch Set 9 : Addressed comments #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+735 lines, -576 lines) Patch
M third_party/WebKit/Source/platform/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 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 1 chunk +1 line, -0 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 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h View 1 2 3 4 5 6 7 5 chunks +25 lines, -102 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc View 1 2 3 4 5 6 4 chunks +15 lines, -168 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/budget_pool_unittest.cc View 1 2 3 4 5 6 5 chunks +105 lines, -43 lines 0 comments Download
A + third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.h View 1 2 3 4 5 6 7 8 3 chunks +16 lines, -79 lines 0 comments Download
A + third_party/WebKit/Source/platform/scheduler/renderer/cpu_time_budget_pool.cc View 1 2 3 4 5 6 7 8 7 chunks +24 lines, -86 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.h View 1 2 3 4 5 6 7 8 3 chunks +4 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc View 1 2 3 4 5 6 7 8 5 chunks +20 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h View 1 2 3 4 5 6 7 6 chunks +28 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc View 1 2 3 4 5 6 7 8 8 chunks +160 lines, -83 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler_unittest.cc View 1 2 3 4 5 6 7 2 chunks +127 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.h View 1 2 3 4 5 6 7 2 chunks +6 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/renderer/throttled_time_domain.cc View 1 2 3 4 5 6 7 1 chunk +7 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.h View 1 2 3 4 5 6 7 8 1 chunk +66 lines, -0 lines 0 comments Download
A third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc View 1 2 3 4 5 6 7 8 1 chunk +120 lines, -0 lines 1 comment Download
Trybot results:  win_clang   win_chromium_x64_rel_ng   win_chromium_compile_dbg_ng   win_chromium_rel_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-simulator   ios-device-xcode-clang   ios-simulator-xcode-clang   ios-device   linux_chromium_tsan_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_rel_ng   linux_chromium_chromeos_ozone_rel_ng   linux_chromium_chromeos_rel_ng   linux_chromium_asan_rel_ng   chromium_presubmit   chromeos_amd64-generic_chromium_compile_only_ng   chromeos_daisy_chromium_compile_only_ng   linux_android_rel_ng   cast_shell_linux   cast_shell_android   android_n5x_swarming_rel   android_compile_dbg   android_cronet   android_arm64_dbg_recipe   android_clang_dbg_recipe   win_clang   win_chromium_rel_ng   win_chromium_x64_rel_ng   win_chromium_compile_dbg_ng   mac_chromium_compile_dbg_ng   mac_chromium_rel_ng   ios-simulator-xcode-clang   linux_chromium_tsan_rel_ng   ios-device-xcode-clang   ios-device   ios-simulator   linux_chromium_rel_ng   linux_chromium_compile_dbg_ng   linux_chromium_chromeos_rel_ng   linux_chromium_asan_rel_ng   linux_chromium_chromeos_ozone_rel_ng   chromium_presubmit   chromeos_amd64-generic_chromium_compile_only_ng   cast_shell_linux   chromeos_daisy_chromium_compile_only_ng   android_n5x_swarming_rel   linux_android_rel_ng   cast_shell_android   android_cronet   android_compile_dbg   android_clang_dbg_recipe   android_arm64_dbg_recipe 
Commit queue not available (can’t edit this change).

Messages

Total messages: 53 (30 generated)
alex clarke (OOO till 29th)
I think my thought is he TimeDomain::Observer interface is wrong. The Throttler is the only ...
4 months, 3 weeks ago (2017-03-30 14:05:20 UTC) #1
altimin
PTAL. (I'll add some more tests tomorrow)
4 months ago (2017-04-20 19:13:49 UTC) #2
alex clarke (OOO till 29th)
https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode265 third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:265: base::Optional<EnqueueOrder> current_enqueue_order; This doesn't seem to be used? https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc ...
4 months ago (2017-04-21 09:14:20 UTC) #7
altimin
PTAL. Please note that due to the current mechanism we can't block immediate task (InsertFencePosition::NOW ...
3 months, 4 weeks ago (2017-04-25 13:22:36 UTC) #8
Sami
The wake up logic seems good to me. Sorry for all the dumb questions below ...
3 months, 4 weeks ago (2017-04-26 13:09:08 UTC) #13
alex clarke (OOO till 29th)
https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc File third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc#newcode1901 third_party/WebKit/Source/platform/scheduler/renderer/renderer_scheduler_impl.cc:1901: task_queue_throttler()->CreateWakeUpBudgetPool("renderer-wake-up-pool"); I wonder if we should use a builder ...
3 months, 4 weeks ago (2017-04-26 13:31:55 UTC) #14
altimin
PTAL https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc#newcode311 third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.cc:311: base::TimeDelta::FromMicroseconds(1); On 2017/04/26 13:09:08, Sami wrote: > Would ...
3 months, 4 weeks ago (2017-04-26 14:31:30 UTC) #15
alex clarke (OOO till 29th)
https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode220 third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:220: class BLINK_PLATFORM_EXPORT WakeupBudgetPool : public BudgetPool { On 2017/04/25 ...
3 months, 3 weeks ago (2017-04-27 10:28:14 UTC) #20
altimin
PTAL https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode220 third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:220: class BLINK_PLATFORM_EXPORT WakeupBudgetPool : public BudgetPool { On ...
3 months, 3 weeks ago (2017-04-27 11:07:41 UTC) #21
Sami
Thanks, couple of more questions below. Maybe I just need some ASCII art or a ...
3 months, 3 weeks ago (2017-04-27 12:12:30 UTC) #22
altimin
https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc (right): https://codereview.chromium.org/2778123003/diff/80001/third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc#newcode339 third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.cc:339: if (CanRunTasksUntil(queue, now, next_desired_run_time.value())) { On 2017/04/27 12:12:30, Sami ...
3 months, 3 weeks ago (2017-04-27 12:23:51 UTC) #24
Sami
So it sounded like you guys converged on this approach after all? I think lg2me ...
3 months, 3 weeks ago (2017-04-29 17:43:02 UTC) #28
alex clarke (OOO till 29th)
https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode265 third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:265: base::Optional<EnqueueOrder> current_enqueue_order; On 2017/04/25 13:22:34, altimin wrote: > On ...
3 months, 3 weeks ago (2017-05-02 10:51:54 UTC) #29
altimin
ptal https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h (right): https://codereview.chromium.org/2778123003/diff/40001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode265 third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:265: base::Optional<EnqueueOrder> current_enqueue_order; On 2017/05/02 10:51:54, alex clarke wrote: ...
3 months, 3 weeks ago (2017-05-02 18:16:58 UTC) #32
Sami
Couple of more questions and then I promise I will stop :) https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h ...
3 months, 3 weeks ago (2017-05-03 16:53:05 UTC) #35
altimin
https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2778123003/diff/140001/third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h#newcode49 third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:49: virtual bool CanRunTasksAt(base::TimeTicks moment, bool is_wake_up) const = 0; ...
3 months, 2 weeks ago (2017-05-04 10:50:06 UTC) #38
Sami
lgtm, thanks! Alex I assume you're happy too? https://codereview.chromium.org/2778123003/diff/160001/third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc File third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc (right): https://codereview.chromium.org/2778123003/diff/160001/third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc#newcode71 third_party/WebKit/Source/platform/scheduler/renderer/wake_up_budget_pool.cc:71: // ...
3 months, 2 weeks ago (2017-05-04 13:25:46 UTC) #41
alex clarke (OOO till 29th)
Yeah LGTM.
3 months, 2 weeks ago (2017-05-04 13:27:53 UTC) #42
altimin
+vollick@ for platform/BUILD.gn
3 months, 2 weeks ago (2017-05-04 15:16:30 UTC) #44
altimin
+jochen@, can you stamp platform/BUILD.gn please?
3 months, 2 weeks ago (2017-05-05 10:05:43 UTC) #46
jochen (gone - plz use gerrit)
lgtm
3 months, 2 weeks ago (2017-05-05 14:34:35 UTC) #47
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/2778123003/160001
3 months, 2 weeks ago (2017-05-05 16:47:56 UTC) #49
commit-bot: I haz the power
3 months, 2 weeks ago (2017-05-05 18:50:51 UTC) #53
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/db65a6534a5e4ee5bdc0e5d31bde...
Sign in to reply to this message.

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