Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2427963002:
Support FileDescriptorWatcher in TaskScheduler. (Closed)
DescriptionSupport FileDescriptorWatcher in TaskScheduler.
TBR=danakj@chromium.org
BUG=645114
Committed: https://crrev.com/9c72b0388e0c92e1d7b20dc17aa003f98b560742
Cr-Commit-Position: refs/heads/master@{#428043}
Patch Set 1 #Patch Set 2 : build on posix #Patch Set 3 : self-review #Patch Set 4 : nacl #
Total comments: 2
Patch Set 5 : refactor #Patch Set 6 : self-review #Patch Set 7 : fix build error #
Total comments: 6
Patch Set 8 : CR robliao #23 #
Total comments: 10
Patch Set 9 : CR gab #29 #Patch Set 10 : fix build error #Patch Set 11 : fix build error #Patch Set 12 : fix build error (add BASE_EXPORT) #
Messages
Total messages: 57 (44 generated)
Description was changed from ========== ler# Enter a description of the change. Support FileDescriptorWatcher in TaskScheduler. BUG=553459, 645114 ========== to ========== Support FileDescriptorWatcher in TaskScheduler. BUG=553459, 645114 ==========
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-...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
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...
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2427963002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2427963002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:40: MessageLoopForIO* watch_file_descriptor_message_loop It feels a bit intrusive to add this directly to TaskTracker. To me TaskTracker is a no-op place used for instrumentation/metrics/tracking/etc, not logic. What do you think of adding TaskTracker::TaskObserver instead which can be registered to be called before/after a task runs (with its TaskTraits say)? This isn't scary like your typical observer IMO because you still need an instance of TaskTracker which means the task_scheduler code still controls who gets to be able to register observers. It also gets rid of all if-defs (which are also a sign to me that it doesn't belong here) but the single callsite that will register. And it probably makes tests cleaner as well by not coupling the need for a |watch_file_descriptor_message_loop_| in production with the TaskTracker logic (the need to adapt every test is another sign IMO that this is the wrong place).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAnL https://codereview.chromium.org/2427963002/diff/60001/base/task_scheduler/tas... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2427963002/diff/60001/base/task_scheduler/tas... base/task_scheduler/task_tracker.h:40: MessageLoopForIO* watch_file_descriptor_message_loop On 2016/10/18 14:40:43, gab wrote: > It feels a bit intrusive to add this directly to TaskTracker. To me TaskTracker > is a no-op place used for instrumentation/metrics/tracking/etc, not logic. We already register TaskRunner handles from TaskTracker::RunTask. I think it's a good thing to put all the work to set up the environment to run a task in TaskTracker::RunTask because that method could easily be reused from a SchedulerWorkerPool impl backed by the Windows thread pool. > > What do you think of adding TaskTracker::TaskObserver instead which can be > registered to be called before/after a task runs (with its TaskTraits say)? This > isn't scary like your typical observer IMO because you still need an instance of > TaskTracker which means the task_scheduler code still controls who gets to be > able to register observers. The environment set up code can still be reused if it's moved to a TaskObserver invoked by TaskTracker::RunTask. However, we loose the ability to instantiate objects on the stack for the scope of the task. i.e. FileDescriptorWatcher file_descriptor_watcher(message_loop); becomes void BeforeRunTask() { // Heap allocation :( file_descriptor_watcher_ = MakeUnique<FileDescriptorWatcher>(); } void AfterRunTask() { // Extra line that we could forget to add :( file_descriptor_watcher_.reset(); } Instead of having a TaskObserver, WDYT of a TaskTracker::Delegate: class TaskTracker::Delegate { public: // Must run the callback in |task|. May perform additional // work before and after running the callback. void RunTask(std::unique_ptr<Task> task) = 0; }; Keeping track of the number of tasks blocking shutdown is handled by TaskTracker. The delegate handles setting up the environment to run the task and tracing [1]. > > It also gets rid of all if-defs (which are also a sign to me that it doesn't > belong here) but the single callsite that will register. > > And it probably makes tests cleaner as well by not coupling the need for a > |watch_file_descriptor_message_loop_| in production with the TaskTracker logic > (the need to adapt every test is another sign IMO that this is the wrong place). I agree. [1] Since tracing and running the task is done by TaskAnnotator::RunTask, it's would be hard to keep tracing in TaskTracker.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL
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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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.
Looks reasonable to me. Some questions and comments. https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:20: #if defined(OS_POSIX) && !defined(OS_NACL_SFI) Worth commenting that we don't support NaCl. Also, is defined(OS_NACL) sufficient? https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_posix.h (right): https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_tracker_posix.h:22: class BASE_EXPORT TaskTrackerPosix : public TaskTracker { Is a BASE_EXPORT currently required for TaskTrackerPosix? https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_tracker_posix.h:27: Nit: No line break needed here.
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.
PTAnL https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:20: #if defined(OS_POSIX) && !defined(OS_NACL_SFI) On 2016/10/20 00:59:40, robliao wrote: > Worth commenting that we don't support NaCl. Added comment in task_tracker_posix.h and in TaskSchedulerImpl::Initialize below. > Also, is defined(OS_NACL) sufficient? We do support OS_NACL_NON_SFI. https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_posix.h (right): https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_tracker_posix.h:22: class BASE_EXPORT TaskTrackerPosix : public TaskTracker { On 2016/10/20 00:59:40, robliao wrote: > Is a BASE_EXPORT currently required for TaskTrackerPosix? Done. https://codereview.chromium.org/2427963002/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_tracker_posix.h:27: On 2016/10/20 00:59:40, robliao wrote: > Nit: No line break needed here. Done.
lvg https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:96: // Start the service thread. On POSIX (except NaCL SFI), the service thread Explain why not NaCL SFI? https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:109: // thread. The service thread MessageLoopForIO and TaskRunner aren't available s/thread/thread's/ also, why mention "TaskRunner" if it's not used below? https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:284: debug::TaskAnnotator().RunTask(kQueueFunctionName, task.get()); Actually this makes me realize that I'm not sure it is/was correct to instantiates a new TaskAnnotator every time.. Not a blocker for this CL because this was already the case but worth looking into. https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:79: // running the task. An override doesn't "have to" perform work IMO, how about: // Runs |task|. An override is expected to call its parent's implementation but is free to perform extra work before and after doing so. https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_posix.cc (right): https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_tracker_posix.cc:26: TaskTracker::PerformRunTask(std::move(task)); Can we add task_tracker_posix_unittest.cc
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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...)
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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Description was changed from ========== Support FileDescriptorWatcher in TaskScheduler. BUG=553459, 645114 ========== to ========== Support FileDescriptorWatcher in TaskScheduler. BUG=645114 ==========
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/2427963002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:96: // Start the service thread. On POSIX (except NaCL SFI), the service thread On 2016/10/21 16:10:48, gab wrote: > Explain why not NaCL SFI? Because NaCL SFI doesn't support Libevent or CFRunLoop (Apple) and FileDescriptorWatcher needs one of those. https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:109: // thread. The service thread MessageLoopForIO and TaskRunner aren't available On 2016/10/21 16:10:48, gab wrote: > s/thread/thread's/ > > also, why mention "TaskRunner" if it's not used below? Done. task_runner() is used to instantiate DelayedTaskManager. https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.cc (right): https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_tracker.cc:284: debug::TaskAnnotator().RunTask(kQueueFunctionName, task.get()); On 2016/10/21 16:10:48, gab wrote: > Actually this makes me realize that I'm not sure it is/was correct to > instantiates a new TaskAnnotator every time.. > > Not a blocker for this CL because this was already the case but worth looking > into. Yes, it's wrong to initialize a TaskAnnotator every time. I plan to fix this in a separate CL. https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_tracker.h (right): https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_tracker.h:79: // running the task. On 2016/10/21 16:10:48, gab wrote: > An override doesn't "have to" perform work IMO, how about: > > // Runs |task|. An override is expected to call its parent's implementation but > is free to perform extra work before and after doing so. Done. https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... File base/task_scheduler/task_tracker_posix.cc (right): https://codereview.chromium.org/2427963002/diff/140001/base/task_scheduler/ta... base/task_scheduler/task_tracker_posix.cc:26: TaskTracker::PerformRunTask(std::move(task)); On 2016/10/21 16:10:48, gab wrote: > Can we add task_tracker_posix_unittest.cc Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
Description was changed from ========== Support FileDescriptorWatcher in TaskScheduler. BUG=645114 ========== to ========== Support FileDescriptorWatcher in TaskScheduler. TBR=danakj@chromium.org BUG=645114 ==========
fdoray@chromium.org changed reviewers: + danakj@chromium.org
TBR danakj for changes in base/BUILD.gn
The CQ bit was checked by fdoray@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Support FileDescriptorWatcher in TaskScheduler. TBR=danakj@chromium.org BUG=645114 ========== to ========== Support FileDescriptorWatcher in TaskScheduler. TBR=danakj@chromium.org BUG=645114 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Support FileDescriptorWatcher in TaskScheduler. TBR=danakj@chromium.org BUG=645114 ========== to ========== Support FileDescriptorWatcher in TaskScheduler. TBR=danakj@chromium.org BUG=645114 Committed: https://crrev.com/9c72b0388e0c92e1d7b20dc17aa003f98b560742 Cr-Commit-Position: refs/heads/master@{#428043} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9c72b0388e0c92e1d7b20dc17aa003f98b560742 Cr-Commit-Position: refs/heads/master@{#428043} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
