Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1864333002:
TaskScheduler: Delegate instead of callbacks for SchedulerWorkerThread. (Closed)
DescriptionTaskScheduler: Delegate instead of callbacks for SchedulerWorkerThread.
Instead of requiring multiple callbacks in the SchedulerWorkerThread's
constructor, require a single delegate with these methods:
- OnMainEntry
- OnMainExit (no callback equivalent before this CL)
- GetWork
- RanTaskFromSequence
In a previous CL, we decided that it was better to use callbacks than
a delegate because OnMainEntry/GetWork were handled by
SchedulerThreadPool while RanTaskFromSequence was handled by
TaskScheduler. Now that we've added OnMainExit
(handled by SchedulerThreadPool), we believe that it's better to make
SchedulerThreadPool a delegate of SchedulerWorkerThread.
SchedulerThreadPool will invoke a callback bound to TaskScheduler when
its RanTaskFromSequence method is called.
BUG=553459
Committed: https://crrev.com/80c05e2c9df0f5085bd90748e2239cc41aef24c5
Cr-Commit-Position: refs/heads/master@{#385888}
Patch Set 1 #
Total comments: 5
Patch Set 2 : CR robliao #3 (remove forward declaration) #
Total comments: 6
Patch Set 3 : CR robliao #5 #
Total comments: 12
Patch Set 4 : Add SchedulerWorkerThread* arg to GetWork. #Patch Set 5 : CR gab #9 #Patch Set 6 : Verify that OnMainEntry isn't called more than once. #
Total comments: 6
Patch Set 7 : CR gab #18 (add "The Task is still in |sequence| when this is called.") #
Dependent Patchsets: Messages
Total messages: 33 (12 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
gab@, robliao@: Can you review this CL? Thanks.
https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_thread.cc:71: scoped_refptr<Sequence> sequence = delegate_->GetWork(); What's the plan for getting work from the single threaded priority queue? https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_thread_delegate.h (right): https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_thread_delegate.h:14: class SchedulerWorkerThread; Remove forward declaration.
robliao@: Done. https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_thread.cc:71: scoped_refptr<Sequence> sequence = delegate_->GetWork(); On 2016/04/06 20:50:18, robliao wrote: > What's the plan for getting work from the single threaded priority queue? 3 possible solutions: 1) Single-threaded PQ is owned by worker thread. Worker thread passes a pointer to its single-threaded PQ to GetWork(). 2) Single-threaded PQ is owned by worker thread. Worker thread passes |this| to GetWork. Delegate accesses the single-threaded PQ via an accessor. 3) Thread pool has a map WorkerThread* -> single-threaded PQ. Worker thread passes |this| to GetWork. Delegate (thread pool) finds the single-threaded PQ in its map. (1) is great because the single-threaded PQ lives with its WorkerThread (no need to make sure that they are created/destroyed at the same time). I don't like (2). (3) is great because worker thread doesn't have to know about PQs. I planned to land this CL as-is and add the arguments we need to GetWork() in the CL that implements the SINGLE_THREADED ExecutionMode. https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_thread_delegate.h (right): https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_thread_delegate.h:14: class SchedulerWorkerThread; On 2016/04/06 20:50:18, robliao wrote: > Remove forward declaration. Done.
lgtm + nits https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_thread.cc (right): https://codereview.chromium.org/1864333002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_thread.cc:71: scoped_refptr<Sequence> sequence = delegate_->GetWork(); On 2016/04/06 21:11:38, fdoray wrote: > On 2016/04/06 20:50:18, robliao wrote: > > What's the plan for getting work from the single threaded priority queue? > > 3 possible solutions: > 1) Single-threaded PQ is owned by worker thread. Worker thread passes a pointer > to its single-threaded PQ to GetWork(). > 2) Single-threaded PQ is owned by worker thread. Worker thread passes |this| to > GetWork. Delegate accesses the single-threaded PQ via an accessor. > 3) Thread pool has a map WorkerThread* -> single-threaded PQ. Worker thread > passes |this| to GetWork. Delegate (thread pool) finds the single-threaded PQ in > its map. > > (1) is great because the single-threaded PQ lives with its WorkerThread (no need > to make sure that they are created/destroyed at the same time). I don't like > (2). (3) is great because worker thread doesn't have to know about PQs. > > I planned to land this CL as-is and add the arguments we need to GetWork() in > the CL that implements the SINGLE_THREADED ExecutionMode. Sounds good. Just wanted to me sure this wasn't omitted since we did pass the this pointer through on the original CL. https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:37: // |delegate| is also notified when the thread's main function enters/exits The |delegate| specific behavior can be removed here and any important details should probably be on SchedulerWorkerThreadDelegate itself. https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_delegate.h (right): https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_delegate.h:18: // Called when the main function of the SchedulerWorkerThread is entered. Nit: 'SchedulerWorkerThread starts.' 'SchedulerWorkerThread enters' works too. https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:35: num_get_work_callback_cv_(lock_.CreateConditionVariable()), Nit: Remove callback? Review for remaining uses.
robliao@: Thanks. gab@: Can you review this CL? https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread.h:37: // |delegate| is also notified when the thread's main function enters/exits On 2016/04/06 21:28:20, robliao wrote: > The |delegate| specific behavior can be removed here and any important details > should probably be on SchedulerWorkerThreadDelegate itself. Done. https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_delegate.h (right): https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_delegate.h:18: // Called when the main function of the SchedulerWorkerThread is entered. On 2016/04/06 21:28:20, robliao wrote: > Nit: 'SchedulerWorkerThread starts.' > > 'SchedulerWorkerThread enters' works too. Done. https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1864333002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:35: num_get_work_callback_cv_(lock_.CreateConditionVariable()), On 2016/04/06 21:28:20, robliao wrote: > Nit: Remove callback? > Review for remaining uses. Done.
fdoray@chromium.org changed reviewers: + danakj@chromium.org
danakj@: Can you review this CL? I rebased the SchedulerThreadPool CL to use this delegate instead of multiple callbacks.
lgtm w/ move to SchedulerWorkerThread:: and nits https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_delegate.h (right): https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_delegate.h:16: class SchedulerWorkerThreadDelegate { I think it's better to define this on SchedulerWorkerThread, i.e. SchedulerWorkerThread::Delegate (this is a typical in Chrome) https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_delegate.h:17: public: I think you need to explicitly make the destructor virtual+default or you won't be able to override this class. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:54: EXPECT_EQ(1U, num_main_entry_); Move to test before Join? https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:61: while (num_main_entry_ < 1) Make |num_main_entry_| a bool (and thus probably a WaitableEvent)? Same for |num_main_exit_|. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:150: // |sequence| (a TaskTracker might have prevented the Task from running). No need to re-state comment from override. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:229: // Wake up |worker_thread_| and wait until it has run all the Tasks returned s/all the Tasks/the Task/ ?
gab@: Done. danakj@: Can you review this CL? Thanks. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_delegate.h (right): https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_delegate.h:16: class SchedulerWorkerThreadDelegate { On 2016/04/07 16:42:08, gab wrote: > I think it's better to define this on SchedulerWorkerThread, i.e. > SchedulerWorkerThread::Delegate (this is a typical in Chrome) Done. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_delegate.h:17: public: On 2016/04/07 16:42:08, gab wrote: > I think you need to explicitly make the destructor virtual+default or you won't > be able to override this class. Done. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:54: EXPECT_EQ(1U, num_main_entry_); On 2016/04/07 16:42:08, gab wrote: > Move to test before Join? I no longer need this in TearDown(). SetUp() verifies that OnMainEntry() is called at least once and OnMainEntry() verifies that it isn't called more than once. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:61: while (num_main_entry_ < 1) On 2016/04/07 16:42:08, gab wrote: > Make |num_main_entry_| a bool (and thus probably a WaitableEvent)? > > Same for |num_main_exit_|. Done. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:150: // |sequence| (a TaskTracker might have prevented the Task from running). On 2016/04/07 16:42:08, gab wrote: > No need to re-state comment from override. Done. https://codereview.chromium.org/1864333002/diff/40001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_thread_unittest.cc:229: // Wake up |worker_thread_| and wait until it has run all the Tasks returned On 2016/04/07 16:42:08, gab wrote: > s/all the Tasks/the Task/ ? Done.
LGTM! 1 question https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:53: EXPECT_TRUE(main_exit_called_); What's the point of OnMainExit if it can only happen from JoinForTesting?
https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:53: EXPECT_TRUE(main_exit_called_); On 2016/04/07 18:40:59, danakj wrote: > What's the point of OnMainExit if it can only happen from JoinForTesting? In production, the thread managed by a SchedulerWorkerThread can exit during shutdown (task_tracker_->shutdown_completed()). OnMainExit() will be used to clear the TLS set by OnMainEntry(). Note that the SchedulerWorkerThread instance is never destroyed in production (despite the fact that its thread might exit).
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/1864333002/#ps100001 (title: "Verify that OnMainEntry isn't called more than once.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864333002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864333002/100001
https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:53: EXPECT_TRUE(main_exit_called_); On 2016/04/07 18:46:58, fdoray wrote: > On 2016/04/07 18:40:59, danakj wrote: > > What's the point of OnMainExit if it can only happen from JoinForTesting? > > In production, the thread managed by a SchedulerWorkerThread can exit during > shutdown (task_tracker_->shutdown_completed()). OnMainExit() will be used to > clear the TLS set by OnMainEntry(). > > Note that the SchedulerWorkerThread instance is never destroyed in production > (despite the fact that its thread might exit). Ah ok thanks.
https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread_unittest.cc (right): https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread_unittest.cc:53: EXPECT_TRUE(main_exit_called_); On 2016/04/07 18:40:59, danakj wrote: > What's the point of OnMainExit if it can only happen from JoinForTesting? We thought we needed it for TLS destruction but turns out we don't (more details coming up on SchedulerThreadPool CL when I send my comments :-))
https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:51: // |sequence| (a TaskTracker might have prevented the Task from running). Add a comment that the Task still hasn't been popped from |sequence| -- this wasn't obvious to me and a surprise when reading the STP CL.
The CQ bit was unchecked by fdoray@chromium.org
https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... File base/task_scheduler/scheduler_worker_thread.h (right): https://codereview.chromium.org/1864333002/diff/100001/base/task_scheduler/sc... base/task_scheduler/scheduler_worker_thread.h:51: // |sequence| (a TaskTracker might have prevented the Task from running). On 2016/04/07 18:52:00, gab wrote: > Add a comment that the Task still hasn't been popped from |sequence| -- this > wasn't obvious to me and a surprise when reading the STP CL. Done.
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, danakj@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/1864333002/#ps120001 (title: "CR gab #18 (add "The Task is still in |sequence| when this is called.")")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864333002/120001
The CQ bit was unchecked by fdoray@chromium.org
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864333002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1864333002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1864333002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Delegate instead of callbacks for SchedulerWorkerThread. Instead of requiring multiple callbacks in the SchedulerWorkerThread's constructor, require a single delegate with these methods: - OnMainEntry - OnMainExit (no callback equivalent before this CL) - GetWork - RanTaskFromSequence In a previous CL, we decided that it was better to use callbacks than a delegate because OnMainEntry/GetWork were handled by SchedulerThreadPool while RanTaskFromSequence was handled by TaskScheduler. Now that we've added OnMainExit (handled by SchedulerThreadPool), we believe that it's better to make SchedulerThreadPool a delegate of SchedulerWorkerThread. SchedulerThreadPool will invoke a callback bound to TaskScheduler when its RanTaskFromSequence method is called. BUG=553459 ========== to ========== TaskScheduler: Delegate instead of callbacks for SchedulerWorkerThread. Instead of requiring multiple callbacks in the SchedulerWorkerThread's constructor, require a single delegate with these methods: - OnMainEntry - OnMainExit (no callback equivalent before this CL) - GetWork - RanTaskFromSequence In a previous CL, we decided that it was better to use callbacks than a delegate because OnMainEntry/GetWork were handled by SchedulerThreadPool while RanTaskFromSequence was handled by TaskScheduler. Now that we've added OnMainExit (handled by SchedulerThreadPool), we believe that it's better to make SchedulerThreadPool a delegate of SchedulerWorkerThread. SchedulerThreadPool will invoke a callback bound to TaskScheduler when its RanTaskFromSequence method is called. BUG=553459 Committed: https://crrev.com/80c05e2c9df0f5085bd90748e2239cc41aef24c5 Cr-Commit-Position: refs/heads/master@{#385888} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/80c05e2c9df0f5085bd90748e2239cc41aef24c5 Cr-Commit-Position: refs/heads/master@{#385888} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
