Refactor WorkerPoolCreationArgs to a Read-Only WorkerPoolParams

base/task_scheduler/worker_pool_params.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_TASK_SCHEDULER_WORKER_POOL_PARAMS_H_
+#define BASE_TASK_SCHEDULER_WORKER_POOL_PARAMS_H_
+
+#include <string>
+
+#include "base/task_scheduler/scheduler_worker_pool_impl.h"
+#include "base/threading/platform_thread.h"
+
+namespace base {
+namespace internal {

[gab, 2016/07/18 15:24:13]

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?

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

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:51:11]

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.

+
+class BASE_EXPORT WorkerPoolParams final {

[gab, 2016/07/18 15:24:13]

SchedulerWorkerPoolParams ? (unfortunately we have to prefix all our stuff with
"Scheduler" IMO because "WorkerPool" is something else in base/)

[robliao, 2016/07/18 19:51:11]

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.

+ public:
+  // Construct a worker pool parameter object that instructs a worker pool to
+  // use the label |name| and create up to |max_threads| threads of priority
+  // |thread_priority|. |io_restriction| indicates whether Tasks on the worker
+  // pool are allowed to make I/O calls.

[gab, 2016/07/18 15:24:13]

s/worker pool/scheduler work pool/

[robliao, 2016/07/18 19:51:11]

See earlier comment about scheduler worker pool vs worker pool.

+  WorkerPoolParams(
+      const std::string& name,
+      ThreadPriority thread_priority,
+      SchedulerWorkerPoolImpl::IORestriction io_restriction,
+      int max_threads);
+  WorkerPoolParams(const WorkerPoolParams& other) = default;

[gab, 2016/07/18 15:24:13]

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)

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

Done.

+
+  // Name of the pool. Used to label the pool's threads.
+  const std::string& name() const { return name_; }
+
+  // Priority of the pool's threads.
+  ThreadPriority thread_priority() const { return thread_priority_; }
+
+  // Whether I/O is allowed in the pool.

[gab, 2016/07/18 15:24:13]

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.

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

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.

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

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

+  SchedulerWorkerPoolImpl::IORestriction io_restriction() const {
+    return io_restriction_;
+  }
+
+  // Maximum number of threads in the pool.
+  size_t max_threads() const { return max_threads_; }
+
+ private:
+  std::string name_;
+  ThreadPriority thread_priority_;
+  SchedulerWorkerPoolImpl::IORestriction io_restriction_;
+  size_t max_threads_;
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_TASK_SCHEDULER_WORKER_POOL_PARAMS_H_