PerfProvider: Run a randomly selected perf command

PerfProvider: Run a randomly selected perf command

Based on the CPU, select a set of perf commands to run, and put these in
a RandomSelector to choose one command at random in each instance. Call
the dbus method org.chromium.debugd.GetPerfOutput to run perf with the
command. The command set used is based on the system architecture, CPU
microarchitecture, or CPU model string.

The command set selection logic is the same as that currently used by
GetRandomPerfOutput in debugd. In the near future, this work will allow
overriding the command set used using FieldTrials and Finch.

BUG=538759
Committed: https://crrev.com/659ffbe6f46382e968a38fb336f371be737f3301
Cr-Commit-Position: refs/heads/master@{#354073}

[dhsharp, 2015-10-09 23:08:53]

dhsharp@chromium.org changed reviewers:
+ asvitkine@chromium.org, isherman@chromium.org

[dhsharp, 2015-10-09 23:08:53]

Adding Ilya this time too to spread the load, and since Monday is a holiday for
Alexei.

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

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:24: namespace internal {
Nit: Move this below the anon namespace.

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:27: const char*
kPerfCommandDelimiter = " ";
Nit: const char kPerfCommandDelimiter[] =

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

Please fix throughout.

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:29: const char*
kPerfRecordCyclesCmd =
These should just be in the anon namespace.

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

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:52: using WeightAndValue =
RandomSelector::WeightAndValue;
Nit: Inline using declarations are not recommended.

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:54: DCHECK(cpuid.arch ==
"x86_64");
Nit: DCHECK_EQ

Also, please make file-level constants for these.

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:93: if (cpuid.arch ==
"x86_64") {  // 64-bit x86
Nit: No {}'s

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
File chrome/browser/metrics/perf/perf_provider_chromeos.h (right):

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.h:239: const
std::vector<RandomSelector::WeightAndValue> GetDefaultCommandsForCpu(
Nit: No need for const on the return value

[dhsharp, 2015-10-13 23:36:23]

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:24: namespace internal {
On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> Nit: Move this below the anon namespace.

Done.

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:27: const char*
kPerfCommandDelimiter = " ";
On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> Nit: const char kPerfCommandDelimiter[] =
> 
> (Preferred style for Chromium and generates more efficient machine code.)
> 
> Please fix throughout.

Done.

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:29: const char*
kPerfRecordCyclesCmd =
On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> These should just be in the anon namespace.
> 
> Only put things in internal:: if they need to be exposed outside of the file.

Done.

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:52: using WeightAndValue =
RandomSelector::WeightAndValue;
On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> Nit: Inline using declarations are not recommended.

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.")

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:54: DCHECK(cpuid.arch ==
"x86_64");
On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> Nit: DCHECK_EQ

Done.

> Also, please make file-level constants for these.

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

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:93: if (cpuid.arch ==
"x86_64") {  // 64-bit x86
On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> Nit: No {}'s

Done.

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
File chrome/browser/metrics/perf/perf_provider_chromeos.h (right):

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.h:239: const
std::vector<RandomSelector::WeightAndValue> GetDefaultCommandsForCpu(
On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> Nit: No need for const on the return value

Done.

[Alexei Svitkine (slow), 2015-10-14 15:53:59]

LGTM % remaining comments

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:52: using WeightAndValue =
RandomSelector::WeightAndValue;
On 2015/10/13 23:36:22, dhsharp wrote:
> On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> > Nit: Inline using declarations are not recommended.
> 
> 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.")

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

https://codereview.chromium.org/1395273002/diff/1/chrome/browser/metrics/perf...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:54: DCHECK(cpuid.arch ==
"x86_64");
On 2015/10/13 23:36:22, dhsharp wrote:
> On 2015/10/13 17:31:19, Alexei Svitkine(OOO til Oct13) wrote:
> > Nit: DCHECK_EQ
> 
> Done.
> 
> > Also, please make file-level constants for these.
> 
> I don't see the point of that when they serve as their own identifier.

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.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:142: if (cpuid.arch ==
"x86_64")  // 64-bit x86
Nit: Make this the first check of the function. That way, all returns following
declaration of |cmds| actually use it.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.h (right):

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.h:236: // Exposed for unit
testing.
Nit: Put above the namespace.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.h:238:
std::vector<RandomSelector::WeightAndValue> GetDefaultCommandsForCpu(
Add a comment describing the function.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc (right):

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:28: const char*
kPerfRecordCyclesCmd =
Nit:

const char kPerfRecordCyclesCmd[] =

Same for the other ones.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:573:
std::vector<RandomSelector::WeightAndValue> cmds;
Nit: Merge with the assignment on line 580.

Do this for the other tests too.

[dhsharp, 2015-10-14 18:07:04]

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.cc (right):

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.cc:142: if (cpuid.arch ==
"x86_64")  // 64-bit x86
On 2015/10/14 15:53:58, Alexei Svitkine wrote:
> Nit: Make this the first check of the function. That way, all returns
following
> declaration of |cmds| actually use it.

Done.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos.h (right):

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.h:236: // Exposed for unit
testing.
On 2015/10/14 15:53:58, Alexei Svitkine wrote:
> Nit: Put above the namespace.

Done.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos.h:238:
std::vector<RandomSelector::WeightAndValue> GetDefaultCommandsForCpu(
On 2015/10/14 15:53:58, Alexei Svitkine wrote:
> Add a comment describing the function.

Done.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc (right):

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:28: const char*
kPerfRecordCyclesCmd =
On 2015/10/14 15:53:58, Alexei Svitkine wrote:
> Nit:
> 
> const char kPerfRecordCyclesCmd[] =
> 
> Same for the other ones.

Sorry, missed it. Done.

https://codereview.chromium.org/1395273002/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/perf/perf_provider_chromeos_unittest.cc:573:
std::vector<RandomSelector::WeightAndValue> cmds;
On 2015/10/14 15:53:58, Alexei Svitkine wrote:
> Nit: Merge with the assignment on line 580.
> 
> Do this for the other tests too.

Done.

[dhsharp, 2015-10-14 18:08:42]

The CQ bit was checked by dhsharp@chromium.org

[dhsharp, 2015-10-14 18:08:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1395273002/#ps40001
(title: "Address comments on PS2")

[commit-bot: I haz the power, 2015-10-14 18:09:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1395273002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1395273002/40001

[commit-bot: I haz the power, 2015-10-14 18:36:30]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-14 18:38:04]

Patchset 3 (id:??) landed as
https://crrev.com/659ffbe6f46382e968a38fb336f371be737f3301
Cr-Commit-Position: refs/heads/master@{#354073}