Split initialization_util into a Hermetic Library and a Variations Library

components/task_scheduler_util/variations/browser_variations_util.cc

Index: components/task_scheduler_util/variations/browser_variations_util.cc
diff --git a/components/task_scheduler_util/variations/browser_variations_util.cc b/components/task_scheduler_util/variations/browser_variations_util.cc
new file mode 100644
index 0000000000000000000000000000000000000000..ee8d13df75e0eaa2386a5f31be5123c5f71d4205
--- /dev/null
+++ b/components/task_scheduler_util/variations/browser_variations_util.cc
@@ -0,0 +1,142 @@
+// 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.
+
+#include "components/task_scheduler_util/variations/browser_variations_util.h"
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_piece.h"
+#include "base/strings/string_split.h"
+#include "base/task_scheduler/initialization_util.h"
+#include "base/task_scheduler/scheduler_worker_pool_params.h"
+#include "base/task_scheduler/switches.h"
+#include "base/task_scheduler/task_traits.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "base/time/time.h"
+#include "build/build_config.h"
+#include "components/task_scheduler_util/initialization/browser_util.h"
+#include "components/variations/variations_associated_data.h"
+
+namespace task_scheduler_util {
+namespace variations {
+
+namespace {
+
+constexpr char kFieldTrialName[] = "BrowserScheduler";
+
+// Converts |pool_descriptor| to a SingleWorkerPoolConfiguration. Returns a
+// default SingleWorkerPoolConfiguration on failure.
+//
+// |pool_descriptor| is a semi-colon separated value string with the following
+// items:
+// 0. Minimum Thread Count (int)
+// 1. Maximum Thread Count (int)
+// 2. Thread Count Multiplier (double)
+// 3. Thread Count Offset (int)
+// 4. Detach Time in Milliseconds (milliseconds)
+// 5. Standby Thread Policy (string)
+// Additional values may appear as necessary and will be ignored.
+initialization::SingleWorkerPoolConfiguration
+StringToSingleWorkerPoolConfiguration(const base::StringPiece pool_descriptor) {
+  using StandbyThreadPolicy =
+      base::SchedulerWorkerPoolParams::StandbyThreadPolicy;
+  const std::vector<std::string> tokens = SplitString(

[gab, 2016/12/06 19:16:07]

Use SplitStringPiece to get a StringPiece vector :)

[robliao, 2016/12/07 00:03:54]

Nice, but there's no StringPiece equivalent of base::StringToDouble. That will
have to happen first.

[gab, 2016/12/07 15:53:06]

Lame, but you can still do this and use StringPiece::as_string() for that one
call.

Or, seems like s/std::string/StringPiece on StringToDouble() should just work
(TM)?!

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

Ah, missed as_string(). I haven't done a deep reading on StringToDouble, but it
seems like it should work. Most (if not all) of the equivalent required calls
are there.

+      pool_descriptor, ";", base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+  int min;
+  int max;
+  double cores_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], &min) &&
+      base::StringToInt(tokens[1], &max) &&
+      base::StringToDouble(tokens[2], &cores_multiplier) &&
+      base::StringToInt(tokens[3], &offset) &&
+      base::StringToInt(tokens[4], &detach_milliseconds)) {
+    initialization::SingleWorkerPoolConfiguration config;
+    config.threads = base::RecommendedMaxNumberOfThreadsInPool(
+        min, max, cores_multiplier, offset);
+    config.detach_period =
+        base::TimeDelta::FromMilliseconds(detach_milliseconds);
+    config.standby_thread_policy = (tokens.size() >= 6 && tokens[5] == "lazy")
+                                       ? StandbyThreadPolicy::LAZY
+                                       : StandbyThreadPolicy::ONE;
+    return config;

[gab, 2016/12/06 19:16:08]

Is there a way to static_assert here that there isn't a new unhandled field in
SingleWorkerPoolConfiguration?

[robliao, 2016/12/07 00:03:54]

I figured one out last night as I drove home and was going to add it to the
default function. I'll add it here too.

+  }
+  DLOG(ERROR) << "Invalid Worker Pool Descriptor: " << pool_descriptor;
+  return initialization::SingleWorkerPoolConfiguration();
+}
+
+// Converts a browser-based |variation_params| to
+// std::vector<base::SchedulerWorkerPoolParams>. Returns an empty vector on
+// failure.
+std::vector<base::SchedulerWorkerPoolParams>
+VariationsParamsToSchedulerWorkerPoolParamsVector(
+    const std::map<std::string, std::string>& variation_params) {
+  static const char* const kWorkerPoolNames[] = {
+      "Background", "BackgroundFileIO", "Foreground", "ForegroundFileIO"};
+  static_assert(
+      arraysize(kWorkerPoolNames) == initialization::WORKER_POOL_COUNT,
+      "Mismatched Worker Pool Types and Worker Pool Names");
+  initialization::BrowserWorkerPoolsConfiguration config;
+  initialization::SingleWorkerPoolConfiguration* const all_pools[]{
+      &config.background, &config.background_file_io, &config.foreground,
+      &config.foreground_file_io,
+  };
+  static_assert(arraysize(kWorkerPoolNames) == arraysize(all_pools),
+                "Mismatched Worker Pool Names and All Pools Array");
+  for (size_t i = 0; i < arraysize(kWorkerPoolNames); ++i) {
+    const auto& worker_pool_name = kWorkerPoolNames[i];

[gab, 2016/12/06 19:16:07]

s/&/*/

(always prefer auto* when referring to a pointer)

[robliao, 2016/12/07 00:03:54]

Done.

+    const auto pair = variation_params.find(worker_pool_name);
+    if (pair == variation_params.end()) {
+      DLOG(ERROR) << "Missing Worker Pool Configuration: " << worker_pool_name;
+      return std::vector<base::SchedulerWorkerPoolParams>();
+    }
+
+    auto& pool_config = *all_pools[i];

[gab, 2016/12/06 19:16:07]

auto* pool_config = all_pools[i]

is less surprising IMO (then you can deref for assignment and -> to read
members)

[robliao, 2016/12/07 00:03:55]

Done.

+    pool_config = StringToSingleWorkerPoolConfiguration(pair->second);
+    if (pool_config.threads <= 0 || pool_config.detach_period.is_zero()) {

[gab, 2016/12/06 19:16:08]

Negative detach_period would also be invalid, right?

[robliao, 2016/12/07 00:03:55]

Done.

+      DLOG(ERROR) << "Invalid Worker Pool Configuration: " << worker_pool_name
+                  << " [" << pair->second << "]";
+      return std::vector<base::SchedulerWorkerPoolParams>();
+    }
+  }
+  return BrowserWorkerPoolConfigurationToSchedulerWorkerPoolParams(config);
+}
+
+}  // namespace
+
+std::vector<base::SchedulerWorkerPoolParams>
+GetBrowserSchedulerWorkerPoolParamsFromVariations() {
+  std::map<std::string, std::string> variation_params;
+  if (!::variations::GetVariationParams(kFieldTrialName, &variation_params))
+    return std::vector<base::SchedulerWorkerPoolParams>();
+
+  return VariationsParamsToSchedulerWorkerPoolParamsVector(variation_params);
+}
+
+void MaybePerformBrowserTaskSchedulerRedirection() {
+  std::map<std::string, std::string> variation_params;
+  ::variations::GetVariationParams(kFieldTrialName, &variation_params);
+
+  // TODO(gab): Remove this when http://crbug.com/622400 concludes.
+  const auto sequenced_worker_pool_param =
+      variation_params.find("RedirectSequencedWorkerPools");
+  if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kDisableBrowserTaskScheduler) &&
+      sequenced_worker_pool_param != variation_params.end() &&
+      sequenced_worker_pool_param->second == "true") {
+    base::SequencedWorkerPool::EnableWithRedirectionToTaskSchedulerForProcess();
+  } else {
+    base::SequencedWorkerPool::EnableForProcess();

[gab, 2016/12/06 19:16:07]

Hadn't previously noticed but it's pretty weird to have this critical call
hidden all the way down here. I guess it's fine for now as it'll go away soonish
but it's still unexpected that the whole browser won't work without this (when
no experiment is involved).

[robliao, 2016/12/07 00:03:54]

Yep, but that's also similar to task scheduler init like we had before. Either
way.

+  }
+}
+
+}  // variations
+}  // namespace task_scheduler_util