|
|
DescriptionAdd TaskScheduler::InitParams and accept it in TaskSchedulerImpl constructor.
TaskScheduler::InitParams is a struct of 4 SchedulerWorkerPoolParams.
Eventually, TaskSchedulerImpl will always be initialized with it
(it will no longer be possible to provide a vector with an
arbitrary number of SchedulerWorkerPoolParams).
Reference CL: https://codereview.chromium.org/2749303002/
BUG=690706
Review-Url: https://codereview.chromium.org/2764603002
Cr-Commit-Position: refs/heads/master@{#460167}
Committed: https://chromium.googlesource.com/chromium/src/+/3f6f57a4713fc45b7e4fddf588df5aa43c909457
Patch Set 1 #Patch Set 2 : self-review #
Total comments: 16
Patch Set 3 : CR gab #Patch Set 4 : add ref to bug #Patch Set 5 : fix build error #Patch Set 6 : kEnvironmentParams #Patch Set 7 : self-review #Patch Set 8 : self-review #Patch Set 9 : self-review #
Total comments: 10
Patch Set 10 : CR robliao #22 #Patch Set 11 : self-review #
Total comments: 2
Patch Set 12 : add ~InitParams #Patch Set 13 : no offsetof #
Messages
Total messages: 43 (20 generated)
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL Note: A big part of this CL is just transition code.
lgtm w/ comments (link to https://codereview.chromium.org/2749303002/ as the meta-CL for reference?) https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_params.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_params.cc:30: : SchedulerWorkerPoolParams(EmptyString(), s/EmptyString()/std::string()/ (first time I see base::EmptyString() and its meta comment leads me to believe it's not meant for this either) https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:24: SchedulerWorkerPoolParams GetWorkerPoolParamsWithPredefinedNameAndPriority( It's not really "predefined" as it's being defined by this method... how about AugmentWorkerPoolParamsWithNameAndPriority()? (with a TODO tagged to same bug to remove it as documentation for anyone wondering) https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:152: // Note: The names and priority hints in |init_params| are ignored. s/are ignored/are ignored (ref. TODO to remove them)/
https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/sch... File base/task_scheduler/scheduler_worker_pool_params.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/sch... base/task_scheduler/scheduler_worker_pool_params.cc:30: : SchedulerWorkerPoolParams(EmptyString(), On 2017/03/20 20:31:56, gab wrote: > s/EmptyString()/std::string()/ > > (first time I see base::EmptyString() and its meta comment leads me to believe > it's not meant for this either) Done. https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:24: SchedulerWorkerPoolParams GetWorkerPoolParamsWithPredefinedNameAndPriority( On 2017/03/20 20:31:56, gab wrote: > It's not really "predefined" as it's being defined by this method... how about > AugmentWorkerPoolParamsWithNameAndPriority()? (with a TODO tagged to same bug to > remove it as documentation for anyone wondering) Done. https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:152: // Note: The names and priority hints in |init_params| are ignored. On 2017/03/20 20:31:56, gab wrote: > s/are ignored/are ignored (ref. TODO to remove them)/ Done.
Description was changed from ========== Add TaskScheduler::InitParams. TaskScheduler::InitParams is a struct of 4 SchedulerWorkerPoolParams. Eventually, TaskSchedulerImpl will always be initialized with it (it will no longer be possible to provide a vector with an arbitrary number of SchedulerWorkerPoolParams). BUG=690706 ========== to ========== Add TaskScheduler::InitParams. TaskScheduler::InitParams is a struct of 4 SchedulerWorkerPoolParams. Eventually, TaskSchedulerImpl will always be initialized with it (it will no longer be possible to provide a vector with an arbitrary number of SchedulerWorkerPoolParams). Reference CL: https://codereview.chromium.org/2749303002/ BUG=690706 ==========
https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, The end state version won't be using SchedulerWorkerPoolParams as direct parameters to the SchedulerWorkerPoolImpls, right? I'd be okay with starting that work here. https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:140: // Deprecated. Use the overload below instead. Add bug number for removal.
PTAnL https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, On 2017/03/20 20:47:32, robliao wrote: > The end state version won't be using SchedulerWorkerPoolParams as direct > parameters to the SchedulerWorkerPoolImpls, right? > > I'd be okay with starting that work here. Yes it will https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc.... SchedulerWorkerPoolParams is exposed outside of TaskScheduler, so it can't contain params that can't be customized. https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.h:140: // Deprecated. Use the overload below instead. On 2017/03/20 20:47:32, robliao wrote: > Add bug number for removal. Done.
https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, On 2017/03/20 20:55:35, fdoray wrote: > On 2017/03/20 20:47:32, robliao wrote: > > The end state version won't be using SchedulerWorkerPoolParams as direct > > parameters to the SchedulerWorkerPoolImpls, right? > > > > I'd be okay with starting that work here. > > Yes it will > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc.... > > SchedulerWorkerPoolParams is exposed outside of TaskScheduler, so it can't > contain params that can't be customized. So once we remove those params, what will this file use?
On 2017/03/20 21:00:56, robliao wrote: > https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... > File base/task_scheduler/task_scheduler.cc (right): > > https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... > base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, > On 2017/03/20 20:55:35, fdoray wrote: > > On 2017/03/20 20:47:32, robliao wrote: > > > The end state version won't be using SchedulerWorkerPoolParams as direct > > > parameters to the SchedulerWorkerPoolImpls, right? > > > > > > I'd be okay with starting that work here. > > > > Yes it will > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc.... > > > > SchedulerWorkerPoolParams is exposed outside of TaskScheduler, so it can't > > contain params that can't be customized. > > So once we remove those params, what will this file use? The constructor of SWP will have name, priority_hint and scheduler_worker_pool_params arguments. TaskSchedulerImpl will instantiate SWPs with predefined names and priority_hints and externally provided scheduler_worker_pool_params. This file will not have knowledge of the predefined names and priority_hints. See reference CL.
https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, On 2017/03/20 21:00:56, robliao wrote: > On 2017/03/20 20:55:35, fdoray wrote: > > On 2017/03/20 20:47:32, robliao wrote: > > > The end state version won't be using SchedulerWorkerPoolParams as direct > > > parameters to the SchedulerWorkerPoolImpls, right? > > > > > > I'd be okay with starting that work here. > > > > Yes it will > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc.... > > > > SchedulerWorkerPoolParams is exposed outside of TaskScheduler, so it can't > > contain params that can't be customized. > > So once we remove those params, what will this file use? Inlining here so we don't lose track of the thread: fdoray: The constructor of SWP will have name, priority_hint and scheduler_worker_pool_params arguments. TaskSchedulerImpl will instantiate SWPs with predefined names and priority_hints and externally provided scheduler_worker_pool_params. This file will not have knowledge of the predefined names and priority_hints. See reference CL.
https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, On 2017/03/20 21:55:00, robliao wrote: > On 2017/03/20 21:00:56, robliao wrote: > > On 2017/03/20 20:55:35, fdoray wrote: > > > On 2017/03/20 20:47:32, robliao wrote: > > > > The end state version won't be using SchedulerWorkerPoolParams as direct > > > > parameters to the SchedulerWorkerPoolImpls, right? > > > > > > > > I'd be okay with starting that work here. > > > > > > Yes it will > > > > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc.... > > > > > > SchedulerWorkerPoolParams is exposed outside of TaskScheduler, so it can't > > > contain params that can't be customized. > > > > So once we remove those params, what will this file use? > > Inlining here so we don't lose track of the thread: > fdoray: > The constructor of SWP will have name, priority_hint and > scheduler_worker_pool_params arguments. TaskSchedulerImpl will instantiate SWPs > with predefined names and priority_hints and externally provided > scheduler_worker_pool_params. This file will not have knowledge of the > predefined names and priority_hints. > > See reference CL. I guess this is where I'm a little confused. From the reference CL https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/ta... kEnvironmentParams prescribes a name and priority to be used in SchedulerWorkerPoolImpl::Create In this CL and in https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/ta... , we specify a fixed set of worker pools. In this CL, https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/sch... has a comment to remove the constructor that has |name| and |priority_hint| (the one used below). The reference CL https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc... also removes |name| and |priority_hint|. So if we remove |name| and |priority_hint|, this file will have to use something else, won't it? Feel free to continue this thread on hangouts.
https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, On 2017/03/20 22:06:27, robliao wrote: > On 2017/03/20 21:55:00, robliao wrote: > > On 2017/03/20 21:00:56, robliao wrote: > > > On 2017/03/20 20:55:35, fdoray wrote: > > > > On 2017/03/20 20:47:32, robliao wrote: > > > > > The end state version won't be using SchedulerWorkerPoolParams as direct > > > > > parameters to the SchedulerWorkerPoolImpls, right? > > > > > > > > > > I'd be okay with starting that work here. > > > > > > > > Yes it will > > > > > > > > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc.... > > > > > > > > SchedulerWorkerPoolParams is exposed outside of TaskScheduler, so it can't > > > > contain params that can't be customized. > > > > > > So once we remove those params, what will this file use? > > > > Inlining here so we don't lose track of the thread: > > fdoray: > > The constructor of SWP will have name, priority_hint and > > scheduler_worker_pool_params arguments. TaskSchedulerImpl will instantiate > SWPs > > with predefined names and priority_hints and externally provided > > scheduler_worker_pool_params. This file will not have knowledge of the > > predefined names and priority_hints. > > > > See reference CL. > > I guess this is where I'm a little confused. > > From the reference CL > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/ta... > kEnvironmentParams prescribes a name and priority to be used in > SchedulerWorkerPoolImpl::Create > > In this CL and in In this CL, TaskSchedulerImpl is still initialized with a vector of SchedulerWorkerPoolParams. The new API calls that old API. > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/ta... > , we specify a fixed set of worker pools. > > In this CL, > https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/sch... > has a comment to remove the constructor that has |name| and |priority_hint| (the > one used below). > > The reference CL > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc... > also removes |name| and |priority_hint|. > > So if we remove |name| and |priority_hint|, this file will have to use something > else, won't it? This file won't have knowledge of names and priority hints. Those will be hard-coded in task_scheduler_impl.cc (kEnvironmentParams). Names and priority hints are currently hard-coded in this file, but this is just temporary. > > Feel free to continue this thread on hangouts. The final state is: -- task_scheduler.cc -- void TaskScheduler::CreateAndSetDefaultTaskScheduler( const std::string& name, const TaskSchedulerInitParams& init_params) { SetInstance(internal::TaskSchedulerImpl::Create(name, init_params)); } -- task_scheduler_impl.cc -- std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create( const std::string& name, const InitParams& init_params) { std::unique_ptr<TaskSchedulerImpl> scheduler(new TaskSchedulerImpl(name)); scheduler->Initialize(init_params); return scheduler; } void TaskSchedulerImpl::Initialize(const InitParams& init_params) { ... ************ Start four worker pools using the names and priority hints from |kEnvironmentParams| (not customizable) and the SchedulerWorkerPoolParams in |init_params| (customizable). ************ // Start worker pools. for (size_t index = 0; index < ENVIRONMENT_COUNT; ++index) { worker_pools_[index] = SchedulerWorkerPoolImpl::Create( name_ + kEnvironmentParams[index].name_suffix, kEnvironmentParams[index].priority_hint, *reinterpret_cast<const SchedulerWorkerPoolParams*>( reinterpret_cast<const char*>(&init_params) + kEnvironmentParams[index].offset), ...); } ... }
https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, On 2017/03/21 14:41:26, fdoray wrote: > On 2017/03/20 22:06:27, robliao wrote: > > On 2017/03/20 21:55:00, robliao wrote: > > > On 2017/03/20 21:00:56, robliao wrote: > > > > On 2017/03/20 20:55:35, fdoray wrote: > > > > > On 2017/03/20 20:47:32, robliao wrote: > > > > > > The end state version won't be using SchedulerWorkerPoolParams as > direct > > > > > > parameters to the SchedulerWorkerPoolImpls, right? > > > > > > > > > > > > I'd be okay with starting that work here. > > > > > > > > > > Yes it will > > > > > > > > > > > > > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc.... > > > > > > > > > > SchedulerWorkerPoolParams is exposed outside of TaskScheduler, so it > can't > > > > > contain params that can't be customized. > > > > > > > > So once we remove those params, what will this file use? > > > > > > Inlining here so we don't lose track of the thread: > > > fdoray: > > > The constructor of SWP will have name, priority_hint and > > > scheduler_worker_pool_params arguments. TaskSchedulerImpl will instantiate > > SWPs > > > with predefined names and priority_hints and externally provided > > > scheduler_worker_pool_params. This file will not have knowledge of the > > > predefined names and priority_hints. > > > > > > See reference CL. > > > > I guess this is where I'm a little confused. > > > > From the reference CL > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/ta... > > kEnvironmentParams prescribes a name and priority to be used in > > SchedulerWorkerPoolImpl::Create > > > > In this CL and in > > In this CL, TaskSchedulerImpl is still initialized with a vector of > SchedulerWorkerPoolParams. The new API calls that old API. > > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/ta... > > , we specify a fixed set of worker pools. > > > > In this CL, > > > https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/sch... > > has a comment to remove the constructor that has |name| and |priority_hint| > (the > > one used below). > > > > The reference CL > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc... > > also removes |name| and |priority_hint|. > > > > So if we remove |name| and |priority_hint|, this file will have to use > something > > else, won't it? > > This file won't have knowledge of names and priority hints. Those will be > hard-coded in task_scheduler_impl.cc (kEnvironmentParams). Names and priority > hints are currently hard-coded in this file, but this is just temporary. > > > > > Feel free to continue this thread on hangouts. > > The final state is: > > -- task_scheduler.cc -- > void TaskScheduler::CreateAndSetDefaultTaskScheduler( > const std::string& name, > const TaskSchedulerInitParams& init_params) { > SetInstance(internal::TaskSchedulerImpl::Create(name, init_params)); > } > > -- task_scheduler_impl.cc -- > std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create( > const std::string& name, > const InitParams& init_params) { > std::unique_ptr<TaskSchedulerImpl> scheduler(new TaskSchedulerImpl(name)); > scheduler->Initialize(init_params); > return scheduler; > } > > void TaskSchedulerImpl::Initialize(const InitParams& init_params) { > ... > > ************ > Start four worker pools using the names and priority hints from > |kEnvironmentParams| (not customizable) and the > SchedulerWorkerPoolParams in |init_params| (customizable). > ************ > > // Start worker pools. > for (size_t index = 0; index < ENVIRONMENT_COUNT; ++index) { > worker_pools_[index] = SchedulerWorkerPoolImpl::Create( > name_ + kEnvironmentParams[index].name_suffix, > kEnvironmentParams[index].priority_hint, > *reinterpret_cast<const SchedulerWorkerPoolParams*>( > reinterpret_cast<const char*>(&init_params) + > kEnvironmentParams[index].offset), > ...); > } > ... > } > Summary from offline discussion: An alternative solution is to have this split be overloaded at TaskSchedulerImpl and have TaskSchedulerImpl handle initialization from either a vector+function or InitParams. This means we can avoid the removal of this code later on.
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...
Description was changed from ========== Add TaskScheduler::InitParams. TaskScheduler::InitParams is a struct of 4 SchedulerWorkerPoolParams. Eventually, TaskSchedulerImpl will always be initialized with it (it will no longer be possible to provide a vector with an arbitrary number of SchedulerWorkerPoolParams). Reference CL: https://codereview.chromium.org/2749303002/ BUG=690706 ========== to ========== Add TaskScheduler::InitParams and accept it in TaskSchedulerImpl constructor. TaskScheduler::InitParams is a struct of 4 SchedulerWorkerPoolParams. Eventually, TaskSchedulerImpl will always be initialized with it (it will no longer be possible to provide a vector with an arbitrary number of SchedulerWorkerPoolParams). Reference CL: https://codereview.chromium.org/2749303002/ BUG=690706 ==========
PTAnL https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/tas... base/task_scheduler/task_scheduler.cc:26: ThreadPriority priority_hint, On 2017/03/21 20:02:29, robliao wrote: > On 2017/03/21 14:41:26, fdoray wrote: > > On 2017/03/20 22:06:27, robliao wrote: > > > On 2017/03/20 21:55:00, robliao wrote: > > > > On 2017/03/20 21:00:56, robliao wrote: > > > > > On 2017/03/20 20:55:35, fdoray wrote: > > > > > > On 2017/03/20 20:47:32, robliao wrote: > > > > > > > The end state version won't be using SchedulerWorkerPoolParams as > > direct > > > > > > > parameters to the SchedulerWorkerPoolImpls, right? > > > > > > > > > > > > > > I'd be okay with starting that work here. > > > > > > > > > > > > Yes it will > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc.... > > > > > > > > > > > > SchedulerWorkerPoolParams is exposed outside of TaskScheduler, so it > > can't > > > > > > contain params that can't be customized. > > > > > > > > > > So once we remove those params, what will this file use? > > > > > > > > Inlining here so we don't lose track of the thread: > > > > fdoray: > > > > The constructor of SWP will have name, priority_hint and > > > > scheduler_worker_pool_params arguments. TaskSchedulerImpl will instantiate > > > SWPs > > > > with predefined names and priority_hints and externally provided > > > > scheduler_worker_pool_params. This file will not have knowledge of the > > > > predefined names and priority_hints. > > > > > > > > See reference CL. > > > > > > I guess this is where I'm a little confused. > > > > > > From the reference CL > > > > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/ta... > > > kEnvironmentParams prescribes a name and priority to be used in > > > SchedulerWorkerPoolImpl::Create > > > > > > In this CL and in > > > > In this CL, TaskSchedulerImpl is still initialized with a vector of > > SchedulerWorkerPoolParams. The new API calls that old API. > > > > > > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/ta... > > > , we specify a fixed set of worker pools. > > > > > > In this CL, > > > > > > https://codereview.chromium.org/2764603002/diff/20001/base/task_scheduler/sch... > > > has a comment to remove the constructor that has |name| and |priority_hint| > > (the > > > one used below). > > > > > > The reference CL > > > > > > https://codereview.chromium.org/2749303002/diff/160001/base/task_scheduler/sc... > > > also removes |name| and |priority_hint|. > > > > > > So if we remove |name| and |priority_hint|, this file will have to use > > something > > > else, won't it? > > > > This file won't have knowledge of names and priority hints. Those will be > > hard-coded in task_scheduler_impl.cc (kEnvironmentParams). Names and priority > > hints are currently hard-coded in this file, but this is just temporary. > > > > > > > > Feel free to continue this thread on hangouts. > > > > The final state is: > > > > -- task_scheduler.cc -- > > void TaskScheduler::CreateAndSetDefaultTaskScheduler( > > const std::string& name, > > const TaskSchedulerInitParams& init_params) { > > SetInstance(internal::TaskSchedulerImpl::Create(name, init_params)); > > } > > > > -- task_scheduler_impl.cc -- > > std::unique_ptr<TaskSchedulerImpl> TaskSchedulerImpl::Create( > > const std::string& name, > > const InitParams& init_params) { > > std::unique_ptr<TaskSchedulerImpl> scheduler(new TaskSchedulerImpl(name)); > > scheduler->Initialize(init_params); > > return scheduler; > > } > > > > void TaskSchedulerImpl::Initialize(const InitParams& init_params) { > > ... > > > > ************ > > Start four worker pools using the names and priority hints from > > |kEnvironmentParams| (not customizable) and the > > SchedulerWorkerPoolParams in |init_params| (customizable). > > ************ > > > > // Start worker pools. > > for (size_t index = 0; index < ENVIRONMENT_COUNT; ++index) { > > worker_pools_[index] = SchedulerWorkerPoolImpl::Create( > > name_ + kEnvironmentParams[index].name_suffix, > > kEnvironmentParams[index].priority_hint, > > *reinterpret_cast<const SchedulerWorkerPoolParams*>( > > reinterpret_cast<const char*>(&init_params) + > > kEnvironmentParams[index].offset), > > ...); > > } > > ... > > } > > > > Summary from offline discussion: > An alternative solution is to have this split be overloaded at TaskSchedulerImpl > and have TaskSchedulerImpl handle initialization from either a vector+function > or InitParams. This means we can avoid the removal of this code later on. 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Thanks for the changes! This will allow for an easier transition. https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler.cc:24: SchedulerWorkerPoolParams AugmentWorkerPoolParamsWithNameAndPriority( This can be removed, right? https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:40: // task scheduler name + |name_suffix|. Might be clearer to say the name from InitParams + |name_suffix| https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:48: // in TaskSchedulerInitParams. Worth discussing that this allows us to easily use a for loop to get the right init params. https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:94: reinterpret_cast<const char*>(&init_params) + Instead of const char*, should this be uintptr_t instead to allow for varying bitness? https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.h:42: // Note: The names and priority hints in |init_params| are ignored (ref. TODO Reference the bug instead?
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
PTAnL https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler.cc (right): https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler.cc:24: SchedulerWorkerPoolParams AugmentWorkerPoolParamsWithNameAndPriority( On 2017/03/23 18:00:52, robliao wrote: > This can be removed, right? Done. https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.cc (right): https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:40: // task scheduler name + |name_suffix|. On 2017/03/23 18:00:52, robliao wrote: > Might be clearer to say the name from InitParams + |name_suffix| We don't use the name from InitParams, we use the |name| argument of TaskSchedulerImpl::Create() + |name_suffix|. https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:48: // in TaskSchedulerInitParams. On 2017/03/23 18:00:52, robliao wrote: > Worth discussing that this allows us to easily use a for loop to get the right > init params. Done. https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.cc:94: reinterpret_cast<const char*>(&init_params) + On 2017/03/23 18:00:52, robliao wrote: > Instead of const char*, should this be uintptr_t instead to allow for varying > bitness? Done. https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler_impl.h (right): https://codereview.chromium.org/2764603002/diff/160001/base/task_scheduler/ta... base/task_scheduler/task_scheduler_impl.h:42: // Note: The names and priority hints in |init_params| are ignored (ref. TODO On 2017/03/23 18:00:52, robliao wrote: > Reference the bug instead? Done.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
lgtm. Thanks for taking the time to make the changes!
https://codereview.chromium.org/2764603002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2764603002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_scheduler.h:40: struct BASE_EXPORT InitParams { You'll want to fix this: E:\b\c\b\win_clang\src\base\task_scheduler\task_scheduler.h(40,3): error: [chromium-style] Complex class/struct needs an explicit out-of-line destructor. struct BASE_EXPORT InitParams { ^
https://codereview.chromium.org/2764603002/diff/200001/base/task_scheduler/ta... File base/task_scheduler/task_scheduler.h (right): https://codereview.chromium.org/2764603002/diff/200001/base/task_scheduler/ta... base/task_scheduler/task_scheduler.h:40: struct BASE_EXPORT InitParams { On 2017/03/24 21:41:06, robliao wrote: > You'll want to fix this: > > E:\b\c\b\win_clang\src\base\task_scheduler\task_scheduler.h(40,3): error: > [chromium-style] Complex class/struct needs an explicit out-of-line destructor. > struct BASE_EXPORT InitParams { > ^ Done.
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, robliao@chromium.org Link to the patchset: https://codereview.chromium.org/2764603002/#ps220001 (title: "add ~InitParams")
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: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
win-clang doesn't like how I use offsetof: E:\b\c\b\win_clang\src\base\task_scheduler\task_scheduler_impl.cc(53,6): error: offset of on non-standard-layout type 'TaskScheduler::InitParams' [-Werror,-Winvalid-offsetof] offsetof(TaskScheduler::InitParams, background_worker_pool_params)}, ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Latest patch set use a slightly more verbose but less obscure way of creating the vector of SchedulerWorkerPoolParams.
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/2764603002/#ps240001 (title: "no offsetof")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/28 16:41:40, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Kind of surprised it didn't like it. This way looks fine too. lgtm.
CQ is committing da patch. Bot data: {"patchset_id": 240001, "attempt_start_ts": 1490719268007500, "parent_rev": "8ff20f23f09ac7211555bbe9f519cf49c9bfe0e7", "commit_rev": "3f6f57a4713fc45b7e4fddf588df5aa43c909457"}
Message was sent while issue was closed.
Description was changed from ========== Add TaskScheduler::InitParams and accept it in TaskSchedulerImpl constructor. TaskScheduler::InitParams is a struct of 4 SchedulerWorkerPoolParams. Eventually, TaskSchedulerImpl will always be initialized with it (it will no longer be possible to provide a vector with an arbitrary number of SchedulerWorkerPoolParams). Reference CL: https://codereview.chromium.org/2749303002/ BUG=690706 ========== to ========== Add TaskScheduler::InitParams and accept it in TaskSchedulerImpl constructor. TaskScheduler::InitParams is a struct of 4 SchedulerWorkerPoolParams. Eventually, TaskSchedulerImpl will always be initialized with it (it will no longer be possible to provide a vector with an arbitrary number of SchedulerWorkerPoolParams). Reference CL: https://codereview.chromium.org/2749303002/ BUG=690706 Review-Url: https://codereview.chromium.org/2764603002 Cr-Commit-Position: refs/heads/master@{#460167} Committed: https://chromium.googlesource.com/chromium/src/+/3f6f57a4713fc45b7e4fddf588df... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001) as https://chromium.googlesource.com/chromium/src/+/3f6f57a4713fc45b7e4fddf588df... |