Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2547173002:
Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. (Closed)
DescriptionUse TaskScheduler instead of WorkerPool in debug_daemon_client.cc.
This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits() call. The following traits are used:
Priority: Inherited (default)
The priority is inherited from the calling context (i.e. TaskTraits
are initialized with the priority of the current task).
Shutdown behavior: CONTINUE_ON_SHUTDOWN
Tasks posted with this mode which have not started executing before
shutdown is initiated will never run. Tasks with this mode running at
shutdown will be ignored (the worker will not be joined).
May block:
Tasks may block. In particular, they may wait on synchronous file
I/O operations: read or write a file from disk, interact with a pipe
or a socket, rename or delete a file, enumerate files in a directory,
etc.
Without sync primitives (default):
Tasks are not allowed to wait on waitable events/condition variables
or to join threads/processes.
BUG=659191
Committed: https://crrev.com/40a9803a7f6df4e31b7a1b5dbae1c800036e3e14
Cr-Commit-Position: refs/heads/master@{#440132}
Patch Set 1 #
Total comments: 7
Patch Set 2 : remove task_runner_ #Patch Set 3 : may block #Messages
Total messages: 27 (18 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: This issue passed the CQ dry run.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). With file IO: The task is allowed to perform synchronous IO operations. No Wait (default): The task does not wait on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 wip ========== to ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). With file IO: The task is allowed to perform synchronous IO operations. No Wait (default): The task does not wait on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ==========
fdoray@chromium.org changed reviewers: + stevenjb@chromium.org
Please review this CL. In particular, make sure that the traits mentioned in the CL description make sense for this task. Thanks! Note: WorkerPool is being deprecated in favor of TaskScheduler. Read the design doc https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Note: On shutdown, WorkerPool does not wait for tasks to run. To get the same behavior in TaskScheduler, we used TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN.
hashimoto@chromium.org changed reviewers: + hashimoto@chromium.org
drive-by https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... File chromeos/dbus/debug_daemon_client.cc (right): https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... chromeos/dbus/debug_daemon_client.cc:58: base::TaskTraits().WithFileIO().WithShutdownBehavior( This task runner is used not to read a file, but to read a pipe which may wait for the debug daemon on the other end of the pipe. Which suits better for this use case, WithFileIO or WithWait? https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... chromeos/dbus/debug_daemon_client.cc:59: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)), PipeReader uses the task runner to provide it to net::FileStream. Is net::FileStream safe to use with a task runner whose shutdown behavior is CONTINUE_ON_SHUTDOWN? https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... chromeos/dbus/debug_daemon_client.cc:95: scoped_refptr<base::TaskRunner> task_runner_; Please remove task_runner_.
PTAnL https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... File chromeos/dbus/debug_daemon_client.cc (right): https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... chromeos/dbus/debug_daemon_client.cc:58: base::TaskTraits().WithFileIO().WithShutdownBehavior( On 2016/12/09 10:54:37, hashimoto wrote: > This task runner is used not to read a file, but to read a pipe which may wait > for the debug daemon on the other end of the pipe. > > Which suits better for this use case, WithFileIO or WithWait? The WithFileIO() trait is appropriate for tasks that wait on synchronous file I/O (file = file on disk, pipe, network socket...). I will improve the comment in task_traits.h in a separate CL. https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... chromeos/dbus/debug_daemon_client.cc:59: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)), On 2016/12/09 10:54:37, hashimoto wrote: > PipeReader uses the task runner to provide it to net::FileStream. > Is net::FileStream safe to use with a task runner whose shutdown behavior is > CONTINUE_ON_SHUTDOWN? WorkerPool tasks are handled the same way as TaskScheduler CONTINUE_ON_SHUTDOWN tasks on shutdown. Since the task ran in WorkerPool before this CL, it shouldn't be a problem to run it in TaskScheduler with CONTINUE_ON_SHUTDOWN. https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... chromeos/dbus/debug_daemon_client.cc:95: scoped_refptr<base::TaskRunner> task_runner_; On 2016/12/09 10:54:37, hashimoto wrote: > Please remove task_runner_. Done.
lgtm https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... File chromeos/dbus/debug_daemon_client.cc (right): https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... chromeos/dbus/debug_daemon_client.cc:58: base::TaskTraits().WithFileIO().WithShutdownBehavior( On 2016/12/09 16:49:14, fdoray wrote: > On 2016/12/09 10:54:37, hashimoto wrote: > > This task runner is used not to read a file, but to read a pipe which may wait > > for the debug daemon on the other end of the pipe. > > > > Which suits better for this use case, WithFileIO or WithWait? > > The WithFileIO() trait is appropriate for tasks that wait on synchronous file > I/O (file = file on disk, pipe, network socket...). I will improve the comment > in task_traits.h in a separate CL. Thanks, improved comments would be appreciated. Even if read() is the syscall used to interact with the file descriptor, pipe IO doesn't wait for the disk to complete IO, but waits for another process to feed data. So IMHO WithWait() suits slightly better in this case, but if WithFileIO is preferred and it's going to be clearly written in task_traits.h, I'll be OK with it.
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...
The CQ bit was unchecked by fdoray@chromium.org
On 2016/12/10 01:13:55, hashimoto wrote: > lgtm > > https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... > File chromeos/dbus/debug_daemon_client.cc (right): > > https://codereview.chromium.org/2547173002/diff/1/chromeos/dbus/debug_daemon_... > chromeos/dbus/debug_daemon_client.cc:58: > base::TaskTraits().WithFileIO().WithShutdownBehavior( > On 2016/12/09 16:49:14, fdoray wrote: > > On 2016/12/09 10:54:37, hashimoto wrote: > > > This task runner is used not to read a file, but to read a pipe which may > wait > > > for the debug daemon on the other end of the pipe. > > > > > > Which suits better for this use case, WithFileIO or WithWait? > > > > The WithFileIO() trait is appropriate for tasks that wait on synchronous file > > I/O (file = file on disk, pipe, network socket...). I will improve the comment > > in task_traits.h in a separate CL. > > Thanks, improved comments would be appreciated. > > Even if read() is the syscall used to interact with the file descriptor, pipe IO > doesn't wait for the disk to complete IO, but waits for another process to feed > data. > So IMHO WithWait() suits slightly better in this case, but if WithFileIO is > preferred and it's going to be clearly written in task_traits.h, I'll be OK with > it. We heard your comments and came up with simpler traits: https://crbug.com/675660 Updated the CL with .MayBlock().
Description was changed from ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). With file IO: The task is allowed to perform synchronous IO operations. No Wait (default): The task does not wait on things other than synchronous file IO operations (e.g. WaitableEvent, ConditionVariable, join a thread, join a process, blocking system call that doesn't involve interactions with the file system). BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). May block: Tasks may block. In particular, they may wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. Without sync primitives: Tasks are not allowed to wait on waitable events/condition variables or to join threads/processes. BUG=659191 ==========
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hashimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2547173002/#ps40001 (title: "may block")
Description was changed from ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). May block: Tasks may block. In particular, they may wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. Without sync primitives: Tasks are not allowed to wait on waitable events/condition variables or to join threads/processes. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). May block: Tasks may block. In particular, they may wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. Without sync primitives (default): Tasks are not allowed to wait on waitable events/condition variables or to join threads/processes. BUG=659191 ==========
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/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482337402745080,
"parent_rev": "49dcffa5c85a20f9c082e4527156006ff0efaec5", "commit_rev":
"9fa84b13c823fe85bce024e0c5b874bcfd4c3d15"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). May block: Tasks may block. In particular, they may wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. Without sync primitives (default): Tasks are not allowed to wait on waitable events/condition variables or to join threads/processes. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). May block: Tasks may block. In particular, they may wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. Without sync primitives (default): Tasks are not allowed to wait on waitable events/condition variables or to join threads/processes. BUG=659191 Review-Url: https://codereview.chromium.org/2547173002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). May block: Tasks may block. In particular, they may wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. Without sync primitives (default): Tasks are not allowed to wait on waitable events/condition variables or to join threads/processes. BUG=659191 Review-Url: https://codereview.chromium.org/2547173002 ========== to ========== Use TaskScheduler instead of WorkerPool in debug_daemon_client.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: Inherited (default) The priority is inherited from the calling context (i.e. TaskTraits are initialized with the priority of the current task). Shutdown behavior: CONTINUE_ON_SHUTDOWN Tasks posted with this mode which have not started executing before shutdown is initiated will never run. Tasks with this mode running at shutdown will be ignored (the worker will not be joined). May block: Tasks may block. In particular, they may wait on synchronous file I/O operations: read or write a file from disk, interact with a pipe or a socket, rename or delete a file, enumerate files in a directory, etc. Without sync primitives (default): Tasks are not allowed to wait on waitable events/condition variables or to join threads/processes. BUG=659191 Committed: https://crrev.com/40a9803a7f6df4e31b7a1b5dbae1c800036e3e14 Cr-Commit-Position: refs/heads/master@{#440132} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/40a9803a7f6df4e31b7a1b5dbae1c800036e3e14 Cr-Commit-Position: refs/heads/master@{#440132} |
