Initialize the TaskScheduler with Variation Parameters if Available

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index d1a7a6e3866a7442a68adb0d2c8ef6b4d0b92f2d..078bc919fcc9ff7b1bc341e890035a22ad7bf299 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -7,6 +7,8 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <algorithm>
+#include <map>
 #include <set>
 #include <string>
 #include <utility>
@@ -33,6 +35,9 @@
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/sys_info.h"
+#include "base/task_scheduler/scheduler_worker_pool_params.h"
+#include "base/task_scheduler/task_scheduler.h"
+#include "base/task_scheduler/task_traits.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
@@ -317,6 +322,158 @@ void AddFirstRunNewTabs(StartupBrowserCreator* browser_creator,
 }
 #endif  // !defined(OS_ANDROID) && !defined(OS_CHROMEOS)
 
+enum WorkerPoolType : size_t {

[fdoray, 2016/08/05 20:58:54]

enum class?

[robliao, 2016/08/05 22:23:28]

I kept this as an enum for the similar reason we did in the tests: It didn't
seem worth it to throw static_casts everywhere since we're really using the
underlying value of this enum.

+  BACKGROUND_WORKER_POOL = 0,
+  BACKGROUND_FILE_IO_WORKER_POOL,
+  FOREGROUND_WORKER_POOL,
+  FOREGROUND_FILE_IO_WORKER_POOL,
+  WORKER_POOL_COUNT,
+};
+
+struct WorkerPoolVariationValues {
+  WorkerPoolVariationValues()
+      : threads(0),
+        detach_milliseconds(0) {}
+
+  int threads;

[fdoray, 2016/08/05 20:58:53]

struct WorkerPoolVariationValues {
  int threads = 0;
  int64_t detach_milliseconds = 0;
};

[robliao, 2016/08/05 22:23:28]

Done. I kept forgetting about inline initialization in C++!

+  int64_t detach_milliseconds;
+};
+
+// Converts |pool_descriptor| to a WorkerPoolVariationValues. Returns a default

[fdoray, 2016/08/05 20:58:53]

|pool_descriptor| is a string that follows the format "minimum number of
threads,maximum number of threads,..."

[robliao, 2016/08/05 22:23:29]

Done.

+// WorkerPoolVariationValues on failure.

[fdoray, 2016/08/05 20:58:54]

No need to document what the function does after NOTREACHED();

[robliao, 2016/08/05 22:23:28]

See below on the NOTREACHED discussion. Changed to a LOG(WARNING) to be
resilient to failure.

