Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781323003:
[scheduler] Split lock in TaskQueueImpl. (Closed)
Description[scheduler] Split lock in TaskQueueImpl.
Move immediate_incoming_queue from AnyThread and protect it with its
own lock. This will allow to call TaskQueue::HasPendingImmediateWork
and some other methods from TimeDomain::OnTimeDomainHasImmediateWork.
BUG=699541
Review-Url: https://codereview.chromium.org/2781323003
Cr-Commit-Position: refs/heads/master@{#461443}
Committed: https://chromium.googlesource.com/chromium/src/+/d7d63db66473bc81e84e1546c346566745f1f8aa
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Addressed comments #
Messages
Total messages: 24 (16 generated)
Description was changed from ========== [scheduler] Split lock into TaskQueueImpl. Move immediate_incoming_queue from AnyThread and protect it with its own lock. This will allow to call TaskQueue::HasPendingImmediateWork and some other methods from TimeDomain::OnTimeDomainHasImmediateWork. BUG= ========== to ========== [scheduler] Split lock in TaskQueueImpl. Move immediate_incoming_queue from AnyThread and protect it with its own lock. This will allow to call TaskQueue::HasPendingImmediateWork and some other methods from TimeDomain::OnTimeDomainHasImmediateWork. BUG=699541 ==========
The CQ bit was checked by altimin@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@chromium.org changed reviewers: + alexclarke@chromium.org, skyostil@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2781323003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2781323003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:354: base::AutoLock lock(immediate_incoming_queue_lock_); Can we avoid locking this twice?
The CQ bit was checked by altimin@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...
PTAL https://codereview.chromium.org/2781323003/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2781323003/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:354: base::AutoLock lock(immediate_incoming_queue_lock_); On 2017/03/30 13:23:46, alex clarke wrote: > Can we avoid locking this twice? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Given that: - It's not straightforward to write the consumer of this notification in a way that avoids re-entrancy. - We want to replace the time domain observer with a task queue observer which can have a stricter contract about which methods it can call re-entrantly (e.g., const-only) - We don't expect this new lock to be contented Let's go ahead with splitting the lock like suggested here. https://codereview.chromium.org/2781323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2781323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:339: bool is_immediate_incoming_queue_empty; s/is/was/ https://codereview.chromium.org/2781323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:362: any_thread().task_queue_manager->DidQueueTask(*queued_task); Let's do this while holding the lock so queued_task remains valid.
The CQ bit was checked by altimin@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...
PTAL https://codereview.chromium.org/2781323003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc (right): https://codereview.chromium.org/2781323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:339: bool is_immediate_incoming_queue_empty; On 2017/04/03 14:29:36, Sami wrote: > s/is/was/ Done. https://codereview.chromium.org/2781323003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc:362: any_thread().task_queue_manager->DidQueueTask(*queued_task); On 2017/04/03 14:29:36, Sami wrote: > Let's do this while holding the lock so queued_task remains valid. Done.
lgtm
The CQ bit was unchecked by altimin@chromium.org
The CQ bit was checked by altimin@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": 40001, "attempt_start_ts": 1491231269411280,
"parent_rev": "6372cfc245d0fcf6a571763036bc0f3a6734ba5d", "commit_rev":
"d7d63db66473bc81e84e1546c346566745f1f8aa"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Split lock in TaskQueueImpl. Move immediate_incoming_queue from AnyThread and protect it with its own lock. This will allow to call TaskQueue::HasPendingImmediateWork and some other methods from TimeDomain::OnTimeDomainHasImmediateWork. BUG=699541 ========== to ========== [scheduler] Split lock in TaskQueueImpl. Move immediate_incoming_queue from AnyThread and protect it with its own lock. This will allow to call TaskQueue::HasPendingImmediateWork and some other methods from TimeDomain::OnTimeDomainHasImmediateWork. BUG=699541 Review-Url: https://codereview.chromium.org/2781323003 Cr-Commit-Position: refs/heads/master@{#461443} Committed: https://chromium.googlesource.com/chromium/src/+/d7d63db66473bc81e84e1546c346... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d7d63db66473bc81e84e1546c346... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
