Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501763002:
Add Thread Standby Policy SchedulerWorkerPoolImpl (Closed)
DescriptionAdd Thread Standby Policy SchedulerWorkerPoolImpl
This change allows callers to specify if a thread should always be on
standby to receive tasks or not.
BUG=664996
TBR=danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc
Committed: https://crrev.com/acfa7150dd0f8a61a64ac8ae8e5d156407fb866d
Cr-Commit-Position: refs/heads/master@{#432919}
Patch Set 1 #
Total comments: 8
Patch Set 2 : CR Feedback #
Total comments: 6
Patch Set 3 : CR Feedback #Patch Set 4 : Rebase to edc7bea #
Dependent Patchsets: Messages
Total messages: 50 (32 generated)
robliao@chromium.org changed reviewers: + fdoray@chromium.org, gab@chromium.org
https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_pool_impl.cc:806: idle_workers_stack_.Remove(worker); Add a TODO to honor StandbyThreadPolicy::ONE here. Let's say that most of the time, there are 3 tasks running in parallel. With StandbyThreadPolicy::ONE, we want to have 4 threads alive at all time, one of which never gets work. If our policy is just to keep the thread on top of the idle stack alive, we might detach the idle thread if its reclaim timer expires when one of the busy threads is temporarily on top of the idle stack. Would it be better if our policy was to always keep alive one thread that didn't run tasks in the last |reclaim_time|? (i.e. a thread that was just pushed on the idle stack doesn't count as a standby thread) https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_pool_impl.h:109: // Returns the number of workers alive in this worker pool. // The return value may not be accurate if workers are woken up // or detached during the call. https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_pool_params.h (right): https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_pool_params.h:27: // When possible, keep one idle thread on standby, reclaimed as necessary. I think it would be clearer if we re-used the *alive* term from scheduler_worker.h/.cc. // When possible, keep one idle thread *alive*, reclaimed as necessary. https://codereview.chromium.org/2501763002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization_util.cc (right): https://codereview.chromium.org/2501763002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization_util.cc:77: values.standby_thread_policy = StandbyThreadPolicy::ONE; values.standby_thread_policy = (tokens.size() >= 6 && tokens[5] == "lazy") ? StandbyThreadPolicy::LAZY : StandbyThreadPolicy::ONE;
Comments applied. Thanks! https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_pool_impl.cc:806: idle_workers_stack_.Remove(worker); On 2016/11/15 15:27:02, fdoray wrote: > Add a TODO to honor StandbyThreadPolicy::ONE here. > > Let's say that most of the time, there are 3 tasks running in parallel. With > StandbyThreadPolicy::ONE, we want to have 4 threads alive at all time, one of > which never gets work. If our policy is just to keep the thread on top of the > idle stack alive, we might detach the idle thread if its reclaim timer expires > when one of the busy threads is temporarily on top of the idle stack. Would it > be better if our policy was to always keep alive one thread that didn't run > tasks in the last |reclaim_time|? (i.e. a thread that was just pushed on the > idle stack doesn't count as a standby thread) It's a good question. I've captured this part of the paragraph in the comment. Another complication to watch out for is we don't want the thread to start processing work while on the idle stack, so GetWork will need to check for that before sending work out. Added the todo to WakeUpWorker. https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_pool_impl.h (right): https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_pool_impl.h:109: // Returns the number of workers alive in this worker pool. On 2016/11/15 15:27:02, fdoray wrote: > // The return value may not be accurate if workers are woken up > // or detached during the call. Done. https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... File base/task_scheduler/scheduler_worker_pool_params.h (right): https://codereview.chromium.org/2501763002/diff/1/base/task_scheduler/schedul... base/task_scheduler/scheduler_worker_pool_params.h:27: // When possible, keep one idle thread on standby, reclaimed as necessary. On 2016/11/15 15:27:02, fdoray wrote: > I think it would be clearer if we re-used the *alive* term from > scheduler_worker.h/.cc. > > // When possible, keep one idle thread *alive*, reclaimed as necessary. Done. https://codereview.chromium.org/2501763002/diff/1/components/task_scheduler_u... File components/task_scheduler_util/initialization_util.cc (right): https://codereview.chromium.org/2501763002/diff/1/components/task_scheduler_u... components/task_scheduler_util/initialization_util.cc:77: values.standby_thread_policy = StandbyThreadPolicy::ONE; On 2016/11/15 15:27:02, fdoray wrote: > values.standby_thread_policy = > (tokens.size() >= 6 && tokens[5] == "lazy") > ? StandbyThreadPolicy::LAZY > : StandbyThreadPolicy::ONE; Done.
lgtm
The CQ bit was checked by robliao@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by robliao@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.
The CQ bit was checked by robliao@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 commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
lgtm % comments https://codereview.chromium.org/2501763002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2501763002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:460: num_alive_workers++; nit: pre-increment is prefered https://codereview.chromium.org/2501763002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:771: // hysteresis to the CanDetach check. Please file a bug for this. https://codereview.chromium.org/2501763002/diff/20001/components/task_schedul... File components/task_scheduler_util/initialization_util.cc (right): https://codereview.chromium.org/2501763002/diff/20001/components/task_schedul... components/task_scheduler_util/initialization_util.cc:51: // 5. Detach Time in Milliseconds (milliseconds) Document 6th token here (also the above list should probably be zero-based to avoid confusion with indices in code)
https://codereview.chromium.org/2501763002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_impl.cc (right): https://codereview.chromium.org/2501763002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:460: num_alive_workers++; On 2016/11/15 20:33:01, gab wrote: > nit: pre-increment is prefered Done. https://codereview.chromium.org/2501763002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_impl.cc:771: // hysteresis to the CanDetach check. On 2016/11/15 20:33:01, gab wrote: > Please file a bug for this. Done. https://codereview.chromium.org/2501763002/diff/20001/components/task_schedul... File components/task_scheduler_util/initialization_util.cc (right): https://codereview.chromium.org/2501763002/diff/20001/components/task_schedul... components/task_scheduler_util/initialization_util.cc:51: // 5. Detach Time in Milliseconds (milliseconds) On 2016/11/15 20:33:01, gab wrote: > Document 6th token here (also the above list should probably be zero-based to > avoid confusion with indices in code) Done.
The CQ bit was checked by robliao@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 ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=553459 ========== to ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=553459 TBR=danakj for sequenced_worker_pool_unittests.cc ==========
Description was changed from ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=553459 TBR=danakj for sequenced_worker_pool_unittests.cc ========== to ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=553459 TBR=danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc ==========
robliao@chromium.org changed reviewers: + danakj@chromium.org
TBR danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc
LGTM
Description was changed from ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=553459 TBR=danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc ========== to ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=664996 TBR=danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc ==========
The CQ bit was unchecked by robliao@chromium.org
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2501763002/#ps40001 (title: "CR Feedback")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by robliao@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 commit-bot@chromium.org
Failed to apply patch for components/task_scheduler_util/initialization_util.cc:
While running git apply --index -p1;
error: patch failed: components/task_scheduler_util/initialization_util.cc:70
error: components/task_scheduler_util/initialization_util.cc: patch does not
apply
Patch: components/task_scheduler_util/initialization_util.cc
Index: components/task_scheduler_util/initialization_util.cc
diff --git a/components/task_scheduler_util/initialization_util.cc
b/components/task_scheduler_util/initialization_util.cc
index
ca94f42171117f036be7702fe7e8116de4395bcf..22c2fa55e79e37294d7d6396ff98051e3858ac1f
100644
--- a/components/task_scheduler_util/initialization_util.cc
+++ b/components/task_scheduler_util/initialization_util.cc
@@ -22,6 +22,9 @@ namespace task_scheduler_util {
namespace {
+using StandbyThreadPolicy =
+ base::SchedulerWorkerPoolParams::StandbyThreadPolicy;
+
enum WorkerPoolType : size_t {
BACKGROUND_WORKER_POOL = 0,
BACKGROUND_FILE_IO_WORKER_POOL,
@@ -31,6 +34,7 @@ enum WorkerPoolType : size_t {
};
struct WorkerPoolVariationValues {
+ StandbyThreadPolicy standby_thread_policy;
int threads = 0;
base::TimeDelta detach_period;
};
@@ -40,11 +44,12 @@ struct WorkerPoolVariationValues {
//
// |pool_descriptor| is a semi-colon separated value string with the following
// items:
-// 1. Minimum Thread Count (int)
-// 2. Maximum Thread Count (int)
-// 3. Thread Count Multiplier (double)
-// 4. Thread Count Offset (int)
-// 5. Detach Time in Milliseconds (milliseconds)
+// 0. Minimum Thread Count (int)
+// 1. Maximum Thread Count (int)
+// 2. Thread Count Multiplier (double)
+// 3. Thread Count Offset (int)
+// 4. Detach Time in Milliseconds (milliseconds)
+// 5. Standby Thread Policy (string)
// Additional values may appear as necessary and will be ignored.
WorkerPoolVariationValues StringToWorkerPoolVariationValues(
const base::StringPiece pool_descriptor) {
@@ -70,6 +75,10 @@ WorkerPoolVariationValues StringToWorkerPoolVariationValues(
values.threads = std::min(maximum, std::max(minimum, threads));
values.detach_period =
base::TimeDelta::FromMilliseconds(detach_milliseconds);
+ values.standby_thread_policy =
+ (tokens.size() >= 6 && tokens[5] == "lazy")
+ ? StandbyThreadPolicy::LAZY
+ : StandbyThreadPolicy::ONE;
return values;
}
DLOG(ERROR) << "Invalid Worker Pool Descriptor: " << pool_descriptor;
@@ -131,6 +140,7 @@ bool InitializeDefaultTaskScheduler(
params_vector.emplace_back(predefined_params.name,
predefined_params.priority_hint,
predefined_params.io_restriction,
+ variation_values.standby_thread_policy,
variation_values.threads,
variation_values.detach_period);
}
The CQ bit was checked by robliao@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 robliao@chromium.org
The CQ bit was checked by robliao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, gab@chromium.org, fdoray@chromium.org Link to the patchset: https://codereview.chromium.org/2501763002/#ps60001 (title: "Rebase to edc7bea")
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 robliao@chromium.org
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=664996 TBR=danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc ========== to ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=664996 TBR=danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=664996 TBR=danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc ========== to ========== Add Thread Standby Policy SchedulerWorkerPoolImpl This change allows callers to specify if a thread should always be on standby to receive tasks or not. BUG=664996 TBR=danakj for signature change in base/threading/sequenced_worker_pool_unittest.cc Committed: https://crrev.com/acfa7150dd0f8a61a64ac8ae8e5d156407fb866d Cr-Commit-Position: refs/heads/master@{#432919} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/acfa7150dd0f8a61a64ac8ae8e5d156407fb866d Cr-Commit-Position: refs/heads/master@{#432919} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
