Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501033002:
Redirect SequencedWorkerPool to TaskScheduler in the service process. (Closed)
DescriptionRedirect SequencedWorkerPool to TaskScheduler in the service process.
With this CL, in the service process:
- TaskScheduler is initialized.
- Tasks posted to a SequencedWorkerPool are redirected to TaskScheduler.
These steps will be done separately:
- Redirect the service process' file thread to TaskScheduler.
- Migrate all file thread / SequencedWorkerPool post task call sites in
the service process to the base/task_scheduler/post_task.h API.
BUG=664996
Committed: https://crrev.com/b32d035990150135bd44f0ae90030bed469c9624
Cr-Commit-Position: refs/heads/master@{#433677}
Patch Set 1 #Patch Set 2 : self-review #
Total comments: 2
Patch Set 3 : fix build error #
Total comments: 4
Patch Set 4 : CR robliao #15 (initialize before SWP, use 30 second reclaim time) #
Total comments: 2
Patch Set 5 : fix test error rebase #Patch Set 6 : CR gab #16 (-> size_t instead of static_cast<size_t>) #Patch Set 7 : Use SimpleTaskScheduler #Patch Set 8 : rebase #Messages
Total messages: 32 (20 generated)
Description was changed from ========== Redirect SequencedWorkerPool to TaskScheduler in service process. With this CL, in the service process: - TaskScheduler is initialized. - Tasks posted to a SequencedWorkerPool are redirected to TaskScheduler. These steps will be done separately: - Redirect the service process' file thread to TaskScheduler. - Migrate all file thread / SequencedWorkerPool post task call sites in the service process to the base/task_scheduler/post_task.h API. BUG=664996 ========== to ========== Redirect SequencedWorkerPool to TaskScheduler in the service process. With this CL, in the service process: - TaskScheduler is initialized. - Tasks posted to a SequencedWorkerPool are redirected to TaskScheduler. These steps will be done separately: - Redirect the service process' file thread to TaskScheduler. - Migrate all file thread / SequencedWorkerPool post task call sites in the service process to the base/task_scheduler/post_task.h API. BUG=664996 ==========
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2501033002/diff/20001/chrome/service/service_... File chrome/service/service_process.cc (right): https://codereview.chromium.org/2501033002/diff/20001/chrome/service/service_... chrome/service/service_process.cc:172: {base::SchedulerWorkerPoolParams( Looks like this is being copy-constructed rather than being move constructed.
PTAnL https://codereview.chromium.org/2501033002/diff/20001/chrome/service/service_... File chrome/service/service_process.cc (right): https://codereview.chromium.org/2501033002/diff/20001/chrome/service/service_... chrome/service/service_process.cc:172: {base::SchedulerWorkerPoolParams( On 2016/11/14 22:54:33, robliao wrote: > Looks like this is being copy-constructed rather than being move constructed. Done.
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2501033002/diff/40001/chrome/service/service_... File chrome/service/service_process.cc (right): https://codereview.chromium.org/2501033002/diff/40001/chrome/service/service_... chrome/service/service_process.cc:164: blocking_pool_ = new base::SequencedWorkerPool( Should this happen after TaskScheduler Initialization? https://codereview.chromium.org/2501033002/diff/40001/chrome/service/service_... chrome/service/service_process.cc:170: base::TimeDelta::FromSeconds(15); Any harm in not using the standard 30 we currently use in other places?
lgtm % comment https://codereview.chromium.org/2501033002/diff/60001/chrome/service/service_... File chrome/service/service_process.cc (right): https://codereview.chromium.org/2501033002/diff/60001/chrome/service/service_... chrome/service/service_process.cc:175: worker_pool_params_vector, base::Bind([](const base::TaskTraits&) { [](const base::TaskTraits&) -> size_t {} (will avoid need to static_cast() below I think)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
fdoray@chromium.org changed reviewers: + scottbyer@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
scottbyer@: PTAL https://codereview.chromium.org/2501033002/diff/40001/chrome/service/service_... File chrome/service/service_process.cc (right): https://codereview.chromium.org/2501033002/diff/40001/chrome/service/service_... chrome/service/service_process.cc:164: blocking_pool_ = new base::SequencedWorkerPool( On 2016/11/15 18:43:57, robliao wrote: > Should this happen after TaskScheduler Initialization? Done. https://codereview.chromium.org/2501033002/diff/40001/chrome/service/service_... chrome/service/service_process.cc:170: base::TimeDelta::FromSeconds(15); On 2016/11/15 18:43:57, robliao wrote: > Any harm in not using the standard 30 we currently use in other places? Done. I thought that since the service process doesn't have to be as responsive as the browser, it could be a good idea to reduce its resource utilization by using a smaller reclaim time. But there was nothing scientific about that. https://codereview.chromium.org/2501033002/diff/60001/chrome/service/service_... File chrome/service/service_process.cc (right): https://codereview.chromium.org/2501033002/diff/60001/chrome/service/service_... chrome/service/service_process.cc:175: worker_pool_params_vector, base::Bind([](const base::TaskTraits&) { On 2016/11/15 20:46:02, gab wrote: > [](const base::TaskTraits&) -> size_t {} > > (will avoid need to static_cast() below I think) Done.
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...)
scottbyer@: 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, scottbyer@chromium.org Link to the patchset: https://codereview.chromium.org/2501033002/#ps140001 (title: "rebase")
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": 140001, "attempt_start_ts": 1479762403446670,
"parent_rev": "d67855227f17656f3ab57aa1a9d9228877389d2c", "commit_rev":
"88ed01cf201c3f6930eb1d4fd944ece5850836f6"}
Message was sent while issue was closed.
Description was changed from ========== Redirect SequencedWorkerPool to TaskScheduler in the service process. With this CL, in the service process: - TaskScheduler is initialized. - Tasks posted to a SequencedWorkerPool are redirected to TaskScheduler. These steps will be done separately: - Redirect the service process' file thread to TaskScheduler. - Migrate all file thread / SequencedWorkerPool post task call sites in the service process to the base/task_scheduler/post_task.h API. BUG=664996 ========== to ========== Redirect SequencedWorkerPool to TaskScheduler in the service process. With this CL, in the service process: - TaskScheduler is initialized. - Tasks posted to a SequencedWorkerPool are redirected to TaskScheduler. These steps will be done separately: - Redirect the service process' file thread to TaskScheduler. - Migrate all file thread / SequencedWorkerPool post task call sites in the service process to the base/task_scheduler/post_task.h API. BUG=664996 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Redirect SequencedWorkerPool to TaskScheduler in the service process. With this CL, in the service process: - TaskScheduler is initialized. - Tasks posted to a SequencedWorkerPool are redirected to TaskScheduler. These steps will be done separately: - Redirect the service process' file thread to TaskScheduler. - Migrate all file thread / SequencedWorkerPool post task call sites in the service process to the base/task_scheduler/post_task.h API. BUG=664996 ========== to ========== Redirect SequencedWorkerPool to TaskScheduler in the service process. With this CL, in the service process: - TaskScheduler is initialized. - Tasks posted to a SequencedWorkerPool are redirected to TaskScheduler. These steps will be done separately: - Redirect the service process' file thread to TaskScheduler. - Migrate all file thread / SequencedWorkerPool post task call sites in the service process to the base/task_scheduler/post_task.h API. BUG=664996 Committed: https://crrev.com/b32d035990150135bd44f0ae90030bed469c9624 Cr-Commit-Position: refs/heads/master@{#433677} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b32d035990150135bd44f0ae90030bed469c9624 Cr-Commit-Position: refs/heads/master@{#433677} |
