Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528893004:
Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. (Closed)
DescriptionUse TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc.
This CL replaces base::WorkerPool::PostTask() with
base::PostTaskWithTraits(). 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 waits on synchronous file IO operations.
With Wait:
The task waits 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
Committed: https://crrev.com/cb096ca51bc796ac7916628ddbc00d8338c8340f
Cr-Commit-Position: refs/heads/master@{#435998}
Patch Set 1 #Patch Set 2 : fix test #
Total comments: 2
Patch Set 3 : CONTINUE_ON_SHUTDOWN #Patch Set 4 : add withfileio() #Patch Set 5 : fix initialization order in test #Patch Set 6 : add WithWait(), remove explicit priority #
Messages
Total messages: 41 (28 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: linux_chromium_chromeos_rel_ng on master.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 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: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits() call. The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. Shutdown behavior: SKIP_ON_SHUTDOWN (default) Tasks posted with this mode that have not started executing at shutdown will never run. However, any task that has already begun executing when shutdown is invoked will be allowed to continue and will block shutdown until completion. No File IO (default): The task does not perform synchronous IO operations. It spends most of its time using the CPU. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. Shutdown behavior: SKIP_ON_SHUTDOWN (default) Tasks posted with this mode that have not started executing at shutdown will never run. However, any task that has already begun executing when shutdown is invoked will be allowed to continue and will block shutdown until completion. No File IO (default): The task does not perform synchronous IO operations. It spends most of its time using the CPU. BUG=659191 ==========
fdoray@chromium.org changed reviewers: + gab@chromium.org
PTAL
lgtm w/ comment + OWNERS approval of priority pick https://codereview.chromium.org/2528893004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc (right): https://codereview.chromium.org/2528893004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:341: base::test::ScopedTaskScheduler scoped_task_scheduler_; In the product TaskScheduler outlives browser threads so it will be a good habit to put it before as a member.
On 2016/11/28 16:36:43, gab wrote: > lgtm w/ comment + OWNERS approval of priority pick > > https://codereview.chromium.org/2528893004/diff/20001/chrome/browser/chromeos... > File > chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc > (right): > > https://codereview.chromium.org/2528893004/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:341: > base::test::ScopedTaskScheduler scoped_task_scheduler_; > In the product TaskScheduler outlives browser threads so it will be a good habit > to put it before as a member. Actually all WorkerPool threads had CONTINUE_ON_SHUTDOWN semantics before, should keep this behavior.
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.
https://codereview.chromium.org/2528893004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc (right): https://codereview.chromium.org/2528893004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:341: base::test::ScopedTaskScheduler scoped_task_scheduler_; On 2016/11/28 16:36:43, gab wrote: > In the product TaskScheduler outlives browser threads so it will be a good habit > to put it before as a member. Done.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. Shutdown behavior: SKIP_ON_SHUTDOWN (default) Tasks posted with this mode that have not started executing at shutdown will never run. However, any task that has already begun executing when shutdown is invoked will be allowed to continue and will block shutdown until completion. No File IO (default): The task does not perform synchronous IO operations. It spends most of its time using the CPU. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. 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 waits on synchronous file 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: + xiyuan@chromium.org
Please review this CL. You should verify that the traits mentioned in the CL description make sense for this task. Thanks! Why are you doing this? WorkerPool is being deprecated in favor of TaskScheduler. Read the design doc https://docs.google.com/document/d/1S2AAeoo1xa_vsLbDYBsDHCqhrkfiMgoIPlyRi6kxa... Is a BACKGROUND priority correct? See all priorities in base/task_scheduler/task_traits.h. Tell us if this task should be USER_VISIBLE or USER_BLOCKING. Is a CONTINUE_ON_SHUTDOWN shutdown behavior correct? It should, because it's how the task was handled by base::WorkerPool before this CL. Is with file IO correct? Interactions with the TPM are considered as file IO. Is no wait correct? Tell us if the task waits on things other than file IO.
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...
xiyuan@chromium.org changed reviewers: + tbarzic@chromium.org
Toni, could you help with the review? The CL looks fine to me but would like you also take a look. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'd consider adding WithWait (similar to https://codereview.chromium.org/2526283004/), but otherwise LGTM
Description was changed from ========== Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). The following traits are used: Priority: BACKGROUND User won't notice if this task takes an arbitrarily long time to complete. 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 waits on synchronous file 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 easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). 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 waits on synchronous file 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 ==========
On 2016/11/29 22:52:59, tbarzic wrote: > I'd consider adding WithWait (similar to > https://codereview.chromium.org/2526283004/), but otherwise LGTM Done.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). 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 waits on synchronous file 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 easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). 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 waits on synchronous file IO operations. With Wait: The task waits 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 ==========
xiyuan@: PTAL
lgtm
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/2528893004/#ps100001 (title: "add WithWait(), remove explicit priority")
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": 100001, "attempt_start_ts": 1480705971317560,
"parent_rev": "0e85e3893a8796e1530ff455deba24199794c5d4", "commit_rev":
"26c022d5fdbdc0e95366150266159f97d3195412"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). 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 waits on synchronous file IO operations. With Wait: The task waits 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 easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). 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 waits on synchronous file IO operations. With Wait: The task waits 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 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). 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 waits on synchronous file IO operations. With Wait: The task waits 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 easy_unlock_tpm_key_manager.cc. This CL replaces base::WorkerPool::PostTask() with base::PostTaskWithTraits(). 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 waits on synchronous file IO operations. With Wait: The task waits 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 Committed: https://crrev.com/cb096ca51bc796ac7916628ddbc00d8338c8340f Cr-Commit-Position: refs/heads/master@{#435998} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/cb096ca51bc796ac7916628ddbc00d8338c8340f Cr-Commit-Position: refs/heads/master@{#435998} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
