Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2501203002:
Move function to compute the max number of threads in a TaskScheduler pool to base/. (Closed)
DescriptionMove function to compute the max number of threads in a TaskScheduler pool to base/.
This CL moves the formula to compute the max number of threads in a
TaskScheduler pool from components/task_scheduler_util/ to base/.
This will allow the formula to be reused in content clients
(content/renderer, content/gpu and content/utility).
TBR=danakj@chromium.org
BUG=664996
Committed: https://crrev.com/70e1c014474f8094e3b84f60761e355576a0f719
Cr-Commit-Position: refs/heads/master@{#432543}
Patch Set 1 #Patch Set 2 : self-review #Patch Set 3 : add BASE_EXPORT #
Total comments: 8
Patch Set 4 : CR robliao #4 (remove formula from comment) #Patch Set 5 : CR robliao #6 (RecommendedMaxNumberOfThreadsInPool) #
Total comments: 2
Patch Set 6 : CR robliao #8 (move BASE_EXPORT) #Patch Set 7 : do no build on nacl #
Dependent Patchsets: Messages
Total messages: 36 (18 generated)
Description was changed from ========== Add a base/ method to compute the max number of threads in a pool. This CL adds to base/ a method that computes the max number of threads in a pool using this formula: clamp(ceil(Number of cores * |cores_multiplier|) + |offset|, |min|, |max|) This will be used from content/ to compute the maximum number of threads in the renderer/gpu/utility process' pools from arguments provided by Content(Renderer/Gpu/Utility)Client. BUG=664996 ========== to ========== Move method to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow this method to be reused in content/renderer, content/gpu and content/utility. BUG=664996 ==========
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... File base/task_scheduler/initialization_util.h (right): https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:13: // Maximum number of threads = This portion of the comment just restates the code. Remove this portion. https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:15: int BASE_EXPORT MaxNumberOfThreadsInPool(int min, This isn't really a hard max. Maybe RecommendedMaxNumberOfThreadsInPool? https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:18: int offset); Is clang-format okay with this alignment? This is now hard enforced in base and it didn't seem to like lined up arguments.
PTAnL https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... File base/task_scheduler/initialization_util.h (right): https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:13: // Maximum number of threads = On 2016/11/15 16:56:10, robliao wrote: > This portion of the comment just restates the code. Remove this portion. Done. https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:15: int BASE_EXPORT MaxNumberOfThreadsInPool(int min, On 2016/11/15 16:56:10, robliao wrote: > This isn't really a hard max. Maybe RecommendedMaxNumberOfThreadsInPool? ? It's a hard max and not a recommendation since a pool will never have more threads than what this returns. The returned value is used to fill the max_threads_ member of SchedulerWorkerPoolParams, not recommended_max_threads_. https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:18: int offset); On 2016/11/15 16:56:10, robliao wrote: > Is clang-format okay with this alignment? This is now hard enforced in base and > it didn't seem to like lined up arguments. This is formatted by git cl format.
https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... File base/task_scheduler/initialization_util.h (right): https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:15: int BASE_EXPORT MaxNumberOfThreadsInPool(int min, On 2016/11/15 17:41:39, fdoray wrote: > On 2016/11/15 16:56:10, robliao wrote: > > This isn't really a hard max. Maybe RecommendedMaxNumberOfThreadsInPool? > > ? It's a hard max and not a recommendation since a pool will never have more > threads than what this returns. The returned value is used to fill the > max_threads_ member of SchedulerWorkerPoolParams, not recommended_max_threads_. Indeed. This calculation is our opinion on how to select max_threads_ based off of these inputs. Another developer can come along and misinterpret that this is the absolute highest number that can be specified for the TaskScheduler when the opposite is true. The TaskScheduler will respect what max_threads you give it. We just happen to have a way of selecting a max that we think is best, but the initializer is free to use their own judgement.
PTAnL https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... File base/task_scheduler/initialization_util.h (right): https://codereview.chromium.org/2501203002/diff/40001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:15: int BASE_EXPORT MaxNumberOfThreadsInPool(int min, On 2016/11/15 17:57:38, robliao wrote: > On 2016/11/15 17:41:39, fdoray wrote: > > On 2016/11/15 16:56:10, robliao wrote: > > > This isn't really a hard max. Maybe RecommendedMaxNumberOfThreadsInPool? > > > > ? It's a hard max and not a recommendation since a pool will never have more > > threads than what this returns. The returned value is used to fill the > > max_threads_ member of SchedulerWorkerPoolParams, not > recommended_max_threads_. > > Indeed. This calculation is our opinion on how to select max_threads_ based off > of these inputs. > > Another developer can come along and misinterpret that this is the absolute > highest number that can be specified for the TaskScheduler when the opposite is > true. The TaskScheduler will respect what max_threads you give it. We just > happen to have a way of selecting a max that we think is best, but the > initializer is free to use their own judgement. Done.
lgtm + nit https://codereview.chromium.org/2501203002/diff/80001/base/task_scheduler/ini... File base/task_scheduler/initialization_util.h (right): https://codereview.chromium.org/2501203002/diff/80001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:14: int BASE_EXPORT RecommendedMaxNumberOfThreadsInPool(int min, Nit: BASE_EXPORT int RecommendedMaxNumberOfThreadsInPool
Description was changed from ========== Move method to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow this method to be reused in content/renderer, content/gpu and content/utility. BUG=664996 ========== to ========== Move method to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content/renderer, content/gpu and content/utility. BUG=664996 ==========
Description was changed from ========== Move method to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content/renderer, content/gpu and content/utility. BUG=664996 ========== to ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content/renderer, content/gpu and content/utility. BUG=664996 ==========
Description was changed from ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content/renderer, content/gpu and content/utility. BUG=664996 ========== to ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content/renderer, content/gpu and content/utility. TBR=danakj@chromium.org BUG=664996 ==========
fdoray@chromium.org changed reviewers: + danakj@chromium.org
TBR danakj for base/BUILD.gn https://codereview.chromium.org/2501203002/diff/80001/base/task_scheduler/ini... File base/task_scheduler/initialization_util.h (right): https://codereview.chromium.org/2501203002/diff/80001/base/task_scheduler/ini... base/task_scheduler/initialization_util.h:14: int BASE_EXPORT RecommendedMaxNumberOfThreadsInPool(int min, On 2016/11/15 18:20:10, robliao wrote: > Nit: BASE_EXPORT int RecommendedMaxNumberOfThreadsInPool Done.
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/2501203002/#ps100001 (title: "CR robliao #8 (move BASE_EXPORT)")
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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Add to CL description where this will be used. lgtm otherwise
Description was changed from ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content/renderer, content/gpu and content/utility. TBR=danakj@chromium.org BUG=664996 ========== to ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content clients (content/renderer, content/gpu and content/utility). TBR=danakj@chromium.org BUG=664996 ==========
On 2016/11/15 20:35:51, gab wrote: > Add to CL description where this will be used. > > lgtm otherwise Done
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/2501203002/#ps120001 (title: "do no build on nacl")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
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...
Message was sent while issue was closed.
Description was changed from ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content clients (content/renderer, content/gpu and content/utility). TBR=danakj@chromium.org BUG=664996 ========== to ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content clients (content/renderer, content/gpu and content/utility). TBR=danakj@chromium.org BUG=664996 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content clients (content/renderer, content/gpu and content/utility). TBR=danakj@chromium.org BUG=664996 ========== to ========== Move function to compute the max number of threads in a TaskScheduler pool to base/. This CL moves the formula to compute the max number of threads in a TaskScheduler pool from components/task_scheduler_util/ to base/. This will allow the formula to be reused in content clients (content/renderer, content/gpu and content/utility). TBR=danakj@chromium.org BUG=664996 Committed: https://crrev.com/70e1c014474f8094e3b84f60761e355576a0f719 Cr-Commit-Position: refs/heads/master@{#432543} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/70e1c014474f8094e3b84f60761e355576a0f719 Cr-Commit-Position: refs/heads/master@{#432543} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
