PerfProvider: Get collection parameters from Finch

chrome/browser/metrics/perf/perf_provider_chromeos.cc

Index: chrome/browser/metrics/perf/perf_provider_chromeos.cc
diff --git a/chrome/browser/metrics/perf/perf_provider_chromeos.cc b/chrome/browser/metrics/perf/perf_provider_chromeos.cc
index bcd88348cb7f6fd462e5d6ab5c1317b3cf5b91aa..d4ffba30a0012a315b5540f2275de5b9c5f950e6 100644
--- a/chrome/browser/metrics/perf/perf_provider_chromeos.cc
+++ b/chrome/browser/metrics/perf/perf_provider_chromeos.cc
@@ -5,24 +5,30 @@
 #include "chrome/browser/metrics/perf/perf_provider_chromeos.h"
 
 #include <algorithm>
+#include <map>
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "chrome/browser/metrics/perf/windowed_incognito_observer.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
+#include "components/variations/variations_associated_data.h"
 
 namespace metrics {
 
 namespace {
 
+const char kCWPFieldTrialName[] = "ChromeOSWideProfilingCollection";
+
 // Limit the total size of protobufs that can be cached, so they don't take up
 // too much memory. If the size of cached protobufs exceeds this value, stop
 // collecting further perf data. The current value is 4 MB.
@@ -184,8 +190,11 @@ PerfProvider::PerfProvider()
       login_observer_(this),
       next_profiling_interval_start_(base::TimeTicks::Now()),
       weak_factory_(this) {
-  command_selector_.SetOdds(
-      internal::GetDefaultCommandsForCpu(GetCPUIdentity())),
+  CHECK(command_selector_.SetOdds(
+      internal::GetDefaultCommandsForCpu(GetCPUIdentity())));
+  std::map<std::string, std::string> params;
+  if (variations::GetVariationParams(kCWPFieldTrialName, &params))
+    SetCollectionParamsFromVariationParams(params);
 
   // Register the login observer with LoginState.
   chromeos::LoginState::Get()->AddObserver(&login_observer_);
@@ -212,6 +221,142 @@ PerfProvider::~PerfProvider() {
   chromeos::LoginState::Get()->RemoveObserver(&login_observer_);
 }
 
+namespace {

[Alexei Svitkine (slow), 2015/10/16 15:37:50]

Put these in the existing anon namespaces.

[dhsharp, 2015/10/19 22:45:28]

I'd prefer to keep these near where they are used.

[Alexei Svitkine (slow), 2015/10/20 20:24:58]

In Chromium, the convention is to have a single anon namespace at the top of the
file, however.

[dhsharp, 2015/10/21 02:17:02]

Well, okay. Done.

But you might want to tell these 414 files that (I realize it's a small fraction
of Chromium files):
https://code.google.com/p/chromium/codesearch#search/&q=pcre:yes%20(?m:namesp...

+
+bool GetInt64Param(const std::map<std::string, std::string>& params,
+                   const std::string& key, int64* out) {
+  auto it = params.find(key);
+  if (it == params.end())
+    return false;
+  int64 value;
+  // NB: StringToInt64 will set value even if the conversion fails.
+  if (!base::StringToInt64(it->second, &value))
+    return false;
+  *out = value;
+  return true;
+}
+
+// Parse the key. e.g.: "PerfCommand::arm::0" returns "arm"
+bool ExtractPerfCommandCpuSpecifier(const std::string& key,
+                                    std::string* cpu_specifier) {
+  std::vector<std::string> tokens;
+  base::SplitStringUsingSubstr(key, "::", &tokens);
+  if (tokens.size() != 3)
+    return false;
+  if (tokens[0] != "PerfCommand")
+    return false;
+  *cpu_specifier = tokens[1];
+  // tokens[2] is just a unique string (usually an index).
+  return true;
+}
+
+}  // namespace
+
+namespace internal {
+
+std::string FindBestCpuSpecifierFromParams(
+    const std::map<std::string, std::string>& params,
+    const CPUIdentity& cpuid) {
+  std::string ret;
+  // The CPU specified in the Finch params could be "default", a system

[Alexei Svitkine (slow), 2015/10/16 15:37:50]

Nit: Finch is an internal codename. Use "Variations" or "FieldTrial" in open
source code. Also, update CL description.

[dhsharp, 2015/10/19 22:45:28]

Done.

+  // architecture, a CPU microarchitecture, or a CPU model substring. We should
+  // prefer to match the most specific.
+  enum MatchSpecificity {
+    NO_MATCH,
+    DEFAULT,
+    SYSTEM_ARCH,
+    CPU_UARCH,
+    CPU_MODEL,
+  };
+  MatchSpecificity match_level = NO_MATCH;
+
+  const std::string intel_uarch = GetIntelUarch(cpuid);
+  const std::string simplified_cpu_model =
+      SimplifyCPUModelName(cpuid.model_name);
+
+  for (const auto& key_val : params) {
+    const std::string& key = key_val.first;
+
+    std::string cpu_specifier;
+    if (!ExtractPerfCommandCpuSpecifier(key, &cpu_specifier))
+      continue;
+
+    if (match_level < DEFAULT && cpu_specifier == "default") {
+      match_level = DEFAULT;
+      ret = cpu_specifier;
+    }
+    if (match_level < SYSTEM_ARCH && cpu_specifier == cpuid.arch) {
+      match_level = SYSTEM_ARCH;
+      ret = cpu_specifier;
+    }
+    if (match_level < CPU_UARCH &&
+        intel_uarch != "" && cpu_specifier == intel_uarch) {
+      match_level = CPU_UARCH;
+      ret = cpu_specifier;
+    }
+    if (match_level < CPU_MODEL &&
+        simplified_cpu_model.find(cpu_specifier) != std::string::npos) {
+      match_level = CPU_MODEL;
+      ret = cpu_specifier;
+    }
+  }
+  return ret;
+}
+
+}  // namespace internal
+
+void PerfProvider::SetCollectionParamsFromVariationParams(
+    const std::map<std::string, std::string>& params) {
+  int64 value;
+  if (GetInt64Param(params, "ProfileCollectionDurationSec", &value))

[Alexei Svitkine (slow), 2015/10/16 15:37:50]

Nit: {}'s for multi-line bodies.

[dhsharp, 2015/10/19 22:45:28]

Done.

+    collection_params_.set_collection_duration(
+        base::TimeDelta::FromSeconds(value));
+  if (GetInt64Param(params, "PeriodicProfilingIntervalMs", &value))
+    collection_params_.set_periodic_interval(
+        base::TimeDelta::FromMilliseconds(value));
+  if (GetInt64Param(params, "ResumeFromSuspend::SamplingFactor", &value))
+    collection_params_.mutable_resume_from_suspend()
+        ->set_sampling_factor(value);
+  if (GetInt64Param(params, "ResumeFromSuspend::MaxCollectionDelaySec", &value))
+    collection_params_.mutable_resume_from_suspend()->set_max_collection_delay(
+        base::TimeDelta::FromSeconds(value));
+  if (GetInt64Param(params, "RestoreSession::SamplingFactor", &value))
+    collection_params_.mutable_restore_session()->set_sampling_factor(value);
+  if (GetInt64Param(params, "RestoreSession::MaxCollectionDelaySec", &value))

[Alexei Svitkine (slow), 2015/10/16 15:37:50]

Nit: Is it possible to use shorter names for these params? They are part of the
payload and every byte adds up.

[dhsharp, 2015/10/19 22:45:28]

It's certainly possible, heh. I guess it's a tradeoff between length and
readability. Do you have suggestions? I'm not exactly thrilled with the name
"MaxCollectionDelaySec" either. The parameter it controls is the upper bound of
the the randomly selected delay between the trigger and collection. I could omit
"Collection", but "MaxDelaySec" leaves me wondering "delay of what?".

I've done that for now. Let me know if you have better ideas.

[Alexei Svitkine (slow), 2015/10/20 20:24:58]

Another option is to have the string names themselves be more cryptic but to
have string constants at the top of the file that spell them out more. Though,
I'm ok with current names too, I guess.

+    collection_params_.mutable_restore_session()->set_max_collection_delay(
+        base::TimeDelta::FromSeconds(value));
+
+  const std::string best_cpu_specifier =
+      internal::FindBestCpuSpecifierFromParams(params, GetCPUIdentity());
+
+  if (best_cpu_specifier.empty())  // no matching cpu specifier. Keep defaults.
+    return;
+
+  std::vector<RandomSelector::WeightAndValue> commands;
+  for (const auto& key_val : params) {
+    const std::string& key = key_val.first;
+    const std::string& val = key_val.second;
+
+    std::string cpu_specifier;
+    if (!ExtractPerfCommandCpuSpecifier(key, &cpu_specifier))
+      continue;
+    if (cpu_specifier != best_cpu_specifier)
+      continue;
+
+    auto split = val.find(" ");
+    if (split == std::string::npos)
+      continue;  // Just drop invalid commands.
+    std::string weight_str = std::string(val.begin(), val.begin() + split);
+
+    double weight;
+    if (!(base::StringToDouble(weight_str, &weight) && weight > 0.0))

[Alexei Svitkine (slow), 2015/10/16 15:37:50]

Nit: !(a && b) -> !a || !b

[dhsharp, 2015/10/19 22:45:28]

I could apply DeMorgan's laws. However:
- I prefer to make positive assertions about required conditions.
- I have to use "weight > 0.0", because the inverse condition, "weight <= 0.0",
would not reject NaN since it would evaluate false (I have test coverage for
this). I think "!(weight > 0.0)" is uglier.

+      continue;  // Just drop invalid commands.
+    std::string command(val.begin() + split + 1, val.end());
+    commands.push_back(RandomSelector::WeightAndValue(weight, command));
+  }
+  command_selector_.SetOdds(commands);
+}
+
 bool PerfProvider::GetSampledProfiles(
     std::vector<SampledProfile>* sampled_profiles) {
   DCHECK(CalledOnValidThread());