+WorkerPoolVariationValues StringToWorkerPoolVariationValues(
+    const base::StringPiece& pool_descriptor) {
+  std::vector<std::string> tokens = SplitString(pool_descriptor, ",",

[fdoray, 2016/08/05 20:58:53]

const

[robliao, 2016/08/05 22:23:29]

Done.

+                                                base::KEEP_WHITESPACE,
+                                                base::SPLIT_WANT_NONEMPTY);
+  int minimum;
+  int maximum;
+  double multiplier;
+  int offset;
+  int detach_milliseconds;
+  // Checking for a size greater than the expected amount allows us to be
+  // forward compatible if we add more variation values.
+  if (tokens.size() >= 5 &&
+      base::StringToInt(tokens[0], &minimum) &&
+      base::StringToInt(tokens[1], &maximum) &&
+      base::StringToDouble(tokens[2], &multiplier) &&
+      base::StringToInt(tokens[3], &offset) &&
+      base::StringToInt(tokens[4], &detach_milliseconds)) {
+    int num_of_cores = base::SysInfo::NumberOfProcessors();
+    int threads = std::round<int>(num_of_cores * multiplier) + offset;

[fdoray, 2016/08/05 20:58:54]

const num_of_cores
const threads

[robliao, 2016/08/05 22:23:28]

Done.

+    WorkerPoolVariationValues values;
+    values.threads = std::min(maximum, std::max(minimum, threads));
+    values.detach_milliseconds = detach_milliseconds;
+    return values;
+  }
+  NOTREACHED() << "Invalid Worker Pool Descriptor: " << pool_descriptor;
+  return WorkerPoolVariationValues();
+}
+
+// Returns the worker pool index for |traits| defaulting to
+// FOREGROUND_WORKER_POOL or FOREGROUND_FILE_IO_WORKER_POOL on unknown
+// priorities.
+size_t WorkerPoolIndexForTraits(const base::TaskTraits& traits) {
+  if (traits.with_file_io()) {
+    return traits.priority() == base::TaskPriority::BACKGROUND
+               ? WorkerPoolType::BACKGROUND_FILE_IO_WORKER_POOL
+               : WorkerPoolType::FOREGROUND_FILE_IO_WORKER_POOL;
+  }
+  return traits.priority() == base::TaskPriority::BACKGROUND
+      ? WorkerPoolType::BACKGROUND_WORKER_POOL
+      : WorkerPoolType::FOREGROUND_WORKER_POOL;
+}
+
+// Initializes the Default Browser Task Scheduler if there is a valid variation
+// parameter for the field trial.
+void MaybeInitializeTaskScheduler() {
+  static const char kFieldTrialName[] = "BrowserScheduler";

[fdoray, 2016/08/05 20:58:54]

constexpr

[robliao, 2016/08/05 22:23:29]

Done.

+  std::map<std::string, std::string> variation_params;
+  if (!variations::GetVariationParams(kFieldTrialName, &variation_params))
+    return;
+
+  // Order matches WorkerPoolType above.
+  static const char* kWorkerPoolNames[] = {

[fdoray, 2016/08/05 20:58:54]

constexpr

[robliao, 2016/08/05 22:23:29]

This triggers a compiler bug, so it's going to have to stay this way for now:
http://stackoverflow.com/questions/35956033/vs-2015-internal-compiler-error-f...

+    "Background",
+    "BackgroundFileIO",
+    "Foreground",
+    "ForegroundFileIO",
+  };
+  static_assert(
+      arraysize(kWorkerPoolNames) == WorkerPoolType::WORKER_POOL_COUNT,
+      "Mismatched Worker Pool Types and Names");
+
+  std::vector<WorkerPoolVariationValues> values;
+  for (size_t i = 0; i < arraysize(kWorkerPoolNames); i++) {
+    const char* current = kWorkerPoolNames[i];
+    auto pair = variation_params.find(current);
+    if (pair == variation_params.end()) {
+      NOTREACHED() << "Missing Worker Pool Configuration: " << current;

[fdoray, 2016/08/05 20:58:54]

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
Should a bad configuration from the server be handled gracefully (remove the
NOTREACHED)?

[robliao, 2016/08/05 22:23:29]

I guess these are coming in from the land of network so anything can happen. It
would be an unfortunate DoS! Changed these to LOG(WARNING) along with the
StringToWorkerPoolVariationValues above.

+      return;
+    }
+
+    const WorkerPoolVariationValues variation_value =
+        StringToWorkerPoolVariationValues(pair->second);
+
+    if (variation_value.threads == 0 ||
+        variation_value.detach_milliseconds == 0) {
+      NOTREACHED() << "Invalid Worker Pool Configuration: " << current <<
+          " [" << pair->second << "]";

[fdoray, 2016/08/05 20:58:53]

ditto

[robliao, 2016/08/05 22:23:28]

See above.

+      return;
+    }
+
+    values.push_back(variation_value);
+  }
+
+  DCHECK_EQ(values.size(), WorkerPoolType::WORKER_POOL_COUNT);

[fdoray, 2016/08/05 20:58:54]

overkill DCHECK?
I don't suggest that you remove it, I'm just curious about the rules that decide
when a DCHECK is appropriate.

[robliao, 2016/08/05 22:23:29]

I guess it's not intrinsic that the values.size() should equal
WORKER_POOL_COUNT. If someone introduced a break in the for-loop rather than a
return, then this DCHECK would trigger. 

+
+  using ThreadPriority = base::ThreadPriority;
+  using IORestriction = base::SchedulerWorkerPoolParams::IORestriction;
+  std::vector<base::SchedulerWorkerPoolParams> params_vector;
+  DCHECK_EQ(params_vector.size(), WorkerPoolType::BACKGROUND_WORKER_POOL);
+  params_vector.emplace_back(
+      kWorkerPoolNames[BACKGROUND_WORKER_POOL],
+      ThreadPriority::BACKGROUND,
+      IORestriction::DISALLOWED,
+      values[BACKGROUND_WORKER_POOL].threads,
+      base::TimeDelta::FromMilliseconds(
+          values[BACKGROUND_WORKER_POOL].detach_milliseconds));
+
+  DCHECK_EQ(params_vector.size(),
+            WorkerPoolType::BACKGROUND_FILE_IO_WORKER_POOL);
+  params_vector.emplace_back(
+      kWorkerPoolNames[BACKGROUND_FILE_IO_WORKER_POOL],
+      ThreadPriority::BACKGROUND,
+      IORestriction::ALLOWED,
+      values[BACKGROUND_FILE_IO_WORKER_POOL].threads,
+      base::TimeDelta::FromMilliseconds(
+          values[BACKGROUND_FILE_IO_WORKER_POOL].detach_milliseconds));
+
+  DCHECK_EQ(params_vector.size(), WorkerPoolType::FOREGROUND_WORKER_POOL);
+  params_vector.emplace_back(
+      kWorkerPoolNames[FOREGROUND_WORKER_POOL],
+      ThreadPriority::NORMAL,
+      IORestriction::DISALLOWED,
+      values[FOREGROUND_WORKER_POOL].threads,
+      base::TimeDelta::FromMilliseconds(
+          values[FOREGROUND_WORKER_POOL].detach_milliseconds));
+
+  DCHECK_EQ(params_vector.size(),
+            WorkerPoolType::FOREGROUND_FILE_IO_WORKER_POOL);
+  params_vector.emplace_back(
+      kWorkerPoolNames[FOREGROUND_FILE_IO_WORKER_POOL],
+      ThreadPriority::NORMAL,
+      IORestriction::ALLOWED,
+      values[FOREGROUND_FILE_IO_WORKER_POOL].threads,
+      base::TimeDelta::FromMilliseconds(
+          values[FOREGROUND_FILE_IO_WORKER_POOL].detach_milliseconds));
+
+  DCHECK_EQ(params_vector.size(), WorkerPoolType::WORKER_POOL_COUNT);
+
+  base::TaskScheduler::CreateAndSetDefaultTaskScheduler(
+      params_vector, base::Bind(WorkerPoolIndexForTraits));
+}
+
 // Returns the new local state object, guaranteed non-NULL.
 // |local_state_task_runner| must be a shutdown-blocking task runner.
 PrefService* InitializeLocalState(
@@ -1214,6 +1371,8 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() {
   // IOThread's initialization which happens in BrowserProcess:PreCreateThreads.
   SetupMetricsAndFieldTrials();
 
+  MaybeInitializeTaskScheduler();
+
   // ChromeOS needs ResourceBundle::InitSharedInstance to be called before this.
   browser_process_->PreCreateThreads();