TaskScheduler: Make the worker pools of TaskSchedulerImpl configurable

base/task_scheduler/task_scheduler.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_TASK_SCHEDULER_H_
 #define BASE_TASK_SCHEDULER_TASK_SCHEDULER_H_
 
+#include <map>
 #include <memory>
+#include <string>
 
 #include "base/base_export.h"
 #include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "base/task_runner.h"
 #include "base/task_scheduler/task_traits.h"
 
 namespace tracked_objects {
 class Location;
 }
@@ skipping 34 matching lines @@
   // registered TaskScheduler will only be deleted when a new TaskScheduler is
   // registered (i.e. otherwise leaked on shutdown). The methods must not be
   // called when TaskRunners created by the previous TaskScheduler are still
   // alive. The methods are not thread-safe; proper synchronization is required
   // to use the post_task.h API after registering a new TaskScheduler.
 
   // Registers |task_scheduler| to handle tasks posted through the post_task.h
   // API for this process.
   static void SetInstance(std::unique_ptr<TaskScheduler> task_scheduler);
 
-  // Initializes the default task scheduler for this process.
-  static void InitializeDefaultTaskScheduler();
+  // Initializes the default task scheduler for this process. |variation_params|
+  // is used to configure the scheduler (can be empty).
+  static void InitializeDefaultTaskScheduler(

[robliao, 2016/06/14 21:43:44]

Can TaskSchedulerImpl pick these up without the caller needing to supply them?

[fdoray, 2016/06/15 17:14:32]

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()).

[robliao, 2016/06/15 17:32:37]

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?

[fdoray, 2016/06/15 19:54:53]

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.

[gab, 2016/06/15 19:56:03]

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.

[fdoray, 2016/06/16 13:02:05]

gab@: WDYT of my new idea? (2016/06/15 19:54:53
https://codereview.chromium.org/2064073003/diff/1/base/task_scheduler/task_sc...)

[gab, 2016/06/16 15:07:07]

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.

[fdoray, 2016/06/16 18:10:53]

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?

+      const std::map<std::string, std::string>& variation_params);
 
   // Retrieve the TaskScheduler set via SetInstance() or
   // InitializeDefaultTaskScheduler(). This should be used very rarely; most
   // users of TaskScheduler should use the post_task.h API.
   static TaskScheduler* GetInstance();
 };
 
 }  // namespace base
 
 #endif  // BASE_TASK_SCHEDULER_TASK_SCHEDULER_H_