Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2525483006:
Use TaskScheduler instead of WorkerPool in keystone_glue.mm. (Closed)
DescriptionUse TaskScheduler instead of WorkerPool in keystone_glue.mm.
This CL replaces a base::WorkerPool::PostTask() call with a
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: 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
Committed: https://crrev.com/7c5994f99f82726f4bc6d26181f7b29f449c3416
Cr-Commit-Position: refs/heads/master@{#435404}
Patch Set 1 #Patch Set 2 : fix build error #Patch Set 3 : CONITNUE_ON_SHUTDOWN #
Total comments: 8
Messages
Total messages: 36 (23 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: 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...
Description was changed from ========== Replace WorkerPool with TaskScheduler in keystone_glue.mm. WorkerPool is being deprecated in favor of TaskScheduler. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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. With File IO: The task is allowed to perform synchronous IO operations. BUG=659191 ==========
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
lgtm (though OWNERS will have to ack that the traits are appropriate)
fdoray@chromium.org changed reviewers: + thomasvl@chromium.org
thomasvl@: PTAL In particular, please make sure that we used appropriate TaskTraits for this task.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
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 keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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. With File IO: The task is allowed to perform synchronous IO operations. BUG=659191 ========== to ========== Use TaskScheduler instead of WorkerPool in keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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. 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 ==========
ping TVL 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? This task seems to read from the disk: - https://cs.chromium.org/chromium/src/chrome/browser/mac/keystone_glue.mm?sq=p... - determineUpdateStatus - currentlyInstalledVersion - [NSDictionary dictionaryWithContentsOfFile:appInfoPlistPath]; Is no wait correct? Tell us if this tasks wait on things other than file IO.
fdoray@chromium.org changed reviewers: + mark@chromium.org - thomasvl@chromium.org
mark@: 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? This task seems to read from the disk: - https://cs.chromium.org/chromium/src/chrome/browser/mac/keystone_glue.mm?sq=p... - determineUpdateStatus - currentlyInstalledVersion - [NSDictionary dictionaryWithContentsOfFile:appInfoPlistPath]; Is no wait correct? Tell us if this task waits on things other than file IO.
LGTM. Notes follow. fdoray wrote: > mark@: 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. I don’t know if I have enough information to answer this definitively. Both of the tasks that use this are sort of in the same category: we’ve got a spinner running while we’re waiting for them to complete. The tasks here aren’t responsible for animating the spinner. We’ve basically designed this on the principle that the tasks will take as long as they need to take to complete. But on the other side, in both cases, we change the UI when the tasks complete, so the user is in fact waiting on something to happen. BACKGROUND does sound fine to me, but I don’t know enough about how the alternatives are used elsewhere in Chrome to say definitively. Maybe with my descriptions of how they behave, you can help verify that BACKGROUND is correct. > Is a CONTINUE_ON_SHUTDOWN shutdown behavior correct? It should, because it's how > the task was handled by base::WorkerPool before this CL. Discussed inline. > Is with file IO correct? This task seems to read from the disk: > - > https://cs.chromium.org/chromium/src/chrome/browser/mac/keystone_glue.mm?sq=p... > - determineUpdateStatus > - currentlyInstalledVersion > - [NSDictionary dictionaryWithContentsOfFile:appInfoPlistPath]; Both tasks do disk I/O. > Is no wait correct? Tell us if this task waits on things other than file IO. The only blocking in either task should just be disk I/O.
https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:79: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN), Revise change description to match this. https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:715: PerformBridge::PostPerform(self, @selector(determineUpdateStatus)); I think that this one could be SKIP_ON_SHUTDOWN… https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:1075: PerformBridge::PostPerform(self, selector, toolPath); …but this one should be CONTINUE_ON_SHUTDOWN, because it’s concerned with continuing and completing a task that’s in progress.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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. 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 keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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: 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 ==========
https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:79: base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN), On 2016/11/29 21:56:06, Mark Mentovai wrote: > Revise change description to match this. Done. https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:715: PerformBridge::PostPerform(self, @selector(determineUpdateStatus)); On 2016/11/29 21:56:06, Mark Mentovai wrote: > I think that this one could be SKIP_ON_SHUTDOWN… Are you sure? CONTINUE_ON_SHUTDOWN tasks are not scheduled during shutdown. Shutdown may complete and the process may be terminated while they are still running. SKIP_ON_SHUTDOWN tasks are not scheduled during shutdown. If a SKIP_ON_SHUTDOWN task was already running when shutdown started, shutdown won't complete and the process won't be terminated before it completes. BLOCK_SHUTDOWN tasks are guaranteed to run to completion if they are posted before shutdown completes. https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:1075: PerformBridge::PostPerform(self, selector, toolPath); On 2016/11/29 21:56:06, Mark Mentovai wrote: > …but this one should be CONTINUE_ON_SHUTDOWN, because it’s concerned with > continuing and completing a task that’s in progress. You want to make sure that this task runs to completion? Then it should be BLOCK_SHUTDOWN. Problem: The main thread stops running tasks before TaskScheduler shutdown starts. This line https://cs.chromium.org/chromium/src/chrome/browser/mac/keystone_glue.mm?l=1096 is a no-op if it runs during or after TaskScheduler shutdown. Therefore, TaskScheduler shutdown should not be blocked on this task.
LGTM https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... File chrome/browser/mac/keystone_glue.mm (right): https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:715: PerformBridge::PostPerform(self, @selector(determineUpdateStatus)); On 2016/11/30 19:25:21, fdoray wrote: > On 2016/11/29 21:56:06, Mark Mentovai wrote: > > I think that this one could be SKIP_ON_SHUTDOWN… > > Are you sure? > > CONTINUE_ON_SHUTDOWN tasks are not scheduled during shutdown. Shutdown may > complete and the process may be terminated while they are still running. OK, that’s confusing terminology to me, but it sounds like CONTINUE_ON_SHUTDOWN is right for this one. https://codereview.chromium.org/2525483006/diff/40001/chrome/browser/mac/keys... chrome/browser/mac/keystone_glue.mm:1075: PerformBridge::PostPerform(self, selector, toolPath); On 2016/11/30 19:25:21, fdoray wrote: > On 2016/11/29 21:56:06, Mark Mentovai wrote: > > …but this one should be CONTINUE_ON_SHUTDOWN, because it’s concerned with > > continuing and completing a task that’s in progress. > > You want to make sure that this task runs to completion? Then it should be > BLOCK_SHUTDOWN. Yes, that’s what I was saying. > Problem: The main thread stops running tasks before TaskScheduler shutdown > starts. This line > https://cs.chromium.org/chromium/src/chrome/browser/mac/keystone_glue.mm?l=1096 > is a no-op if it runs during or after TaskScheduler shutdown. Therefore, > TaskScheduler shutdown should not be blocked on this task. Oh well, then. OK.
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/2525483006/#ps40001 (title: "CONITNUE_ON_SHUTDOWN")
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": 1480535757286350,
"parent_rev": "84f568106f03dde72768eda88975a0403d53b57d", "commit_rev":
"4039253684e5e09e371d857038a96615c62258d1"}
Message was sent while issue was closed.
Description was changed from ========== Use TaskScheduler instead of WorkerPool in keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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: 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 keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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: 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 ==========
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 keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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: 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 keystone_glue.mm. This CL replaces a base::WorkerPool::PostTask() call with a 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: 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 Committed: https://crrev.com/7c5994f99f82726f4bc6d26181f7b29f449c3416 Cr-Commit-Position: refs/heads/master@{#435404} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/7c5994f99f82726f4bc6d26181f7b29f449c3416 Cr-Commit-Position: refs/heads/master@{#435404} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
