Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810873008:
Separate the create and start phases in DelayedTaskManager. (Closed)
DescriptionSeparate the create and start phases in DelayedTaskManager.
Tasks added to the DelayedTaskManager before Start() are sent to the
service thread when Start() is called. Tasks added to the
DelayedTaskManager after Start() are sent to the service thread as
they are added. The service thread calls the post task callback of
a task when it becomes ripe for execution.
BUG=690706
Review-Url: https://codereview.chromium.org/2810873008
Cr-Commit-Position: refs/heads/master@{#465988}
Committed: https://chromium.googlesource.com/chromium/src/+/840441cf01a424125e56e25369a75994ec4b9588
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : self-review #
Total comments: 11
Patch Set 4 : rebase #Patch Set 5 : CR-gab-11 #
Total comments: 4
Patch Set 6 : rebase #
Total comments: 6
Patch Set 7 : CR-robliao-gab-28-29 #Patch Set 8 : self-review #
Dependent Patchsets: Messages
Total messages: 45 (34 generated)
The CQ bit was checked by fdoray@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by fdoray@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
lg https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:50: SchedulerLock lock_; The lock is irrelevant after threads have all "seen" Start()'s effect, right? Should we add an AtomicFlag and only use the lock when it's not set? i.e. if (IsSet()) tr->PostTask; else { lock + if tr post /else queue } https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:68: Bind(&MockTaskTarget::PostTaskNowCallback, Unretained(&task_target))); BindOnce https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:70: // Fast-forward time. Don't expect any forwarding to |task_target|. What ensures this? Shouldn't your task trigger a failure if ran? Edit: Oh I see, StrictMock ensures this, that's subtle... hint at this in this comment? (it's more obvious in following tests that are followed by EXPECT_CALL statements but this test is hard to read) https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:98: } Also test that task that became ripe before start is executed upon Start()?
https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:45: &task_tracker_, &delayed_task_manager_); Now that I see the operator& usage, I think we should keep to using .get() for smart pointers when we simply want to get the pointer. Using operator& makes it look like we can mutate the pointer instance. .get() at least is const.
PTAnL https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:45: &task_tracker_, &delayed_task_manager_); On 2017/04/12 19:23:23, robliao wrote: > Now that I see the operator& usage, I think we should keep to using .get() for > smart pointers when we simply want to get the pointer. > > Using operator& makes it look like we can mutate the pointer instance. .get() at > least is const. |delayed_task_manager_| is no longer a std::unique_ptr. Can't use .get().
Waiting on gab@ comments. https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc (right): https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:45: &task_tracker_, &delayed_task_manager_); On 2017/04/13 12:03:53, fdoray wrote: > On 2017/04/12 19:23:23, robliao wrote: > > Now that I see the operator& usage, I think we should keep to using .get() for > > smart pointers when we simply want to get the pointer. > > > > Using operator& makes it look like we can mutate the pointer instance. .get() > at > > least is const. > > |delayed_task_manager_| is no longer a std::unique_ptr. Can't use .get(). Ah, right you are. Missed that.
The CQ bit was checked by fdoray@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 fdoray@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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...) 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 fdoray@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...
PTAnL https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:50: SchedulerLock lock_; On 2017/04/12 19:15:51, gab wrote: > The lock is irrelevant after threads have all "seen" Start()'s effect, right? > > Should we add an AtomicFlag and only use the lock when it's not set? i.e. if > (IsSet()) tr->PostTask; else { lock + if tr post /else queue } Done. https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:68: Bind(&MockTaskTarget::PostTaskNowCallback, Unretained(&task_target))); On 2017/04/12 19:15:51, gab wrote: > BindOnce Done. https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:70: // Fast-forward time. Don't expect any forwarding to |task_target|. On 2017/04/12 19:15:51, gab wrote: > What ensures this? Shouldn't your task trigger a failure if ran? > > Edit: Oh I see, StrictMock ensures this, that's subtle... hint at this in this > comment? (it's more obvious in following tests that are followed by EXPECT_CALL > statements but this test is hard to read) Done. https://codereview.chromium.org/2810873008/diff/40001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:98: } On 2017/04/12 19:15:51, gab wrote: > Also test that task that became ripe before start is executed upon Start()? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm + comments. https://codereview.chromium.org/2810873008/diff/100001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/2810873008/diff/100001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:30: DCHECK(service_thread_task_runner); Nit: Move this DCHECK to the top of the function now that it's in its own scope. https://codereview.chromium.org/2810873008/diff/100001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:35: // |service_thread_task_runner_| must not change after |started_| is set. Add a note explaining the why (accesses to service_thread_task_runner_ are unsynchronized after this or refer to the header.) https://codereview.chromium.org/2810873008/diff/100001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:61: // Fast path. Remove this comment and the one below. It's not clear if noting a fast or slow path adds value here. Instead, state that if started_.IsSet(), then the delayed task manager is in a stable state allowing a direct call to AddDelayedTaskNow. Otherwise, we have to synchronize and recheck.
lgtm w/ comments https://codereview.chromium.org/2810873008/diff/80001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/2810873008/diff/80001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:38: DelayedTaskManager( Add a comment that |tick_clock| can be specified for testing https://codereview.chromium.org/2810873008/diff/80001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/2810873008/diff/80001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:54: // The delayed run time is computed from real time. Use mock time instead. I think this comment is more confusing than no comment... i.e. it's *not* computed from real time since you set a mock tick clock on DelayedTaskManager..? Drop the comment?
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
https://codereview.chromium.org/2810873008/diff/80001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager.h (right): https://codereview.chromium.org/2810873008/diff/80001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager.h:38: DelayedTaskManager( On 2017/04/19 18:59:17, gab wrote: > Add a comment that |tick_clock| can be specified for testing Done. https://codereview.chromium.org/2810873008/diff/80001/base/task_scheduler/del... File base/task_scheduler/delayed_task_manager_unittest.cc (right): https://codereview.chromium.org/2810873008/diff/80001/base/task_scheduler/del... base/task_scheduler/delayed_task_manager_unittest.cc:54: // The delayed run time is computed from real time. Use mock time instead. On 2017/04/19 18:59:17, gab wrote: > I think this comment is more confusing than no comment... i.e. it's *not* > computed from real time since you set a mock tick clock on DelayedTaskManager..? > Drop the comment? Reworded. https://codereview.chromium.org/2810873008/diff/100001/base/task_scheduler/de... File base/task_scheduler/delayed_task_manager.cc (right): https://codereview.chromium.org/2810873008/diff/100001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:30: DCHECK(service_thread_task_runner); On 2017/04/19 17:54:07, robliao wrote: > Nit: Move this DCHECK to the top of the function now that it's in its own scope. Done. https://codereview.chromium.org/2810873008/diff/100001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:35: // |service_thread_task_runner_| must not change after |started_| is set. On 2017/04/19 17:54:07, robliao wrote: > Add a note explaining the why (accesses to service_thread_task_runner_ are > unsynchronized after this or refer to the header.) Done. https://codereview.chromium.org/2810873008/diff/100001/base/task_scheduler/de... base/task_scheduler/delayed_task_manager.cc:61: // Fast path. On 2017/04/19 17:54:07, robliao wrote: > Remove this comment and the one below. It's not clear if noting a fast or slow > path adds value here. > > Instead, state that if started_.IsSet(), then the delayed task manager is in a > stable state allowing a direct call to AddDelayedTaskNow. > > Otherwise, we have to synchronize and recheck. Done.
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2810873008/#ps120001 (title: "CR-robliao-gab-28-29")
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 fdoray@chromium.org
The CQ bit was checked by fdoray@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 fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robliao@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2810873008/#ps140001 (title: "self-review")
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": 140001, "attempt_start_ts": 1492690337953060,
"parent_rev": "5705c2783128e015cf8c9da3f7a1b188d81101b8", "commit_rev":
"840441cf01a424125e56e25369a75994ec4b9588"}
Message was sent while issue was closed.
Description was changed from ========== Separate the create and start phases in DelayedTaskManager. Tasks added to the DelayedTaskManager before Start() are sent to the service thread when Start() is called. Tasks added to the DelayedTaskManager after Start() are sent to the service thread as they are added. The service thread calls the post task callback of a task when it becomes ripe for execution. BUG=690706 ========== to ========== Separate the create and start phases in DelayedTaskManager. Tasks added to the DelayedTaskManager before Start() are sent to the service thread when Start() is called. Tasks added to the DelayedTaskManager after Start() are sent to the service thread as they are added. The service thread calls the post task callback of a task when it becomes ripe for execution. BUG=690706 Review-Url: https://codereview.chromium.org/2810873008 Cr-Commit-Position: refs/heads/master@{#465988} Committed: https://chromium.googlesource.com/chromium/src/+/840441cf01a424125e56e25369a7... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/840441cf01a424125e56e25369a7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
