Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2525073002:
Initialize TaskScheduler in Mash. (Closed)
DescriptionInitialize TaskScheduler in Mash.
This CL initializes TaskScheduler in MashRunner::RunMain() and sets
a flag to redirect all SequencedWorkerPool tasks to it
That means that all tasks posted to the |blocking_pool_| in
service_manager::Context will be redirected to TaskScheduler. Call
sites will be migrated to the base/task_scheduler/post_task.h API
and the |blocking_pool_| will be removed in separate CLs.
BUG=664996
Committed: https://crrev.com/57fc6249459477222cebba16f5d4c84f5e5dcee7
Cr-Commit-Position: refs/heads/master@{#434360}
Patch Set 1 #Patch Set 2 : add TODO #
Total comments: 2
Patch Set 3 : clarified TODO #
Total comments: 2
Patch Set 4 : shared constant for max num threads #
Messages
Total messages: 29 (12 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.
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL TaskScheduler is shut down shortly before the SequencedWorkerPool that it replaces MashRunner::RunMain -> [added by this CL] creates a TaskScheduler -> creates a service_manager::BackgroundServiceManager -> calls service_manager::BackgroundServiceManager::Init, which creates a thread --> The thread creates a service_manager::Context which creates a SequencedWorkerPool. ... -> [added by this CL] shuts down TaskScheduler -> destroys service_manager::BackgroundServiceManager, causing the thread to be stopped --> The thread deletes its service_manager::Context, and hence shuts down the SequencedWorkerPool
On 2016/11/23 19:08:10, fdoray wrote: > PTAL > > TaskScheduler is shut down shortly before the SequencedWorkerPool that it > replaces > > MashRunner::RunMain > -> [added by this CL] creates a TaskScheduler > -> creates a service_manager::BackgroundServiceManager > -> calls service_manager::BackgroundServiceManager::Init, which creates a thread > --> The thread creates a service_manager::Context which creates a > SequencedWorkerPool. > ... > -> [added by this CL] shuts down TaskScheduler > -> destroys service_manager::BackgroundServiceManager, causing the thread to be > stopped > --> The thread deletes its service_manager::Context, and hence shuts down the > SequencedWorkerPool Do we want it to run it its child processes while we're here?
lgtm
On 2016/11/23 19:12:44, robliao wrote: > On 2016/11/23 19:08:10, fdoray wrote: > > PTAL > > > > TaskScheduler is shut down shortly before the SequencedWorkerPool that it > > replaces > > > > MashRunner::RunMain > > -> [added by this CL] creates a TaskScheduler > > -> creates a service_manager::BackgroundServiceManager > > -> calls service_manager::BackgroundServiceManager::Init, which creates a > thread > > --> The thread creates a service_manager::Context which creates a > > SequencedWorkerPool. > > ... > > -> [added by this CL] shuts down TaskScheduler > > -> destroys service_manager::BackgroundServiceManager, causing the thread to > be > > stopped > > --> The thread deletes its service_manager::Context, and hence shuts down the > > SequencedWorkerPool > > Do we want it to run it its child processes while we're here? Child processes dynamically load a library https://cs.chromium.org/chromium/src/services/service_manager/runner/host/chi... This library won't have access to a TaskScheduler initialized in the main executable. If we want a TaskScheduler in child processes, it should be initialized from service_manager::ServiceRunner (would be done in a separate CL).
On 2016/11/23 20:37:23, fdoray wrote: > On 2016/11/23 19:12:44, robliao wrote: > > On 2016/11/23 19:08:10, fdoray wrote: > > > PTAL > > > > > > TaskScheduler is shut down shortly before the SequencedWorkerPool that it > > > replaces > > > > > > MashRunner::RunMain > > > -> [added by this CL] creates a TaskScheduler > > > -> creates a service_manager::BackgroundServiceManager > > > -> calls service_manager::BackgroundServiceManager::Init, which creates a > > thread > > > --> The thread creates a service_manager::Context which creates a > > > SequencedWorkerPool. > > > ... > > > -> [added by this CL] shuts down TaskScheduler > > > -> destroys service_manager::BackgroundServiceManager, causing the thread to > > be > > > stopped > > > --> The thread deletes its service_manager::Context, and hence shuts down > the > > > SequencedWorkerPool > > > > Do we want it to run it its child processes while we're here? > > Child processes dynamically load a library > https://cs.chromium.org/chromium/src/services/service_manager/runner/host/chi... > This library won't have access to a TaskScheduler initialized in the main > executable. If we want a TaskScheduler in child processes, it should be > initialized from service_manager::ServiceRunner (would be done in a separate > CL). sgtm. Comment that for now in the child launch path. lgtm in the meantime.
fdoray@chromium.org changed reviewers: + sky@chromium.org
sky@: PTAL
lgtm + nit on the comment. https://codereview.chromium.org/2525073002/diff/20001/chrome/app/mash/mash_ru... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2525073002/diff/20001/chrome/app/mash/mash_ru... chrome/app/mash/mash_runner.cc:184: // service_manager::ServiceRunner. https://crbug.com/664996 Add why the scheduler shouldn't be initialized here (ServiceRunner may not have access to this address space).
sky@: PTAL https://codereview.chromium.org/2525073002/diff/20001/chrome/app/mash/mash_ru... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2525073002/diff/20001/chrome/app/mash/mash_ru... chrome/app/mash/mash_runner.cc:184: // service_manager::ServiceRunner. https://crbug.com/664996 On 2016/11/23 21:02:20, robliao wrote: > Add why the scheduler shouldn't be initialized here (ServiceRunner may not have > access to this address space). Done.
https://codereview.chromium.org/2525073002/diff/40001/chrome/app/mash/mash_ru... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2525073002/diff/40001/chrome/app/mash/mash_ru... chrome/app/mash/mash_runner.cc:132: constexpr int kNumThreads = 3; Where does the 3 come from? Is there a constant else where we should be using?
https://codereview.chromium.org/2525073002/diff/40001/chrome/app/mash/mash_ru... File chrome/app/mash/mash_runner.cc (right): https://codereview.chromium.org/2525073002/diff/40001/chrome/app/mash/mash_ru... chrome/app/mash/mash_runner.cc:132: constexpr int kNumThreads = 3; On 2016/11/23 21:47:35, sky wrote: > Where does the 3 come from? Is there a constant else where we should be using? From the SequencedWorkerPool that TaskScheduler replaces https://cs.chromium.org/chromium/src/services/service_manager/standalone/cont... I didn't put TaskScheduler initialization in service_manager::Context because it enables the post_task.h API for the whole process (would be weird to enable post_task.h for the whole process when a Context is initialized for the whole process - I prefer to put it here to make it clear that any code running in a Mash process may use post_task.h).
LGTM - a shared constant would be nice for this.
On 2016/11/23 23:02:59, sky wrote: > LGTM - a shared constant would be nice for this. Done
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, robliao@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2525073002/#ps60001 (title: "shared constant for max num threads")
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
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_...) linux_chromium_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
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": 60001, "attempt_start_ts": 1480003510230650,
"parent_rev": "4bb1a3c48cde6438649e84d96d74a5c6bdd57591", "commit_rev":
"d9a6fa1bcd15a4c11023ff086d46d7ad866eaaa1"}
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Initialize TaskScheduler in Mash. This CL initializes TaskScheduler in MashRunner::RunMain() and sets a flag to redirect all SequencedWorkerPool tasks to it That means that all tasks posted to the |blocking_pool_| in service_manager::Context will be redirected to TaskScheduler. Call sites will be migrated to the base/task_scheduler/post_task.h API and the |blocking_pool_| will be removed in separate CLs. BUG=664996 ========== to ========== Initialize TaskScheduler in Mash. This CL initializes TaskScheduler in MashRunner::RunMain() and sets a flag to redirect all SequencedWorkerPool tasks to it That means that all tasks posted to the |blocking_pool_| in service_manager::Context will be redirected to TaskScheduler. Call sites will be migrated to the base/task_scheduler/post_task.h API and the |blocking_pool_| will be removed in separate CLs. BUG=664996 Committed: https://crrev.com/57fc6249459477222cebba16f5d4c84f5e5dcee7 Cr-Commit-Position: refs/heads/master@{#434360} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/57fc6249459477222cebba16f5d4c84f5e5dcee7 Cr-Commit-Position: refs/heads/master@{#434360} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
