PerfProvider: Run a randomly selected perf command

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 fb496d59dc1a04d20aaa8ce1854562aec364b9ce..0d425d868d79165a47e25eae71db183b3a47ef1b 100644
--- a/chrome/browser/metrics/perf/perf_provider_chromeos.cc
+++ b/chrome/browser/metrics/perf/perf_provider_chromeos.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/metrics/perf/perf_provider_chromeos.h"
 
+#include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ -11,6 +12,7 @@
 #include "base/compiler_specific.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.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"
@@ -19,6 +21,85 @@
 
 namespace metrics {
 
+namespace internal {

[Alexei Svitkine (slow), 2015/10/13 17:31:19]

Nit: Move this below the anon namespace.

[dhsharp, 2015/10/13 23:36:22]

Done.

+
+// Hopefully we never need a space in a command argument.
+const char* kPerfCommandDelimiter = " ";

[Alexei Svitkine (slow), 2015/10/13 17:31:19]

Nit: const char kPerfCommandDelimiter[] =

(Preferred style for Chromium and generates more efficient machine code.)

Please fix throughout.

[dhsharp, 2015/10/13 23:36:22]

Done.

+
+const char* kPerfRecordCyclesCmd =

[Alexei Svitkine (slow), 2015/10/13 17:31:19]

These should just be in the anon namespace.

Only put things in internal:: if they need to be exposed outside of the file.

[dhsharp, 2015/10/13 23:36:22]

Done.

+  "perf record -a -e cycles -c 1000003";
+
+const char* kPerfRecordCallgraphCmd =
+  "perf record -a -e cycles -g -c 4000037";
+
+const char* kPerfRecordLBRCmd =
+  "perf record -a -e r2c4 -b -c 20011";
+
+const char* kPerfRecordInstructionTLBMissesCmd =
+  "perf record -a -e iTLB-misses -c 2003";
+
+const char* kPerfRecordDataTLBMissesCmd =
+  "perf record -a -e dTLB-misses -c 2003";
+
+const char* kPerfStatMemoryBandwidthCmd =
+  "perf stat -a -e cycles -e instructions "
+  "-e uncore_imc/data_reads/ -e uncore_imc/data_writes/ "
+  "-e cpu/event=0xD0,umask=0x11,name=MEM_UOPS_RETIRED-STLB_MISS_LOADS/ "
+  "-e cpu/event=0xD0,umask=0x12,name=MEM_UOPS_RETIRED-STLB_MISS_STORES/";
+
+const std::vector<RandomSelector::WeightAndValue> GetDefaultCommands_x86_64(
+    const CPUIdentity& cpuid) {
+  using WeightAndValue = RandomSelector::WeightAndValue;

[Alexei Svitkine (slow), 2015/10/13 17:31:19]

Nit: Inline using declarations are not recommended.

[dhsharp, 2015/10/13 23:36:22]

This is a type alias, not a using-declaration. It's the same as a typedef, just
a little easier to read (and it can be templated, but I'm not using that here).
It's an allowed C++11 feature.

(Also: "You may use a using-declaration anywhere in a .cc file, and in
functions, methods or classes in .h files.")

[Alexei Svitkine (slow), 2015/10/14 15:53:58]

Fair enough. Metrics code generally avoids these, but I guess this use is
harmless enough. :)

+  std::vector<WeightAndValue> cmds;
+  DCHECK(cpuid.arch == "x86_64");

[Alexei Svitkine (slow), 2015/10/13 17:31:19]

Nit: DCHECK_EQ

Also, please make file-level constants for these.

[dhsharp, 2015/10/13 23:36:22]

Done.
I don't see the point of that when they serve as their own identifier.

[Alexei Svitkine (slow), 2015/10/14 15:53:58]

Well, my reasoning was that you use it in two places. But I guess given the
second is a DCHECK based on the first, in this particular case a typo would
actually cause it to trigger, which should be good enough to leave as-is, as you
say.

+  const std::string intel_uarch = GetIntelUarch(cpuid);
+  if (intel_uarch == "IvyBridge" ||
+      intel_uarch == "Haswell" ||
+      intel_uarch == "Broadwell") {
+    cmds.push_back(WeightAndValue(60.0, kPerfRecordCyclesCmd));
+    cmds.push_back(WeightAndValue(20.0, kPerfRecordCallgraphCmd));
+    cmds.push_back(WeightAndValue(5.0, kPerfRecordLBRCmd));
+    cmds.push_back(WeightAndValue(5.0, kPerfRecordInstructionTLBMissesCmd));
+    cmds.push_back(WeightAndValue(5.0, kPerfRecordDataTLBMissesCmd));
+    cmds.push_back(WeightAndValue(5.0, kPerfStatMemoryBandwidthCmd));
+    return cmds;
+  }
+  if (intel_uarch == "SandyBridge") {
+    cmds.push_back(WeightAndValue(65.0, kPerfRecordCyclesCmd));
+    cmds.push_back(WeightAndValue(20.0, kPerfRecordCallgraphCmd));
+    cmds.push_back(WeightAndValue(5.0, kPerfRecordLBRCmd));
+    cmds.push_back(WeightAndValue(5.0, kPerfRecordInstructionTLBMissesCmd));
+    cmds.push_back(WeightAndValue(5.0, kPerfRecordDataTLBMissesCmd));
+    return cmds;
+  }
+  // Other 64-bit x86
+  cmds.push_back(WeightAndValue(70.0, kPerfRecordCyclesCmd));
+  cmds.push_back(WeightAndValue(20.0, kPerfRecordCallgraphCmd));
+  cmds.push_back(WeightAndValue(5.0, kPerfRecordInstructionTLBMissesCmd));
+  cmds.push_back(WeightAndValue(5.0, kPerfRecordDataTLBMissesCmd));
+  return cmds;
+}
+
+const std::vector<RandomSelector::WeightAndValue> GetDefaultCommandsForCpu(
+    const CPUIdentity& cpuid) {
+  using WeightAndValue = RandomSelector::WeightAndValue;
+  std::vector<WeightAndValue> cmds;
+  if (cpuid.arch == "x86" ||     // 32-bit x86, or...
+      cpuid.arch == "armv7l") {  // ARM
+    cmds.push_back(WeightAndValue(80.0, kPerfRecordCyclesCmd));
+    cmds.push_back(WeightAndValue(20.0, kPerfRecordCallgraphCmd));
+    return cmds;
+  }
+  if (cpuid.arch == "x86_64") {  // 64-bit x86

[Alexei Svitkine (slow), 2015/10/13 17:31:19]

Nit: No {}'s

[dhsharp, 2015/10/13 23:36:22]

Done.

+    return GetDefaultCommands_x86_64(cpuid);
+  }
+  // Unknown CPUs
+  cmds.push_back(WeightAndValue(1.0, kPerfRecordCyclesCmd));
+  return cmds;
+}
+
+}  // namespace internal
+
 namespace {
 
 // Limit the total size of protobufs that can be cached, so they don't take up
@@ -101,6 +182,9 @@ PerfProvider::PerfProvider()
       login_observer_(this),
       next_profiling_interval_start_(base::TimeTicks::Now()),
       weak_factory_(this) {
+  command_selector_.SetOdds(
+      internal::GetDefaultCommandsForCpu(GetCPUIdentity())),
+
   // Register the login observer with LoginState.
   chromeos::LoginState::Get()->AddObserver(&login_observer_);
 
@@ -340,8 +424,11 @@ void PerfProvider::CollectIfNecessary(
   chromeos::DebugDaemonClient* client =
       chromeos::DBusThreadManager::Get()->GetDebugDaemonClient();
 
+  std::vector<std::string> command = base::SplitString(
+      command_selector_.Select(), internal::kPerfCommandDelimiter,
+      base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
   client->GetPerfOutput(
-      collection_params_.collection_duration().InSeconds(),
+      collection_params_.collection_duration().InSeconds(), command,
       base::Bind(&PerfProvider::ParseOutputProtoIfValid,
                  weak_factory_.GetWeakPtr(), base::Passed(&incognito_observer),
                  base::Passed(&sampled_profile)));