Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791423003:
Initialize TaskScheduler with InitParams in CreateAndSetSimpleTaskScheduler(). (Closed)
DescriptionInitialize TaskScheduler with InitParams in CreateAndSetSimpleTaskScheduler().
Initialization with a vector of SchedulerWorkerPoolParams is
deprecated.
BUG=690706
Review-Url: https://codereview.chromium.org/2791423003
Cr-Commit-Position: refs/heads/master@{#462616}
Committed: https://chromium.googlesource.com/chromium/src/+/5217ff6448e98b789300c01dc178bdac31f8a134
Patch Set 1 #
Total comments: 4
Patch Set 2 : CR #
Total comments: 2
Patch Set 3 : CR-robliao-10 #Messages
Total messages: 19 (9 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2791423003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2791423003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.cc:54: constexpr int kMaxNumForegroundBlockingThreadsUpperBound = 64; Why 32 and 64? Seems very high for a "simple" task scheduler.
https://codereview.chromium.org/2791423003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2791423003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.cc:54: constexpr int kMaxNumForegroundBlockingThreadsUpperBound = 64; On 2017/04/04 20:48:13, gab wrote: > Why 32 and 64? Seems very high for a "simple" task scheduler. My thinking was that someone instantiating a simple task scheduler could want to use all the cores (it's what Windows Thread Pool does). The 32/64 maximums could even be higher (I don't want them to be hit on most machines). What constants would you use?
https://codereview.chromium.org/2791423003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2791423003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.cc:54: constexpr int kMaxNumForegroundBlockingThreadsUpperBound = 64; On 2017/04/05 13:01:28, fdoray wrote: > On 2017/04/04 20:48:13, gab wrote: > > Why 32 and 64? Seems very high for a "simple" task scheduler. > > My thinking was that someone instantiating a simple task scheduler could want to > use all the cores (it's what Windows Thread Pool does). The 32/64 maximums could > even be higher (I don't want them to be hit on most machines). > > What constants would you use? An alternative way to go about this is to adjust using the multiplier. Maybe we want the Sum(All multipliers) <= 1 as that would mean we would have the num_of_cores or fewer threads. This would make the upperbounds a little less important, but still useful for those machines that have lots of cores.
https://codereview.chromium.org/2791423003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2791423003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.cc:55: constexpr double kMaxNumForegroundBlockingThreadsCoresMultiplier = 2; This isn't a MaxNum, it's a multiplier only, right? How about naming the constants: k(PoolName)(MaxUpperBound|MaxLowerBound|CoresMultiplier|Offset)? Also, PS: will we really ever care about |offset| param? It's never been anything but 0 so far...
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm + comment. Thanks! https://codereview.chromium.org/2791423003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2791423003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:45: const int num_cores = SysInfo::NumberOfProcessors(); Add a quick blurb on how we set these values: Intentions for the simple task scheduler Under full load... * Background threads never outnumber foreground threads. * The system is utilized maximally.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
The CQ bit was unchecked by fdoray@chromium.org
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 Link to the patchset: https://codereview.chromium.org/2791423003/#ps40001 (title: "CR-robliao-10")
https://codereview.chromium.org/2791423003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2791423003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:45: const int num_cores = SysInfo::NumberOfProcessors(); On 2017/04/06 18:09:37, robliao wrote: > Add a quick blurb on how we set these values: > Intentions for the simple task scheduler > Under full load... > * Background threads never outnumber foreground threads. > * The system is utilized maximally. Done.
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": 1491502942300610,
"parent_rev": "accd49670253459e941376bad06cf07e8a5c9eb4", "commit_rev":
"5217ff6448e98b789300c01dc178bdac31f8a134"}
Message was sent while issue was closed.
Description was changed from ========== Initialize TaskScheduler with InitParams in CreateAndSetSimpleTaskScheduler(). Initialization with a vector of SchedulerWorkerPoolParams is deprecated. BUG=690706 ========== to ========== Initialize TaskScheduler with InitParams in CreateAndSetSimpleTaskScheduler(). Initialization with a vector of SchedulerWorkerPoolParams is deprecated. BUG=690706 Review-Url: https://codereview.chromium.org/2791423003 Cr-Commit-Position: refs/heads/master@{#462616} Committed: https://chromium.googlesource.com/chromium/src/+/5217ff6448e98b789300c01dc178... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5217ff6448e98b789300c01dc178... |
