Refactor WorkerPoolCreationArgs to a Read-Only WorkerPoolParams

Refactor WorkerPoolCreationArgs to a Read-Only WorkerPoolParams

This new layout requires callers to specify their desired behavior with regards
to the scheduler worker pool.

Additionally, this also plumbs WorkerPoolParams straight to
SchedulerWorkerPoolImpl so that additional parameters can be added without
changing the constructor signature for WorkerPoolParams and creation method
signature for SchedulerWorkerPoolImpl

BUG=553459
Committed: https://crrev.com/5e490aa00595487db9a89efbbbd46a612d7be2e4
Cr-Commit-Position: refs/heads/master@{#406680}

[robliao, 2016-07-13 23:20:41]

Patchset #1 (id:1) has been deleted

[robliao, 2016-07-13 23:21:28]

robliao@chromium.org changed reviewers:
+ fdoray@chromium.org, gab@chromium.org

[robliao, 2016-07-13 23:42:45]

The CQ bit was checked by robliao@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 23:43:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-07-14 00:18:23]

Patchset #1 (id:20001) has been deleted

[gab, 2016-07-18 15:24:14]

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (right):

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:231: const WorkerPoolParams&
worker_pool_params,
s/worker_pool_params/params/ (just as descriptive in this context but less
wordy?)

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
File base/task_scheduler/worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:14: namespace internal {
These are intended to be used by callers of our API and as such shouldn't be in
internal:: I think. On a similar note, should TaskSchedulerImpl also not be in
internal::?

Or should we have
TaskScheduler::Create(std::vector<SchedulerWorkerPoolParams>&...) and keep the
impl hidden?

The intent was previously that people could want to set other implementations of
TaskScheduler, but perhaps this is no longer true with the params?

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:16: class BASE_EXPORT WorkerPoolParams
final {
SchedulerWorkerPoolParams ? (unfortunately we have to prefix all our stuff with
"Scheduler" IMO because "WorkerPool" is something else in base/)

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:21: // pool are allowed to make I/O
calls.
s/worker pool/scheduler work pool/

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:27: WorkerPoolParams(const
WorkerPoolParams& other) = default;
Explicitly declare default move-constructor (out-of-line) to avoid string copies
and DISALLOW_COPY_AND_ASSIGN below?

(also need to declare default move-assignment operator per style-guide even
though it's unnecessary for STL)

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:35: // Whether I/O is allowed in the
pool.
I don't think these method comments are necessary. The constructor explicitly
states what they do and then it's immutable so these are redundant.

[fdoray, 2016-07-18 15:51:11]

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
File base/task_scheduler/worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:14: namespace internal {
On 2016/07/18 15:24:13, gab wrote:
> These are intended to be used by callers of our API and as such shouldn't be
in
> internal:: I think. On a similar note, should TaskSchedulerImpl also not be in
> internal::?
> 
> Or should we have
> TaskScheduler::Create(std::vector<SchedulerWorkerPoolParams>&...) and keep the
> impl hidden?
> 
> The intent was previously that people could want to set other implementations
of
> TaskScheduler, but perhaps this is no longer true with the params?

Is there a reason why consumers of the API would want to create their own
TaskScheduler? Maybe in renderers?

If yes -> I would move TaskSchedulerImpl to base:: and let consumers instantiate
it directly.
If not -> I would keep TaskSchedulerImpl in base::internal:: and require that
consumers use TaskScheduler::Create to instantiate a TaskSchedulerImpl and
register it for the current process. We could have Create* methods for other
generic impls (ForTesting, WindowsThreadPool).

[robliao, 2016-07-18 19:44:25]

Patchset #2 (id:60001) has been deleted

[robliao, 2016-07-18 19:51:12]

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/sch...
File base/task_scheduler/scheduler_worker_pool_impl.cc (right):

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/sch...
base/task_scheduler/scheduler_worker_pool_impl.cc:231: const WorkerPoolParams&
worker_pool_params,
On 2016/07/18 15:24:13, gab wrote:
> s/worker_pool_params/params/ (just as descriptive in this context but less
> wordy?)

sgtm. Done.

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
File base/task_scheduler/worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:14: namespace internal {
On 2016/07/18 15:24:13, gab wrote:
> These are intended to be used by callers of our API and as such shouldn't be
in
> internal:: I think. On a similar note, should TaskSchedulerImpl also not be in
> internal::?
> 
> Or should we have
> TaskScheduler::Create(std::vector<SchedulerWorkerPoolParams>&...) and keep the
> impl hidden?
> 
> The intent was previously that people could want to set other implementations
of
> TaskScheduler, but perhaps this is no longer true with the params?

Yup. I noticed this when I coded this up. It was currently internal so I kept it
internal and planned for a internal -> base change after this.

The operating assumption before was that we would have a default constructable
TaskScheduler invokable from post_task.h. This allowed us to keep
TaskSchedulerImpl in base::internal. That changed with the introduction of
WorkerPool parameters, so we will indeed need to pull these into the base
namespace.

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:16: class BASE_EXPORT WorkerPoolParams
final {
On 2016/07/18 15:24:13, gab wrote:
> SchedulerWorkerPoolParams ? (unfortunately we have to prefix all our stuff
with
> "Scheduler" IMO because "WorkerPool" is something else in base/)

The WorkerPoolParams may not just apply to the SchedulerWorkerPool in the future
(e.g. if we have a WindowsWorkerPool). WorkerPoolParams seemed to allow for
that.

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:21: // pool are allowed to make I/O
calls.
On 2016/07/18 15:24:13, gab wrote:
> s/worker pool/scheduler work pool/

See earlier comment about scheduler worker pool vs worker pool.

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:27: WorkerPoolParams(const
WorkerPoolParams& other) = default;
On 2016/07/18 15:24:13, gab wrote:
> Explicitly declare default move-constructor (out-of-line) to avoid string
copies
> and DISALLOW_COPY_AND_ASSIGN below?
> 
> (also need to declare default move-assignment operator per style-guide even
> though it's unnecessary for STL)

Done.

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:35: // Whether I/O is allowed in the
pool.
On 2016/07/18 15:24:13, gab wrote:
> I don't think these method comments are necessary. The constructor explicitly
> states what they do and then it's immutable so these are redundant.

I agree, but it's idiomatic with what we have in the Scheduler code today (e.g.
TaskTraits does this too). I think we could stand to do some redundant comment
cleanup.

[gab, 2016-07-19 18:04:08]

lgtm w/ comment and rename (plus comments adjustments from previous review) as
discussed offline

https://codereview.chromium.org/2146223002/diff/80001/base/task_scheduler/wor...
File base/task_scheduler/worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/80001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:28: WorkerPoolParams(WorkerPoolParams&&
other);
Also need to declare default move-assignment operator per-style guide even
though it's unnecessary for STL.

https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types

" If you define a copy or move constructor, define the corresponding assignment
operator, and vice-versa."

[fdoray, 2016-07-19 18:24:49]

lgtm

https://codereview.chromium.org/2146223002/diff/80001/base/task_scheduler/wor...
File base/task_scheduler/worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/80001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:11: #include
"base/task_scheduler/scheduler_worker_pool_impl.h"
Maybe define IORestriction in WorkerPoolParams to avoid this include?

[robliao, 2016-07-19 20:05:20]

Patchset #3 (id:90001) has been deleted

[robliao, 2016-07-19 20:34:55]

Applied the renames. Feel free to take a look again to make sure I didn't miss
anything.

https://codereview.chromium.org/2146223002/diff/80001/base/task_scheduler/wor...
File base/task_scheduler/worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/80001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:11: #include
"base/task_scheduler/scheduler_worker_pool_impl.h"
On 2016/07/19 18:24:49, fdoray wrote:
> Maybe define IORestriction in WorkerPoolParams to avoid this include?

Done.

https://codereview.chromium.org/2146223002/diff/80001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:28: WorkerPoolParams(WorkerPoolParams&&
other);
On 2016/07/19 18:04:08, gab wrote:
> Also need to declare default move-assignment operator per-style guide even
> though it's unnecessary for STL.
> 
> https://google.github.io/styleguide/cppguide.html#Copyable_Movable_Types
> 
> " If you define a copy or move constructor, define the corresponding
assignment
> operator, and vice-versa."

Ah yup, you're right! Done.

[gab, 2016-07-20 01:08:27]

lgtm % nits

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/sc...
File base/task_scheduler/scheduler_worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker_pool_params.h:23: // Construct a worker
pool parameter object that instructs a worker pool to
s/worker pool/scheduler worker pool/

(3 places in this comment)

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:23: const
std::vector<SchedulerWorkerPoolParams>& worker_pool_params,
|params_list|

(same in header)

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:88: const
std::vector<SchedulerWorkerPoolParams>& worker_pool_params) {
|params_list|

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:95: for (const auto&
worker_pool_param : worker_pool_params) {
|params : params_list|

(i.e. items are still plural "params")

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
File base/task_scheduler/task_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl_unittest.cc:164:
std::vector<SchedulerWorkerPoolParams> params;
|params_list| to follow suggestions elsewhere

[robliao, 2016-07-20 17:24:28]

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/sc...
File base/task_scheduler/scheduler_worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/sc...
base/task_scheduler/scheduler_worker_pool_params.h:23: // Construct a worker
pool parameter object that instructs a worker pool to
On 2016/07/20 01:08:27, gab wrote:
> s/worker pool/scheduler worker pool/
> 
> (3 places in this comment)

Done.

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
File base/task_scheduler/task_scheduler_impl.cc (right):

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:23: const
std::vector<SchedulerWorkerPoolParams>& worker_pool_params,
On 2016/07/20 01:08:27, gab wrote:
> |params_list|
> 
> (same in header)

Going with worker_pool_param_vector. I want to keep this one explicit as there
is the potential to add more non-worker pool parameters in the future.

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:88: const
std::vector<SchedulerWorkerPoolParams>& worker_pool_params) {
On 2016/07/20 01:08:27, gab wrote:
> |params_list|

Done.

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl.cc:95: for (const auto&
worker_pool_param : worker_pool_params) {
On 2016/07/20 01:08:27, gab wrote:
> |params : params_list|
> 
> (i.e. items are still plural "params")

Done.

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
File base/task_scheduler/task_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/2146223002/diff/130001/base/task_scheduler/ta...
base/task_scheduler/task_scheduler_impl_unittest.cc:164:
std::vector<SchedulerWorkerPoolParams> params;
On 2016/07/20 01:08:27, gab wrote:
> |params_list| to follow suggestions elsewhere

Done.

[robliao, 2016-07-20 17:25:00]

robliao@chromium.org changed reviewers:
+ danakj@chromium.org

[robliao, 2016-07-20 17:25:01]

danakj@chromium.org: Please review this CL, thanks!

[gab, 2016-07-20 18:18:42]

On 2016/07/20 17:25:01, robliao wrote:
> https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=danakj@chromium.org: Please
review this CL, thanks!

An FYI CC Dana is good enough for mechanical changes IMO. We should keep her
cycles for scheduler stuff that actually matters.

[robliao, 2016-07-20 18:20:50]

On 2016/07/20 18:18:42, gab wrote:
> On 2016/07/20 17:25:01, robliao wrote:
> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=danakj@chromium.org:
Please
> review this CL, thanks!
> 
> An FYI CC Dana is good enough for mechanical changes IMO. We should keep her
> cycles for scheduler stuff that actually matters.

Indeed, but I do need her approval for the build file changes.

[danakj, 2016-07-20 18:50:39]

LGTM

[robliao, 2016-07-20 19:15:13]

The CQ bit was checked by robliao@chromium.org

[robliao, 2016-07-20 19:15:14]

The patchset sent to the CQ was uploaded after l-g-t-m from fdoray@chromium.org,
gab@chromium.org
Link to the patchset: https://codereview.chromium.org/2146223002/#ps150001
(title: "CR Feedback")

[commit-bot: I haz the power, 2016-07-20 19:15:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-20 20:26:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-20 20:26:09]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[gab, 2016-07-20 20:28:01]

On 2016/07/20 18:20:50, robliao wrote:
> On 2016/07/20 18:18:42, gab wrote:
> > On 2016/07/20 17:25:01, robliao wrote:
> > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=danakj@chromium.org:
> Please
> > review this CL, thanks!
> > 
> > An FYI CC Dana is good enough for mechanical changes IMO. We should keep her
> > cycles for scheduler stuff that actually matters.
> 
> Indeed, but I do need her approval for the build file changes.

Ah, trivial GYP/GN are TBR material by Chromium's TBR rules :-)

[robliao, 2016-07-20 20:59:44]

The CQ bit was checked by robliao@chromium.org

[commit-bot: I haz the power, 2016-07-20 21:00:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[robliao, 2016-07-20 21:00:52]

On 2016/07/20 20:28:01, gab wrote:
> On 2016/07/20 18:20:50, robliao wrote:
> > On 2016/07/20 18:18:42, gab wrote:
> > > On 2016/07/20 17:25:01, robliao wrote:
> > > > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=danakj@chromium.org:
> > Please
> > > review this CL, thanks!
> > > 
> > > An FYI CC Dana is good enough for mechanical changes IMO. We should keep
her
> > > cycles for scheduler stuff that actually matters.
> > 
> > Indeed, but I do need her approval for the build file changes.
> 
> Ah, trivial GYP/GN are TBR material by Chromium's TBR rules :-)

Ah, didn't know that! Thanks for the tip.
I guess we might need to update
https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...

[commit-bot: I haz the power, 2016-07-20 21:47:08]

Committed patchset #5 (id:150001)

[commit-bot: I haz the power, 2016-07-20 21:50:49]

Description was changed from

==========
Refactor WorkerPoolCreationArgs to a Read-Only WorkerPoolParams

This new layout requires callers to specify their desired behavior with regards
to the scheduler worker pool.

Additionally, this also plumbs WorkerPoolParams straight to
SchedulerWorkerPoolImpl so that additional parameters can be added without
changing the constructor signature for WorkerPoolParams and creation method
signature for SchedulerWorkerPoolImpl

BUG=553459
==========

to

==========
Refactor WorkerPoolCreationArgs to a Read-Only WorkerPoolParams

This new layout requires callers to specify their desired behavior with regards
to the scheduler worker pool.

Additionally, this also plumbs WorkerPoolParams straight to
SchedulerWorkerPoolImpl so that additional parameters can be added without
changing the constructor signature for WorkerPoolParams and creation method
signature for SchedulerWorkerPoolImpl

BUG=553459

Committed: https://crrev.com/5e490aa00595487db9a89efbbbd46a612d7be2e4
Cr-Commit-Position: refs/heads/master@{#406680}
==========

[commit-bot: I haz the power, 2016-07-20 21:50:51]

Patchset 5 (id:??) landed as
https://crrev.com/5e490aa00595487db9a89efbbbd46a612d7be2e4
Cr-Commit-Position: refs/heads/master@{#406680}

[gab, 2016-07-21 13:12:20]

On 2016/07/20 21:00:52, robliao wrote:
> On 2016/07/20 20:28:01, gab wrote:
> > On 2016/07/20 18:20:50, robliao wrote:
> > > On 2016/07/20 18:18:42, gab wrote:
> > > > On 2016/07/20 17:25:01, robliao wrote:
> > > > >
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=danakj@chromium.org:
> > > Please
> > > > review this CL, thanks!
> > > > 
> > > > An FYI CC Dana is good enough for mechanical changes IMO. We should keep
> her
> > > > cycles for scheduler stuff that actually matters.
> > > 
> > > Indeed, but I do need her approval for the build file changes.
> > 
> > Ah, trivial GYP/GN are TBR material by Chromium's TBR rules :-)
> 
> Ah, didn't know that! Thanks for the tip.
> I guess we might need to update
>
https://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Review...

Here's the only official thing I could find on this:
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/jTRrdNZatKM/dis....
But I'm pretty sure I've seen it elsewhere (i.e. probably been told by top-level
owners on past reviews). Feel free to re-open the discussion officially and
change the documentation (or just do it until someone barks -- no one will ;-)).

[fdoray, 2016-08-04 20:42:51]

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
File base/task_scheduler/worker_pool_params.h (right):

https://codereview.chromium.org/2146223002/diff/40001/base/task_scheduler/wor...
base/task_scheduler/worker_pool_params.h:35: // Whether I/O is allowed in the
pool.
On 2016/07/18 19:51:12, robliao wrote:
> On 2016/07/18 15:24:13, gab wrote:
> > I don't think these method comments are necessary. The constructor
explicitly
> > states what they do and then it's immutable so these are redundant.
> 
> I agree, but it's idiomatic with what we have in the Scheduler code today
(e.g.
> TaskTraits does this too). I think we could stand to do some redundant comment
> cleanup.

The constructor of TaskTraits doesn't have arguments. The comment on it doesn't
describe individual members/arguments.