Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2546423002:
[Try # 3] Scheduler refactoring to virtually eliminate redundant DoWorks (Closed)
|
Created:
4 years ago by alex clarke (OOO till 29th) Modified:
3 years, 10 months ago CC:
chromium-reviews, blink-reviews, cc-bugs_chromium.org, scheduler-bugs_chromium.org, altimin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionScheduler refactoring to virtually eliminate redundant DoWorks
Note this patch, while useful in it's own right, needs base messageloop
cancellation support for full effect. Until that patch lands there may be
some pointless wakeups for canceled DoWorks. This is of particular concern
for using the scheduler on the compositor thread.
Includes fix for telemetry and tsan issues. Also fixes a lock order
inversion present in the second attempt.
BUG=578176, 674238, 674157, 677404, 677041
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c
Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0
Cr-Original-Original-Commit-Position: refs/heads/master@{#438173}
Cr-Original-Commit-Position: refs/heads/master@{#438857}
Review-Url: https://codereview.chromium.org/2546423002
Cr-Commit-Position: refs/heads/master@{#446380}
Committed: https://chromium.googlesource.com/chromium/src/+/eda1d2e08c6769ebdbae66db618a2fae609f170c
Patch Set 1 #Patch Set 2 : Few tweaks #
Total comments: 17
Patch Set 3 : for fun try work_batch_size_ = 100 #Patch Set 4 : Responding to feedback and setting work_batch_size back to 4 #Patch Set 5 : Rebased #
Total comments: 32
Patch Set 6 : Responding to feedback and adding some tests #Patch Set 7 : Added the MoveableAutoLock per Sami's suggestion #
Total comments: 14
Patch Set 8 : Fixing nits #Patch Set 9 : Fix nested messageloop problem #Patch Set 10 : Fix lock order inversion #
Total comments: 2
Patch Set 11 : Fix telemetry plus address feedback #Patch Set 12 : Rebase plus a number of small incremental fixes #Patch Set 13 : Fix compile #Patch Set 14 : Simplify immediate task relocading code for clarity #Patch Set 15 : Add an extra dcheck #
Total comments: 2
Patch Set 16 : Rebased #Patch Set 17 : Rebased #Patch Set 18 : Rebase it's even smaller now! #
Total comments: 12
Patch Set 19 : Fixed a bug #Patch Set 20 : Rebased #Messages
Total messages: 145 (110 generated)
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks BUG= ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by alexclarke@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...
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
I think overall it makes sense to be smarter about when to post DoWork(), and this seems like a good way to accomplish that. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:345: void TaskQueueImpl::OnIncomingImmediateTaskAvailable() { I think having this method makes sense but the name feels misleading because it's not called synchronously in response to an immediate task becoming available, and hence needs to do some extra checking. Should we call this something like ReloadImmediateWorkQueue()? (Could have Maybe in there too if you prefer.) https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:184: (!queue->IsQueueEnabled() || queue->BlockedByFenceLocked())) { Hmm, we're not holding the any thread lock here, are we? https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:205: const tracked_objects::Location& from_here) { Can we check that a spinlock is held? Not sure... We could add a debug-only flag if necessary. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:253: void TaskQueueManager::DoWork(bool delayed) { meta-comment: This function is getting long -- maybe we should split it into more helpers? https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:256: delayed ? "false" : "true"); nit: you can just pass |delayed| here and tracing will know to turn it into a boolean. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:146: // need them, you can turn them off. Or we could consider deleting them :) (Don't recall us looking at that data much.) https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:252: std::set<internal::TaskQueueImpl*> has_incomming_immediate_work_; Does this need to be ordered by the way?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
PTAL https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:345: void TaskQueueImpl::OnIncomingImmediateTaskAvailable() { On 2016/12/05 17:52:54, Sami wrote: > I think having this method makes sense but the name feels misleading because > it's not called synchronously in response to an immediate task becoming > available, and hence needs to do some extra checking. Should we call this > something like ReloadImmediateWorkQueue()? (Could have Maybe in there too if you > prefer.) Done. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:184: (!queue->IsQueueEnabled() || queue->BlockedByFenceLocked())) { On 2016/12/05 17:52:54, Sami wrote: > Hmm, we're not holding the any thread lock here, are we? Actually we are but that's perhaps a little fragile WRT subsequent patches. Call stack is TaskQueueImpl::BlockedByFenceLocked TaskQueueManager::OnQueueHasImmediateWork TaskQueueImpl::PushOntoImmediateIncomingQueueLocked ... Note TaskQueueImpl::BlockedByFenceLocked has any_thread_lock_.AssertAcquired(); Perhaps the cleanest thing to do is to pull the logic checking if we should call MaybeScheduleImmediateWorkLocked back out of the function? https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:205: const tracked_objects::Location& from_here) { On 2016/12/05 17:52:54, Sami wrote: > Can we check that a spinlock is held? Not sure... > > We could add a debug-only flag if necessary. Currently no it's not possible to do so in a good way. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:253: void TaskQueueManager::DoWork(bool delayed) { On 2016/12/05 17:52:54, Sami wrote: > meta-comment: This function is getting long -- maybe we should split it into > more helpers? Done. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:256: delayed ? "false" : "true"); On 2016/12/05 17:52:54, Sami wrote: > nit: you can just pass |delayed| here and tracing will know to turn it into a > boolean. Done. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:146: // need them, you can turn them off. On 2016/12/05 17:52:54, Sami wrote: > Or we could consider deleting them :) (Don't recall us looking at that data > much.) Lets make that decision in a different patch? https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:252: std::set<internal::TaskQueueImpl*> has_incomming_immediate_work_; On 2016/12/05 17:52:55, Sami wrote: > Does this need to be ordered by the way? No. I expect this to be mostly empty, it would be a good candidate for a small set if that lands.
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch requires the base messageloop to support cancellation. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Thanks for the fixes! Added a few more observations. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:184: (!queue->IsQueueEnabled() || queue->BlockedByFenceLocked())) { On 2016/12/06 17:37:54, alex clarke wrote: > On 2016/12/05 17:52:54, Sami wrote: > > Hmm, we're not holding the any thread lock here, are we? > > Actually we are but that's perhaps a little fragile WRT subsequent patches. > Call stack is > > TaskQueueImpl::BlockedByFenceLocked > TaskQueueManager::OnQueueHasImmediateWork > TaskQueueImpl::PushOntoImmediateIncomingQueueLocked > ... > > Note TaskQueueImpl::BlockedByFenceLocked has any_thread_lock_.AssertAcquired(); > > Perhaps the cleanest thing to do is to pull the logic checking if we should call > MaybeScheduleImmediateWorkLocked back out of the function? Yeah, something like that would be better. The coupling between TaskQueueImpl and TaskQueueManager is already pretty tight and I wouldn't want them to start making assumptions about each other's critical sections. https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:205: const tracked_objects::Location& from_here) { On 2016/12/06 17:37:54, alex clarke wrote: > On 2016/12/05 17:52:54, Sami wrote: > > Can we check that a spinlock is held? Not sure... > > > > We could add a debug-only flag if necessary. > > Currently no it's not possible to do so in a good way. I was thinking something like a checked spinlock that also uses a regular lock in debug builds. Can be left for later too... https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2546423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:146: // need them, you can turn them off. On 2016/12/06 17:37:54, alex clarke wrote: > On 2016/12/05 17:52:54, Sami wrote: > > Or we could consider deleting them :) (Don't recall us looking at that data > > much.) > Lets make that decision in a different patch? Sure, no problem. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:61: do_work_pending_lock_(), // NOTE this calls the constructor! Is this gonna work? This[1] says spinlocks can only be static globals -- presumably because they don't have initializers or even constructors to initialize them. [1] https://cs.chromium.org/chromium/src/base/synchronization/spin_lock.h?rcl=148... https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:171: SpinLock::Guard guard(do_work_pending_lock_); Seems risky holding to this during the PostTask -- could we move this below it? https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:200: // Unlwss we're ensted, try to avoid posting redundant DoWorks. typo: Unless we're nested https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:206: delegate_->PostTask(from_here, immediate_do_work_closure_); Ditto about the spinlock and the PostTask. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:312: SpinLock::Guard guard(do_work_pending_lock_); Could you also delimit this scope explicitly? Feels like it's too easy to add stuff at the end not realizing the spinlock is being held. Also not sure if everything here needs to be in that scope... https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:333: void TaskQueueManager::PostDoWorkContinuation( ...Locked()? https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:350: delegate_->PostTask(FROM_HERE, immediate_do_work_closure_); Ditto about calling into the delegate/selector/time domains while holding the spinlock. Maybe I'm worrying too much about this? Feels like we should be a little careful still. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:372: if (!next_continuation || next_continuation.value() < continuation.value()) Should '<' be '>' or am I missing something? https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:378: bool TaskQueueManager::SelectWorkQueueToService( ...Locked()? https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:575: state->SetBoolean("is_nested", is_nested_); Looks like this should be inside the lock. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:577: SpinLock::Guard guard(do_work_pending_lock_); nit: explicit scope please. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:258: mutable WTF::SpinLock do_work_pending_lock_; nit: I think this could just be base::subtle::Spinlock since we already use base locking primitives for everything else. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:259: int do_work_running_count_; Should we wrap these into a struct like we do in the scheduler? https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:261: std::set<internal::TaskQueueImpl*> has_incomming_immediate_work_; typo: incoming https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:66: // if possible and return true if time was advanced. nit: Comment needs updating. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:39: // There is a small overhead to recording task delay histograms, we may not nit: reflow
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch requires the base messageloop to support cancellation. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch requires the base messageloop to support cancellation. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:61: do_work_pending_lock_(), // NOTE this calls the constructor! On 2016/12/07 16:13:42, Sami wrote: > Is this gonna work? This[1] says spinlocks can only be static globals -- > presumably because they don't have initializers or even constructors to > initialize them. > > [1] > https://cs.chromium.org/chromium/src/base/synchronization/spin_lock.h?rcl=148... Acknowledged. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:171: SpinLock::Guard guard(do_work_pending_lock_); On 2016/12/07 16:13:42, Sami wrote: > Seems risky holding to this during the PostTask -- could we move this below it? Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:200: // Unlwss we're ensted, try to avoid posting redundant DoWorks. On 2016/12/07 16:13:42, Sami wrote: > typo: Unless we're nested Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:312: SpinLock::Guard guard(do_work_pending_lock_); On 2016/12/07 16:13:42, Sami wrote: > Could you also delimit this scope explicitly? Feels like it's too easy to add > stuff at the end not realizing the spinlock is being held. Also not sure if > everything here needs to be in that scope... Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:333: void TaskQueueManager::PostDoWorkContinuation( On 2016/12/07 16:13:42, Sami wrote: > ...Locked()? Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:350: delegate_->PostTask(FROM_HERE, immediate_do_work_closure_); On 2016/12/07 16:13:42, Sami wrote: > Ditto about calling into the delegate/selector/time domains while holding the > spinlock. Maybe I'm worrying too much about this? Feels like we should be a > little careful still. Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:372: if (!next_continuation || next_continuation.value() < continuation.value()) On 2016/12/07 16:13:42, Sami wrote: > Should '<' be '>' or am I missing something? Good catch, I added a test. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:378: bool TaskQueueManager::SelectWorkQueueToService( On 2016/12/07 16:13:42, Sami wrote: > ...Locked()? This doesn't need to be locked? https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:575: state->SetBoolean("is_nested", is_nested_); On 2016/12/07 16:13:42, Sami wrote: > Looks like this should be inside the lock. Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:577: SpinLock::Guard guard(do_work_pending_lock_); On 2016/12/07 16:13:42, Sami wrote: > nit: explicit scope please. Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:258: mutable WTF::SpinLock do_work_pending_lock_; On 2016/12/07 16:13:42, Sami wrote: > nit: I think this could just be base::subtle::Spinlock since we already use base > locking primitives for everything else. I'm actually thinking we should initially use a mutex and try a spinlock in a subsequent patch. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:259: int do_work_running_count_; On 2016/12/07 16:13:43, Sami wrote: > Should we wrap these into a struct like we do in the scheduler? Good idea. Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:261: std::set<internal::TaskQueueImpl*> has_incomming_immediate_work_; On 2016/12/07 16:13:43, Sami wrote: > typo: incoming Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/time_domain.h (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/time_domain.h:66: // if possible and return true if time was advanced. On 2016/12/07 16:13:43, Sami wrote: > nit: Comment needs updating. Done. https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/child/scheduler_helper.h:39: // There is a small overhead to recording task delay histograms, we may not On 2016/12/07 16:13:43, Sami wrote: > nit: reflow Done.
The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run
https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:206: delegate_->PostTask(from_here, immediate_do_work_closure_); On 2016/12/07 16:13:42, Sami wrote: > Ditto about the spinlock and the PostTask. Done.
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 refactoring to virtually elimate redundant DoWorks Note this patch requires the base messageloop to support cancellation. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch requires the base messageloop to support cancellation and until that patch lands there may be some pointless wakeups for canceled DoWorks. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Great! lgtm with a few nits. https://codereview.chromium.org/2546423002/diff/120001/cc/scheduler/begin_fra... File cc/scheduler/begin_frame_source_unittest.cc (right): https://codereview.chromium.org/2546423002/diff/120001/cc/scheduler/begin_fra... cc/scheduler/begin_frame_source_unittest.cc:93: // There was a task posed but it's canceled. typo: posted https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/moveable_auto_lock.h (right): https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/moveable_auto_lock.h:1: // Copyright 206 The Chromium Authors. All rights reserved. 2016 :) https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:244: TRACE_EVENT1("tracing_category_", "MaybeScheduleDelayedWorkInternal", "delay", Probably did not mean to add quotes around tracing_category_. https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:245: delay.InSecondsF()); nit: Could you do InMillisecondsF() and "delay_ms" to match the existing trace events? https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:297: work_queue = nullptr; // The queue may have been unregistered. Looks like this line could be removed now (even before this patch). https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:351: // If a delayed DoWork is pending then we don't need to post a nit: reflow https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:604: state->SetInteger("immediate_do_work_posted", nit: _count
The CQ bit was checked by alexclarke@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...
+altimin@ you might want to have a look at this as well. https://codereview.chromium.org/2546423002/diff/120001/cc/scheduler/begin_fra... File cc/scheduler/begin_frame_source_unittest.cc (right): https://codereview.chromium.org/2546423002/diff/120001/cc/scheduler/begin_fra... cc/scheduler/begin_frame_source_unittest.cc:93: // There was a task posed but it's canceled. On 2016/12/09 11:04:02, Sami wrote: > typo: posted I found a way to remove this change. https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/moveable_auto_lock.h (right): https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/moveable_auto_lock.h:1: // Copyright 206 The Chromium Authors. All rights reserved. On 2016/12/09 11:04:02, Sami wrote: > 2016 :) Done. https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:244: TRACE_EVENT1("tracing_category_", "MaybeScheduleDelayedWorkInternal", "delay", On 2016/12/09 11:04:02, Sami wrote: > Probably did not mean to add quotes around tracing_category_. Done. https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:245: delay.InSecondsF()); On 2016/12/09 11:04:02, Sami wrote: > nit: Could you do InMillisecondsF() and "delay_ms" to match the existing trace > events? Done. https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:297: work_queue = nullptr; // The queue may have been unregistered. On 2016/12/09 11:04:02, Sami wrote: > Looks like this line could be removed now (even before this patch). Done. https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:351: // If a delayed DoWork is pending then we don't need to post a On 2016/12/09 11:04:02, Sami wrote: > nit: reflow Done. https://codereview.chromium.org/2546423002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:604: state->SetInteger("immediate_do_work_posted", On 2016/12/09 11:04:02, Sami wrote: > nit: _count Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by alexclarke@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...
alexclarke@chromium.org changed reviewers: + haraken@chromium.org
haraken@chromium.org: Please review changes in third_party/WebKit/Source/platform/BUILD.gn
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by alexclarke@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...
On 2016/12/12 17:21:17, alex clarke wrote: > mailto:haraken@chromium.org: Please review changes in > third_party/WebKit/Source/platform/BUILD.gn rs LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@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/2546423002/#ps160001 (title: "Fix nested messageloop problem")
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 refactoring to virtually elimate redundant DoWorks Note this patch requires the base messageloop to support cancellation and until that patch lands there may be some pointless wakeups for canceled DoWorks. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch while useful in it;'s own right needse base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for putitng the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch while useful in it;'s own right needse base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for putitng the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch while useful in it's own right needse base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch while useful in it's own right needse base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch while useful in it's own right needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch while useful in it's own right needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481642605162090,
"parent_rev": "6bbac82231faf27ab64cbd73df6e7e047225984d", "commit_rev":
"6c09b8150b975d4fa88b755516a4cab18fd62a85"}
Message was sent while issue was closed.
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2546423002 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2546423002 ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173}
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2574363002/ by gab@chromium.org. The reason for reverting is: TSAN lock-order-inversion: http://crbug.com/674238.
Message was sent while issue was closed.
On 2016/12/14 19:34:19, gab wrote: > A revert of this CL (patchset #9 id:160001) has been created in > https://codereview.chromium.org/2574363002/ by mailto:gab@chromium.org. > > The reason for reverting is: TSAN lock-order-inversion: http://crbug.com/674238. This was also causing problems on telemetry runs. crbug.com/674157
Message was sent while issue was closed.
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ========== to ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176, 674238 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ==========
Description was changed from ========== Scheduler refactoring to virtually elimate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176, 674238 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176, 674238 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ==========
Description was changed from ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176, 674238 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ==========
The CQ bit was checked by alexclarke@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...
Sami can you take another look? I've fixed the lock order inversion. BTW I'm going to check that v8.top_25_smooth works too.
The CQ bit was unchecked by alexclarke@chromium.org
https://codereview.chromium.org/2546423002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:279: for (internal::TaskQueueImpl* queue : has_incoming_immediate_work) { Would this fit better into UpdateWorkQueues?
PTAL https://codereview.chromium.org/2546423002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:279: for (internal::TaskQueueImpl* queue : has_incoming_immediate_work) { On 2016/12/15 11:45:45, Sami wrote: > Would this fit better into UpdateWorkQueues? Done.
PTAL
The CQ bit was checked by alexclarke@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, thanks.
The CQ bit was unchecked by alexclarke@chromium.org
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2546423002/#ps200001 (title: "Fix telemetry plus address feedback")
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 refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ==========
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by alexclarke@chromium.org
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": 200001, "attempt_start_ts": 1481818171558270,
"parent_rev": "a526d7058682e37c6137f2aad4e720f327904762", "commit_rev":
"abade360cae02e93e2e267abb60d4971250584f4"}
Message was sent while issue was closed.
Description was changed from ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} Review-Url: https://codereview.chromium.org/2546423002 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Cr-Commit-Position: refs/heads/master@{#438173} Review-Url: https://codereview.chromium.org/2546423002 ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Commit-Position: refs/heads/master@{#438857}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/2590593002/ by gab@chromium.org. The reason for reverting is: http://crbug.com/674895 (revert of dependent CL must land before this one: https://codereview.chromium.org/2583333002/).
Message was sent while issue was closed.
Description was changed from ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ==========
Description was changed from ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157, 677404 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ==========
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by alexclarke@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 refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157, 677404 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157, 677404, 677041 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ==========
alexclarke@chromium.org changed reviewers: + altimin@chromium.org
Sami and Alexander would you mind taking a look at this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2546423002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:331: void TaskQueueManager::PostDoWorkContinuationLocked( Can you please add a DCHECK that we're on the main thread?
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
Description was changed from ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. BUG=578176, 674238, 674157, 677404, 677041 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. Also fixes a lock order inversion present in the second attempt. BUG=578176, 674238, 674157, 677404, 677041 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ==========
Sami can you have another look please? I think we're ready to try this again. https://codereview.chromium.org/2546423002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:331: void TaskQueueManager::PostDoWorkContinuationLocked( On 2017/01/17 17:25:54, altimin wrote: > Can you please add a DCHECK that we're on the main thread? Done.
The CQ bit was checked by alexclarke@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks again for simplifying this! Logic lgtm now, but left a few suggestions for making some things more obvious. https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:219: return; Could we DCHECK here that ComputeDelayTillNextTaskLocked returns a zero delay, i.e., that DoWork will actually post a continuation for us? https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:245: return; Similarly a DCHECK that verifies the continuation will be there might be useful. https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:284: any_thread().is_nested = is_nested; If I understood right, we only need to update this flag *between* tasks (and also only when !is_nested) to detect when a nested message loop has exited, right? Should we move the setting part to an else branch below to make that more obvious? https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:412: // Unfortunately because |any_thread_lock_| is held it's not safe to call I guess we could call ReloadWorkQueues() right before entering the lock but as far as I can tell it shouldn't make a huge difference since we reload before every task we run and OnQueueHasIncomingImmediateWork() will ensure a DoWork will eventually happen. https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:291: base::TimeTicks next_delayed_do_work_; nit: Should we call this next_delayed_do_work_time_, next_posted_delayed_do_work_ or something to make the purpose a little more obvious and match the other names better (e.g., *_closure)? https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:292: base::CancelableClosure cancelable_delayed_do_work_closure_; Mind moving this next to |delayed_do_work_closure_| since they're pretty related?
PTAL https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc (right): https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:219: return; On 2017/01/26 12:29:17, Sami wrote: > Could we DCHECK here that ComputeDelayTillNextTaskLocked returns a zero delay, > i.e., that DoWork will actually post a continuation for us? Unfortunately ComputeDelayTillNextTaskLocked has side effects for virtual time domains. Otherwise this could be made to work with: if (delegate_->BelongsToCurrentThread()) { LazyNow lazy_now(real_time_domain()->CreateLazyNow()); DCHECK_EQ(ComputeDelayTillNextTaskLocked(&lazy_now).value(), base::TimeDelta()); } https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:245: return; On 2017/01/26 12:29:17, Sami wrote: > Similarly a DCHECK that verifies the continuation will be there might be useful. Same problem here. https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:284: any_thread().is_nested = is_nested; On 2017/01/26 12:29:17, Sami wrote: > If I understood right, we only need to update this flag *between* tasks (and > also only when !is_nested) to detect when a nested message loop has exited, > right? Should we move the setting part to an else branch below to make that more > obvious? Done. https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.cc:412: // Unfortunately because |any_thread_lock_| is held it's not safe to call On 2017/01/26 12:29:17, Sami wrote: > I guess we could call ReloadWorkQueues() right before entering the lock but as > far as I can tell it shouldn't make a huge difference since we reload before > every task we run and OnQueueHasIncomingImmediateWork() will ensure a DoWork > will eventually happen. So in theory the check now is entirely equivalent to calling ReloadWorkQueues and then seeing if the selector has something to do. There was actually a missing case (that of a task enabled by inserting a new fence). I've added tests to cover that. https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h (right): https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:291: base::TimeTicks next_delayed_do_work_; On 2017/01/26 12:29:17, Sami wrote: > nit: Should we call this next_delayed_do_work_time_, > next_posted_delayed_do_work_ or something to make the purpose a little more > obvious and match the other names better (e.g., *_closure)? Done. https://codereview.chromium.org/2546423002/diff/340001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/scheduler/base/task_queue_manager.h:292: base::CancelableClosure cancelable_delayed_do_work_closure_; On 2017/01/26 12:29:17, Sami wrote: > Mind moving this next to |delayed_do_work_closure_| since they're pretty > related? Done.
The CQ bit was checked by alexclarke@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...
The CQ bit was checked by alexclarke@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...
Thanks, still lgtm.
The CQ bit was unchecked by alexclarke@chromium.org
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2546423002/#ps380001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by alexclarke@chromium.org
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": 380001, "attempt_start_ts": 1485452538818440,
"parent_rev": "3ae6f734f3566484634a068cbe663bbccc24937c", "commit_rev":
"eda1d2e08c6769ebdbae66db618a2fae609f170c"}
Message was sent while issue was closed.
Description was changed from ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. Also fixes a lock order inversion present in the second attempt. BUG=578176, 674238, 674157, 677404, 677041 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Commit-Position: refs/heads/master@{#438173} Cr-Commit-Position: refs/heads/master@{#438857} ========== to ========== Scheduler refactoring to virtually eliminate redundant DoWorks Note this patch, while useful in it's own right, needs base messageloop cancellation support for full effect. Until that patch lands there may be some pointless wakeups for canceled DoWorks. This is of particular concern for using the scheduler on the compositor thread. Includes fix for telemetry and tsan issues. Also fixes a lock order inversion present in the second attempt. BUG=578176, 674238, 674157, 677404, 677041 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Committed: https://crrev.com/3a6ffff92addfa71d401e2718ab99677520f250c Committed: https://crrev.com/8c6cf02ab73c8026ac2b0ca2aa8d6fd9bd8f2db0 Cr-Original-Original-Commit-Position: refs/heads/master@{#438173} Cr-Original-Commit-Position: refs/heads/master@{#438857} Review-Url: https://codereview.chromium.org/2546423002 Cr-Commit-Position: refs/heads/master@{#446380} Committed: https://chromium.googlesource.com/chromium/src/+/eda1d2e08c6769ebdbae66db618a... ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001) as https://chromium.googlesource.com/chromium/src/+/eda1d2e08c6769ebdbae66db618a... |
