Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2064073003:
TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable (Closed)
|
Created:
4 years, 6 months ago by fdoray Modified:
4 years, 5 months ago CC:
chromium-reviews, gab+watch_chromium.org, robliao+watch_chromium.org, fdoray+watch_chromium.org, danakj Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTaskScheduler: Make the worker pools of TaskSchedulerImpl configurable
With this CL, TaskSchedulerImpl::Create takes as argument a vector
of worker pool creation arguments and a callback that indicates in
which worker pool a task with given traits should run.
In the Chrome browser process, we plan to have 4 worker pools:
- Background
- Background with file i/o
- Foreground
- Foreground with file i/o
The number of workers per pool will be derived from system
characteristics and from variation params obtained from
variations::GetVariationParams.
In tests and in renderer processes, we might create a different
set of worker pools (e.g. file i/o doesn't make sense in a
renderer).
BUG=553459
Committed: https://crrev.com/51da6a83632b8ba4a49c3d7765b810e9d739e214
Cr-Commit-Position: refs/heads/master@{#402583}
Patch Set 1 #
Total comments: 15
Patch Set 2 : CR robliao #3 (map argument first) #
Total comments: 8
Patch Set 3 : delegate #
Total comments: 5
Patch Set 4 : vector of struct to configure worker pools #Patch Set 5 : rebase #
Total comments: 22
Patch Set 6 : CR #Patch Set 7 : self-review #
Total comments: 4
Patch Set 8 : CR robliao #22 #Patch Set 9 : remove unused "using" #
Total comments: 2
Messages
Total messages: 40 (13 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
Can you review this CL? Thanks.
https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:67: static void InitializeDefaultTaskScheduler( Can TaskSchedulerImpl pick these up without the caller needing to supply them? https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:36: int default_min_threads; uint32_t might be more appropriate. https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:41: constexpr ThreadPoolSettings kBackgroundPoolSettings{"Background", 1, 1, 0}; This shouldn't trigger a static init, right? https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:52: const std::map<std::string, std::string>& map) { Consider having the map argument first. This would read more like a "this" pointer for the map.
PTAnL https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:67: static void InitializeDefaultTaskScheduler( On 2016/06/14 21:43:44, robliao wrote: > Can TaskSchedulerImpl pick these up without the caller needing to supply them? No because variations::GetVariationParams() is in components/variations/variations_associated_data.h I don't want to describe supported keys in the header file because the map shouldn't be filled manually (it should come directly from variations::GetVariationParams()). https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:36: int default_min_threads; On 2016/06/14 21:43:44, robliao wrote: > uint32_t might be more appropriate. Does this rule apply here? "You should not use the unsigned integer types such as uint32_t, unless there is a valid reason such as representing a bit pattern rather than a number, or you need defined overflow modulo 2^N. In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this." https://google.github.io/styleguide/cppguide.html#Integer_Types https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:41: constexpr ThreadPoolSettings kBackgroundPoolSettings{"Background", 1, 1, 0}; On 2016/06/14 21:43:44, robliao wrote: > This shouldn't trigger a static init, right? No (constexpr + POD) https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:52: const std::map<std::string, std::string>& map) { On 2016/06/14 21:43:44, robliao wrote: > Consider having the map argument first. This would read more like a "this" > pointer for the map. Done.
https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:67: static void InitializeDefaultTaskScheduler( On 2016/06/15 17:14:32, fdoray wrote: > On 2016/06/14 21:43:44, robliao wrote: > > Can TaskSchedulerImpl pick these up without the caller needing to supply them? > > No because variations::GetVariationParams() is in > components/variations/variations_associated_data.h > > I don't want to describe supported keys in the header file because the map > shouldn't be filled manually (it should come directly from > variations::GetVariationParams()). Looking a bit more broadly, it's conceivable that folks who consume the task scheduler may not have access to variation params. Maybe we should have our own strongly structured initialization parameters and leave the finch unwrapping to the higher architectural level? https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler_impl.cc:36: int default_min_threads; On 2016/06/15 17:14:32, fdoray wrote: > On 2016/06/14 21:43:44, robliao wrote: > > uint32_t might be more appropriate. > > Does this rule apply here? > "You should not use the unsigned integer types such as uint32_t, unless there is > a valid reason such as representing a bit pattern rather than a number, or you > need defined overflow modulo 2^N. In particular, do not use unsigned types to > say a number will never be negative. Instead, use assertions for this." > https://google.github.io/styleguide/cppguide.html#Integer_Types Hrm... interesting. I guess we'll stick with that then. We're pretty inconsistent with regards to this around Chromium.
PTAnL https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:67: static void InitializeDefaultTaskScheduler( On 2016/06/15 17:32:37, robliao wrote: > On 2016/06/15 17:14:32, fdoray wrote: > > On 2016/06/14 21:43:44, robliao wrote: > > > Can TaskSchedulerImpl pick these up without the caller needing to supply > them? > > > > No because variations::GetVariationParams() is in > > components/variations/variations_associated_data.h > > > > I don't want to describe supported keys in the header file because the map > > shouldn't be filled manually (it should come directly from > > variations::GetVariationParams()). > > Looking a bit more broadly, it's conceivable that folks who consume the task > scheduler may not have access to variation params. > > Maybe we should have our own strongly structured initialization parameters and > leave the finch unwrapping to the higher architectural level? What do you think of this? class SchedulerThreadPoolImpl { public: struct Config { std::string name; ThreadPriority thread_priority; int max_num_threads; }; static std::unique_ptr<SchedulerThreadPoolImpl> Create( const Config& config, ... ); }; class TaskSchedulerImpl { public: class Delegate { public: // Returns the number of thread pools to create. int GetNumThreadPools(); // Returns arguments used to create the pool at index // |pool_index|. Config GetThreadPoolConfig(int pool_index); // Returns the index of the pool in which a task with // |traits| should run. int GetThreadPoolForTraits(const TaskTraits& traits); }; static std::unique_ptr<TaskSchedulerImpl> Create( std::unique_ptr<Delegate> delegate, ... ); }; - The formula to compute the max number of threads isn't in TaskSchedulerImpl (less complexity). - The number of pools and the algorithm to choose in which pool a task runs is customizable.
lg https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:67: static void InitializeDefaultTaskScheduler( On 2016/06/15 17:32:37, robliao wrote: > On 2016/06/15 17:14:32, fdoray wrote: > > On 2016/06/14 21:43:44, robliao wrote: > > > Can TaskSchedulerImpl pick these up without the caller needing to supply > them? > > > > No because variations::GetVariationParams() is in > > components/variations/variations_associated_data.h > > > > I don't want to describe supported keys in the header file because the map > > shouldn't be filled manually (it should come directly from > > variations::GetVariationParams()). > > Looking a bit more broadly, it's conceivable that folks who consume the task > scheduler may not have access to variation params. > > Maybe we should have our own strongly structured initialization parameters and > leave the finch unwrapping to the higher architectural level? I think it's fine to match our variations to finch's (i.e. as-is done now). Perhaps we want to document what goes in the map though for users that don't have Finch to generate the map for them. https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:30: // Name of the pool. Used to name threads and to find variation params that s/name threads/name the pool's threads/ https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:31: // apply to this pool. s/to this pool/to it/ (since I'm adding "the pool" to the beginning of the sentence above..) https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:32: const char* name; const char[] (i.e. to get const char* const, but [] is cleaner for string literals) https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:38: int default_threads_percentage_num_processors; default_threads_per_processor ? https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:46: 8, 32, 200}; Minimums 4/8 above sound arbitrary :-), why do we care about a minimum? Isn't the scheduler inherently trying to keep thread count to a minimum unless contention occurs anyways? https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:63: DCHECK_GT(settings.default_min_threads, 0); Overkill given other DCHECK below which encompasses the default if it matters? https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:69: variation_params, pool_name + "MaxThreads", settings.default_max_threads); DCHECK_GE(min_threads, max_threads); https://codereview.chromium.org/2064073003/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:72: settings.default_threads_percentage_num_processors); Split these with new lines between different variables, my eyes are torn from too much text :-)
A question for gab@ https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:67: static void InitializeDefaultTaskScheduler( On 2016/06/15 19:56:03, gab wrote: > On 2016/06/15 17:32:37, robliao wrote: > > On 2016/06/15 17:14:32, fdoray wrote: > > > On 2016/06/14 21:43:44, robliao wrote: > > > > Can TaskSchedulerImpl pick these up without the caller needing to supply > > them? > > > > > > No because variations::GetVariationParams() is in > > > components/variations/variations_associated_data.h > > > > > > I don't want to describe supported keys in the header file because the map > > > shouldn't be filled manually (it should come directly from > > > variations::GetVariationParams()). > > > > Looking a bit more broadly, it's conceivable that folks who consume the task > > scheduler may not have access to variation params. > > > > Maybe we should have our own strongly structured initialization parameters and > > leave the finch unwrapping to the higher architectural level? > > I think it's fine to match our variations to finch's (i.e. as-is done now). > Perhaps we want to document what goes in the map though for users that don't > have Finch to generate the map for them. gab@: WDYT of my new idea? (2016/06/15 19:54:53 https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc...)
https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:67: static void InitializeDefaultTaskScheduler( On 2016/06/15 19:54:53, fdoray wrote: > On 2016/06/15 17:32:37, robliao wrote: > > On 2016/06/15 17:14:32, fdoray wrote: > > > On 2016/06/14 21:43:44, robliao wrote: > > > > Can TaskSchedulerImpl pick these up without the caller needing to supply > > them? > > > > > > No because variations::GetVariationParams() is in > > > components/variations/variations_associated_data.h > > > > > > I don't want to describe supported keys in the header file because the map > > > shouldn't be filled manually (it should come directly from > > > variations::GetVariationParams()). > > > > Looking a bit more broadly, it's conceivable that folks who consume the task > > scheduler may not have access to variation params. > > > > Maybe we should have our own strongly structured initialization parameters and > > leave the finch unwrapping to the higher architectural level? > > What do you think of this? > > class SchedulerThreadPoolImpl { > public: > struct Config { > std::string name; > ThreadPriority thread_priority; > int max_num_threads; > }; > > static std::unique_ptr<SchedulerThreadPoolImpl> Create( > const Config& config, > ... > ); > }; > > class TaskSchedulerImpl { > public: > class Delegate { > public: > // Returns the number of thread pools to create. > int GetNumThreadPools(); > > // Returns arguments used to create the pool at index > // |pool_index|. > Config GetThreadPoolConfig(int pool_index); > > // Returns the index of the pool in which a task with > // |traits| should run. > int GetThreadPoolForTraits(const TaskTraits& traits); > }; > > static std::unique_ptr<TaskSchedulerImpl> Create( > std::unique_ptr<Delegate> delegate, > ... > ); > }; > > - The formula to compute the max number of threads isn't in TaskSchedulerImpl > (less complexity). > - The number of pools and the algorithm to choose in which pool a task runs is > customizable. > As discussed in person, this SGTM as a customizable delegate will also be a requirement to support non-browser processes. I'd define Delegate on TaskScheduler:: and Config on TaskScheduler::Delegate::, not in the impls I think though.
one question https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc... base/task_scheduler/task_scheduler.h:67: static void InitializeDefaultTaskScheduler( On 2016/06/16 15:07:07, gab wrote: > On 2016/06/15 19:54:53, fdoray wrote: > > On 2016/06/15 17:32:37, robliao wrote: > > > On 2016/06/15 17:14:32, fdoray wrote: > > > > On 2016/06/14 21:43:44, robliao wrote: > > > > > Can TaskSchedulerImpl pick these up without the caller needing to supply > > > them? > > > > > > > > No because variations::GetVariationParams() is in > > > > components/variations/variations_associated_data.h > > > > > > > > I don't want to describe supported keys in the header file because the map > > > > shouldn't be filled manually (it should come directly from > > > > variations::GetVariationParams()). > > > > > > Looking a bit more broadly, it's conceivable that folks who consume the task > > > scheduler may not have access to variation params. > > > > > > Maybe we should have our own strongly structured initialization parameters > and > > > leave the finch unwrapping to the higher architectural level? > > > > What do you think of this? > > > > class SchedulerThreadPoolImpl { > > public: > > struct Config { > > std::string name; > > ThreadPriority thread_priority; > > int max_num_threads; > > }; > > > > static std::unique_ptr<SchedulerThreadPoolImpl> Create( > > const Config& config, > > ... > > ); > > }; > > > > class TaskSchedulerImpl { > > public: > > class Delegate { > > public: > > // Returns the number of thread pools to create. > > int GetNumThreadPools(); > > > > // Returns arguments used to create the pool at index > > // |pool_index|. > > Config GetThreadPoolConfig(int pool_index); > > > > // Returns the index of the pool in which a task with > > // |traits| should run. > > int GetThreadPoolForTraits(const TaskTraits& traits); > > }; > > > > static std::unique_ptr<TaskSchedulerImpl> Create( > > std::unique_ptr<Delegate> delegate, > > ... > > ); > > }; > > > > - The formula to compute the max number of threads isn't in TaskSchedulerImpl > > (less complexity). > > - The number of pools and the algorithm to choose in which pool a task runs is > > customizable. > > > > As discussed in person, this SGTM as a customizable delegate will also be a > requirement to support non-browser processes. > > I'd define Delegate on TaskScheduler:: and Config on TaskScheduler::Delegate::, > not in the impls I think though. Our design doc says that we'll have a TestTaskScheduler that "runs tasks in FIFO order on a single thread". If that's still true, Delegate shouldn't be on TaskScheduler because not all TaskScheduler impls suport a delegate. WDYT?
Description was changed from ========== TaskScheduler: Control max number of threads per pool via field trial. This CL allows TaskSchedulerImpl::Create to receive variation params returned by variations::GetVariationParams. These variation params are put in a formula to compute the maximum number of threads per pool. BUG=553459 ========== to ========== TaskScheduler: Get thread pool settings via a delegate. With this CL, TaskSchedulerImpl::Create takes as argument a delegate that provides the number of thread pools to create, arguments to create this thread pools and a mapping between TaskTraits and thread pools. The TaskSchedulerImpl instantiated in the Chrome browser process will receive a delegate implemented in chrome/. This delegate will be able to call variations::GetVariationParams (components/variations/*). BUG=553459 ==========
Description was changed from ========== TaskScheduler: Get thread pool settings via a delegate. With this CL, TaskSchedulerImpl::Create takes as argument a delegate that provides the number of thread pools to create, arguments to create this thread pools and a mapping between TaskTraits and thread pools. The TaskSchedulerImpl instantiated in the Chrome browser process will receive a delegate implemented in chrome/. This delegate will be able to call variations::GetVariationParams (components/variations/*). BUG=553459 ========== to ========== TaskScheduler: Get thread pool creation settings via a delegate. With this CL, TaskSchedulerImpl::Create takes as argument a delegate that provides the number of thread pools to create, arguments to create this thread pools and a mapping between TaskTraits and thread pools. The TaskSchedulerImpl instantiated in the Chrome browser process will receive a delegate implemented in chrome/. This delegate will be able to call variations::GetVariationParams (components/variations/*). BUG=553459 ==========
PTAnL https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (left): https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:64: static void InitializeDefaultTaskScheduler(); I'm not convinced that we want this method if TaskSchedulerImpl's constructor has arguments (do we want to copy arguments + comments here?)
https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (left): https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:64: static void InitializeDefaultTaskScheduler(); On 2016/06/16 19:33:18, fdoray wrote: > I'm not convinced that we want this method if TaskSchedulerImpl's constructor > has arguments (do we want to copy arguments + comments here?) The intention here is to make it easy for tests or other components to initialize the system with reasonable defaults. We would use some reasonable defaults here. https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:89: const size_t num_thread_pools = delegate_->GetNumThreadPools(); I'm not sure it's necessary to make the TaskScheduler this generic. Do we need the ability to reconfigure the threadpool hierarchy with finch? If the answer for reconfiguration is no, then it may be more straightforward to pursue a Factory pattern instead of a delegate pattern. I anticipate that we could have two concrete implementations of TaskSchedulerImpl instead of needing to do data directed configuration for both scenarios.
https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:89: const size_t num_thread_pools = delegate_->GetNumThreadPools(); On 2016/06/16 20:51:15, robliao wrote: > I'm not sure it's necessary to make the TaskScheduler this generic. > > Do we need the ability to reconfigure the threadpool hierarchy with finch? > > If the answer for reconfiguration is no, then it may be more straightforward to > pursue a Factory pattern instead of a delegate pattern. > > I anticipate that we could have two concrete implementations of > TaskSchedulerImpl instead of needing to do data directed configuration for both > scenarios. We don't plan to reconfigure the thread pool hierarchy with finch. However, having this capability makes it easier to use TaskSchedulerImpl outside the Chrome browser process (e.g. a file i/o thread pool doesn't make sense in a renderer). TaskSchedulerImpl needs variation params put cannot call variations::GetVariationParams directly (because it's in components/variations/*). That means that code that instantiates TaskSchedulerImpl is responsible for providing these variation params. Most strategies to do that involve that TaskSchedulerImpl exposes the fact that it manages 4 thread pools in its interface. With a delegate, TaskSchedulerImpl doesn't have to commit itself to managing 4 thread pools in its interface. A Factory that TaskSchedulerImpl would use to create its ThreadPools (e.g. "give me a background file i/o thread pool"?) That still involves TaskSchedulerImpl exposing the fact that it manages 4 thread pools to its users? "two concrete implementations" = test and prod?
PTAnL https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (left): https://codereview.chromium.org/2064073003/diff/40001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:64: static void InitializeDefaultTaskScheduler(); On 2016/06/16 20:51:14, robliao wrote: > On 2016/06/16 19:33:18, fdoray wrote: > > I'm not convinced that we want this method if TaskSchedulerImpl's constructor > > has arguments (do we want to copy arguments + comments here?) > > The intention here is to make it easy for tests or other components to > initialize the system with reasonable defaults. We would use some reasonable > defaults here. I think we should wait until we have a set of defaults that is used by a Chrome process (test or prod) before re-introducing this method. Having a method that isn't used anywhere doesn't seem useful. Chat discussion: Gabriel Charette re. InitializeDefaultTaskScheduler(), I think I agree with Francois. Each process will want its own thing anyways (the point of the delegate) and unittests will use a TestTaskScheduler which internally drives a TaskScheduler with a simple delegate asking for 1 pool with 1 thread mixing all traits
Description was changed from ========== TaskScheduler: Get thread pool creation settings via a delegate. With this CL, TaskSchedulerImpl::Create takes as argument a delegate that provides the number of thread pools to create, arguments to create this thread pools and a mapping between TaskTraits and thread pools. The TaskSchedulerImpl instantiated in the Chrome browser process will receive a delegate implemented in chrome/. This delegate will be able to call variations::GetVariationParams (components/variations/*). BUG=553459 ========== to ========== TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable With this CL, TaskSchedulerImpl::Create takes as argument a vector of worker pool creation arguments and a callback that indicates in which worker pool a task with given traits should run. BUG=553459 ==========
Description was changed from ========== TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable With this CL, TaskSchedulerImpl::Create takes as argument a vector of worker pool creation arguments and a callback that indicates in which worker pool a task with given traits should run. BUG=553459 ========== to ========== TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable With this CL, TaskSchedulerImpl::Create takes as argument a vector of worker pool creation arguments and a callback that indicates in which worker pool a task with given traits should run. In the Chrome browser process, we plan to have 4 worker pools: - Background - Background with file i/o - Foreground - Foreground with file i/o The number of workers per pool will be derived from system characteristics and from variation params obtained from variations::GetVariationParams. In tests and in renderer processes, we might create a different set of worker pools (e.g. file i/o doesn't make sense in a renderer). BUG=553459 ==========
Looking good. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:76: worker_pool_index_for_traits_callback) DCHECK(!worker_pool_index_for_traits_callback.is_null()) https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:52: // run. Also document that this should be coded in a future-proof way. New traits should gracefully map to a default pool. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:57: // do so (never returns null). |worker_pools| is a list of worker pools to Nit: "CHECKs on failure" is sufficient. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:58: // create. |worker_pool_index_for_traits_callback| returns the index in |worker_pools| describes the worker pools to create. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:140: return traits.priority() == TaskPriority::BACKGROUND ? 1U : 3U; Should we encourage that the return values of these be enums : size_t? https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:152: std::vector<TaskSchedulerImpl::WorkerPoolCreationArgs> Since the vector is set up at the get go, we can make the vector const. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:166: EXPECT_TRUE(scheduler_); This should probably be an ASSERT since the guarantee is we CHECK on failure.
https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:140: return traits.priority() == TaskPriority::BACKGROUND ? 1U : 3U; On 2016/06/23 22:37:03, robliao wrote: > Should we encourage that the return values of these be enums : size_t? I would like for implementations of this to look similar to how TaskSchedulerImpl::GetWorkerPoolForTraits() used to look, returning raw indices looks easy to mess up to me. I think callers should keep constants to the index of each pool (or maybe an enum : size_t, not sure yet) and then use names instead of raw indices here. This will be strong versus a refactor as the names would change whereas it's easy to forget one raw index.. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:153: worker_pool_creation_args{ Space before '{' or is no space the style for these in-place initialization list? https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:161: IORestriction::ALLOWED, 12U}, Reinforcing my comment above: I think impls should be written such that this list and the indices returned from GetThreadPoolIndexForTraits() have a strong connection (i.e. a shared constant or something). This test needs to be the first example of how this can be written (and if it can't then I don't think this API will do). https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:166: EXPECT_TRUE(scheduler_); On 2016/06/23 22:37:03, robliao wrote: > This should probably be an ASSERT since the guarantee is we CHECK on failure. Agreed, plus the rest of the test is meaningless if this is null.
PTAnL https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.cc:76: worker_pool_index_for_traits_callback) On 2016/06/23 22:37:03, robliao wrote: > DCHECK(!worker_pool_index_for_traits_callback.is_null()) Done. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:52: // run. On 2016/06/23 22:37:03, robliao wrote: > Also document that this should be coded in a future-proof way. New traits should > gracefully map to a default pool. Done. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:57: // do so (never returns null). |worker_pools| is a list of worker pools to On 2016/06/23 22:37:03, robliao wrote: > Nit: "CHECKs on failure" is sufficient. Done. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl.h:58: // create. |worker_pool_index_for_traits_callback| returns the index in On 2016/06/23 22:37:03, robliao wrote: > |worker_pools| describes the worker pools to create. Done. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:140: return traits.priority() == TaskPriority::BACKGROUND ? 1U : 3U; On 2016/06/27 18:48:23, gab wrote: > On 2016/06/23 22:37:03, robliao wrote: > > Should we encourage that the return values of these be enums : size_t? > > I would like for implementations of this to look similar to how > TaskSchedulerImpl::GetWorkerPoolForTraits() used to look, returning raw indices > looks easy to mess up to me. > > I think callers should keep constants to the index of each pool (or maybe an > enum : size_t, not sure yet) and then use names instead of raw indices here. > This will be strong versus a refactor as the names would change whereas it's > easy to forget one raw index.. I now use an enum. Not sure that an enum class that we always cast to size_t is better than a simple enum. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:152: std::vector<TaskSchedulerImpl::WorkerPoolCreationArgs> On 2016/06/23 22:37:03, robliao wrote: > Since the vector is set up at the get go, we can make the vector const. The vector can no longer be const. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:153: worker_pool_creation_args{ On 2016/06/27 18:48:23, gab wrote: > Space before '{' or is no space the style for these in-place initialization > list? git cl format removes the space when I add it... https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:161: IORestriction::ALLOWED, 12U}, On 2016/06/27 18:48:23, gab wrote: > Reinforcing my comment above: I think impls should be written such that this > list and the indices returned from GetThreadPoolIndexForTraits() have a strong > connection (i.e. a shared constant or something). > > This test needs to be the first example of how this can be written (and if it > can't then I don't think this API will do). Done. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:166: EXPECT_TRUE(scheduler_); On 2016/06/27 18:48:23, gab wrote: > On 2016/06/23 22:37:03, robliao wrote: > > This should probably be an ASSERT since the guarantee is we CHECK on failure. > > Agreed, plus the rest of the test is meaningless if this is null. Done.
lgtm + nits. https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:152: std::vector<TaskSchedulerImpl::WorkerPoolCreationArgs> On 2016/06/27 19:45:05, fdoray wrote: > On 2016/06/23 22:37:03, robliao wrote: > > Since the vector is set up at the get go, we can make the vector const. > > The vector can no longer be const. An alternative is to do a post examination of the vector after construction to keep the const. The size checking is fine too and avoids having to store the WorkerPoolCreationArgs as a local for comparison. Your choice. e.g. worker_pools[BACKGROUND_WORKER_POOL] == background_worker_pool_creation_args https://codereview.chromium.org/2064073003/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2064073003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:165: EXPECT_EQ(BACKGROUND_WORKER_POOL, worker_pools.size()); These should probably be ASSERTS. If an index mismatch occurs. the test shouldn't run. https://codereview.chromium.org/2064073003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:179: DCHECK_EQ(FOREGROUND_FILE_IO_WORKER_POOL, worker_pools.size()); Typo?
gab@: PTAnL https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2064073003/diff/80001/base/task_scheduler/tas... base/task_scheduler/task_scheduler_impl_unittest.cc:152: std::vector<TaskSchedulerImpl::WorkerPoolCreationArgs> On 2016/06/28 00:24:24, robliao wrote: > On 2016/06/27 19:45:05, fdoray wrote: > > On 2016/06/23 22:37:03, robliao wrote: > > > Since the vector is set up at the get go, we can make the vector const. > > > > The vector can no longer be const. > > An alternative is to do a post examination of the vector after construction to > keep the const. The size checking is fine too and avoids having to store the > WorkerPoolCreationArgs as a local for comparison. Your choice. > > e.g. > worker_pools[BACKGROUND_WORKER_POOL] == background_worker_pool_creation_args I prefer not to define operator== just for this. https://codereview.chromium.org/2064073003/diff/120001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/2064073003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:165: EXPECT_EQ(BACKGROUND_WORKER_POOL, worker_pools.size()); On 2016/06/28 00:24:24, robliao wrote: > These should probably be ASSERTS. If an index mismatch occurs. the test > shouldn't run. Done. https://codereview.chromium.org/2064073003/diff/120001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl_unittest.cc:179: DCHECK_EQ(FOREGROUND_FILE_IO_WORKER_POOL, worker_pools.size()); On 2016/06/28 00:24:24, robliao wrote: > Typo? Done.
lgtm
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/2064073003/#ps140001 (title: "CR robliao #22")
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
+cc: danakj@ FYI, this CL adds arguments to TaskSchedulerImpl::Create to configure the worker pools.
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/2064073003/#ps160001 (title: "remove unused "using"")
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 ========== TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable With this CL, TaskSchedulerImpl::Create takes as argument a vector of worker pool creation arguments and a callback that indicates in which worker pool a task with given traits should run. In the Chrome browser process, we plan to have 4 worker pools: - Background - Background with file i/o - Foreground - Foreground with file i/o The number of workers per pool will be derived from system characteristics and from variation params obtained from variations::GetVariationParams. In tests and in renderer processes, we might create a different set of worker pools (e.g. file i/o doesn't make sense in a renderer). BUG=553459 ========== to ========== TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable With this CL, TaskSchedulerImpl::Create takes as argument a vector of worker pool creation arguments and a callback that indicates in which worker pool a task with given traits should run. In the Chrome browser process, we plan to have 4 worker pools: - Background - Background with file i/o - Foreground - Foreground with file i/o The number of workers per pool will be derived from system characteristics and from variation params obtained from variations::GetVariationParams. In tests and in renderer processes, we might create a different set of worker pools (e.g. file i/o doesn't make sense in a renderer). BUG=553459 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable With this CL, TaskSchedulerImpl::Create takes as argument a vector of worker pool creation arguments and a callback that indicates in which worker pool a task with given traits should run. In the Chrome browser process, we plan to have 4 worker pools: - Background - Background with file i/o - Foreground - Foreground with file i/o The number of workers per pool will be derived from system characteristics and from variation params obtained from variations::GetVariationParams. In tests and in renderer processes, we might create a different set of worker pools (e.g. file i/o doesn't make sense in a renderer). BUG=553459 ========== to ========== TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable With this CL, TaskSchedulerImpl::Create takes as argument a vector of worker pool creation arguments and a callback that indicates in which worker pool a task with given traits should run. In the Chrome browser process, we plan to have 4 worker pools: - Background - Background with file i/o - Foreground - Foreground with file i/o The number of workers per pool will be derived from system characteristics and from variation params obtained from variations::GetVariationParams. In tests and in renderer processes, we might create a different set of worker pools (e.g. file i/o doesn't make sense in a renderer). BUG=553459 Committed: https://crrev.com/51da6a83632b8ba4a49c3d7765b810e9d739e214 Cr-Commit-Position: refs/heads/master@{#402583} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/51da6a83632b8ba4a49c3d7765b810e9d739e214 Cr-Commit-Position: refs/heads/master@{#402583}
Message was sent while issue was closed.
danakj@chromium.org changed reviewers: + danakj@chromium.org
Message was sent while issue was closed.
LGTM https://codereview.chromium.org/2064073003/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2064073003/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.h:85: TaskSchedulerImpl(const WorkerPoolIndexForTraitsCallback& explicit
Message was sent while issue was closed.
https://codereview.chromium.org/2064073003/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2064073003/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.h:85: TaskSchedulerImpl(const WorkerPoolIndexForTraitsCallback& On 2016/06/28 22:58:33, danakj wrote: > explicit Done here https://codereview.chromium.org/2124623002/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
