PerfProvider: Get collection parameters from Finch

chrome/browser/metrics/perf/perf_provider_chromeos.cc

 // Copyright (c) 2012 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 "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.
 const size_t kCachedPerfDataProtobufSizeThreshold = 4 * 1024 * 1024;
 
 // This is used to space out session restore collections in the face of several
 // notifications in a short period of time. There should be no less than this
 // much time between collections.
 const int kMinIntervalBetweenSessionRestoreCollectionsInSec = 30;
 
@@ skipping 141 matching lines @@
       /* max_collection_delay = */ base::TimeDelta::FromSeconds(5)),
   /* restore_session = */ PerfProvider::CollectionParams::TriggerParams(
       /* sampling_factor = */ 10,
       /* max_collection_delay = */ base::TimeDelta::FromSeconds(10)));
 
 PerfProvider::PerfProvider()
     : collection_params_(kDefaultParameters),
       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_);
 
   // Register as an observer of power manager events.
   chromeos::DBusThreadManager::Get()->GetPowerManagerClient()->
       AddObserver(this);
 
   // Register as an observer of session restore.
   on_session_restored_callback_subscription_ =
       SessionRestore::RegisterOnSessionRestoredCallback(
           base::Bind(&PerfProvider::OnSessionRestoreDone,
                      weak_factory_.GetWeakPtr()));
 
   // Check the login state. At the time of writing, this class is instantiated
   // before login. A subsequent login would activate the profiling. However,
   // that behavior may change in the future so that the user is already logged
   // when this class is instantiated. By calling LoggedInStateChanged() here,
   // PerfProvider will recognize that the system is already logged in.
   login_observer_.LoggedInStateChanged();
 }
 
 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());
   if (cached_perf_data_.empty()) {
     AddToPerfHistogram(NOT_READY_TO_UPLOAD);
     return false;
   }
 
   sampled_profiles->swap(cached_perf_data_);
   cached_perf_data_.clear();
@@ skipping 237 matching lines @@
   scoped_ptr<SampledProfile> sampled_profile(new SampledProfile);
   sampled_profile->set_trigger_event(SampledProfile::RESTORE_SESSION);
   sampled_profile->set_ms_after_restore(time_after_restore.InMilliseconds());
   sampled_profile->set_num_tabs_restored(num_tabs_restored);
 
   CollectIfNecessary(sampled_profile.Pass());
   last_session_restore_collection_time_ = base::TimeTicks::Now();
 }
 
 }  // namespace metrics