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

Issue 2258713004: Make tasks cancellable inside the blink scheduler. (Closed)

Created:
4 years, 4 months ago by alex clarke (OOO till 29th)
Modified:
4 years, 4 months ago
Reviewers:
haraken, Sami
CC:
chromium-reviews, blink-reviews, dglazkov+blink, scheduler-bugs_chromium.org, blink-reviews-api_chromium.org, haraken, kinuko
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make tasks cancellable inside the blink scheduler. The complexity of WorkQueue got a bit worse O(log n) insert & pop but this seems unlikely to matter in prartice because the queues never really get big enough for this to matter. Follow on patches will enable Timer<> to use the new style of cancellation which result in smaller queues. Note the cost of running a NOP task is currently quite high so overall we should get a net benefit once Blink tasks use the new API even if this patch adds extra overhead. BUG=638542, 605718 Committed: https://crrev.com/098f0e5e5786dbccfe2aef2d9b8319dc92798b99 Cr-Commit-Position: refs/heads/master@{#413120}

Patch Set 1 #

Total comments: 25

Patch Set 2 : Responding to feedback. #

Patch Set 3 : Better error messageon dcheck #

Patch Set 4 : Faster set insert using hints for amortized O(1) operations #

Patch Set 5 : Yet more logging for the dcheck #

Patch Set 6 : Fix cross thread delayed task ordering bug #

Total comments: 12

Patch Set 7 : Remove whitespace #

Patch Set 8 : Various comment nits addressed #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1111 lines, -113 lines) Patch
M third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h View 1 1 chunk +8 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/enqueue_order.cc View 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h View 1 2 3 4 5 6 7 9 chunks +71 lines, -8 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc View 1 2 3 4 5 6 13 chunks +254 lines, -46 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_manager_unittest.cc View 1 3 chunks +386 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/task_queue_selector_unittest.cc View 2 chunks +12 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/time_domain.h View 2 chunks +16 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/time_domain.cc View 1 2 3 4 5 6 7 1 chunk +37 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/time_domain_unittest.cc View 2 chunks +78 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/work_queue.h View 1 2 3 4 5 6 7 3 chunks +17 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/work_queue.cc View 1 2 3 4 5 6 3 chunks +59 lines, -28 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/work_queue_sets.h View 1 2 3 4 5 6 7 2 chunks +13 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/work_queue_sets.cc View 1 2 3 4 5 6 3 chunks +36 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/base/work_queue_sets_unittest.cc View 1 2 3 3 chunks +55 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/compositor_worker_scheduler.cc View 1 chunk +18 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/public/platform/scheduler/base/task_queue.h View 3 chunks +47 lines, -1 line 0 comments Download

Messages

Total messages: 44 (35 generated)
alex clarke (OOO till 29th)
PTAL :)
4 years, 4 months ago (2016-08-18 13:28:13 UTC) #4
Sami
Looks awesome. Thanks for adding the class-level comments too. Added some minor observations. I'll cc ...
4 years, 4 months ago (2016-08-18 15:53:44 UTC) #9
alex clarke (OOO till 29th)
PTAL https://codereview.chromium.org/2258713004/diff/1/third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h File third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h (right): https://codereview.chromium.org/2258713004/diff/1/third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h#newcode18 third_party/WebKit/Source/platform/scheduler/base/enqueue_order.h:18: // A 64bit integer used to provide ordering ...
4 years, 4 months ago (2016-08-18 16:51:41 UTC) #10
Sami
lgtm -- let's give Kentaro a chance to take a look too. https://codereview.chromium.org/2258713004/diff/1/third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc File third_party/WebKit/Source/platform/scheduler/child/webthread_impl_for_worker_scheduler.cc ...
4 years, 4 months ago (2016-08-18 17:12:18 UTC) #13
haraken
Sorry about the delayed review. LGTM. https://codereview.chromium.org/2258713004/diff/100001/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/2258713004/diff/100001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode129 third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:129: // enqueued on ...
4 years, 4 months ago (2016-08-19 10:42:51 UTC) #34
alex clarke (OOO till 29th)
Thanks, all comments addressed. https://codereview.chromium.org/2258713004/diff/100001/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/2258713004/diff/100001/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h#newcode129 third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.h:129: // enqueued on the |immediate_incoming_queue_|. ...
4 years, 4 months ago (2016-08-19 11:02:59 UTC) #37
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/2258713004/140001
4 years, 4 months ago (2016-08-19 11:03:38 UTC) #40
commit-bot: I haz the power
Committed patchset #8 (id:140001)
4 years, 4 months ago (2016-08-19 12:56:00 UTC) #42
commit-bot: I haz the power
4 years, 4 months ago (2016-08-19 12:57:59 UTC) #44
Message was sent while issue was closed.
Patchset 8 (id:??) landed as
https://crrev.com/098f0e5e5786dbccfe2aef2d9b8319dc92798b99
Cr-Commit-Position: refs/heads/master@{#413120}

Powered by Google App Engine
This is Rietveld 408576698