Trace-based benchmarking via a mojo app.

Trace-based benchmarking via a mojo app.

This patch adds `benchmark.mojo` app for trace-based benchmarking and a
runner script.

$ mojo/devtools/common/mojo_benchmark mojo/tools/data/benchmarks
[Trace me]
measurement: time_until/trace_me/DoWork 4488.810000
measurement: avg_duration/trace_me/DoWork 300.449714
[Spinning cube]
measurement: time_until/gpu/GpuScheduler:PutChanged 4341.874000

Fixes #371.

R=etiennej@chromium.org, jamesr@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/1d909941168487e5a0c7f81f6b7c73c026c63054

[ppi, 2015-08-21 16:53:30]

ppi@chromium.org changed reviewers:
+ jamesr@chromium.org

[ppi, 2015-08-21 16:53:30]

Hi James,

This needs tests, but could you take a first-pass look already?

[jamesr, 2015-08-21 22:56:16]

Thanks, this looks like a better setup.  Can I ask why the analysis is done in
C++?  I would think another language that has an easier time with string + json
processing would make writing the analysis easier.

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/README.md
File apps/benchmark/README.md (right):

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/README.m...
apps/benchmark/README.md:1: # benchmark
yay documentation!

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/README.m...
apps/benchmark/README.md:29: Devtools offer [a helper
script](mojo/devtools/common/mojo_benchmark) allowing
nit: "Devtools offers", perhaps?

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/benchmar...
File apps/benchmark/benchmark_app.cc (right):

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:51: // time origin trace event.
i'm not sure why this is needed - isn't the tracing time origin the same as
MojoGetTimeTicksNow() ? i don't think there is anything happening within this
app that is terribly meaningful

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:52: tracing_.Initialize(app);
you would want to do this if you wanted this app to provide tracing data to the
tracing service, but that's pretty useless here as you are the app that will be
processing the trace data.  just pass whatever data you want to collect within
this app directly to the measurement mechanism.  if you do want to provide a
unified trace file you can combine local data at the json level, but that
shouldn't be needed.

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:148: fflush(NULL);
use 'nullptr', this is c++ code

[ppi, 2015-08-24 15:41:05]

Thanks!

As for the language, I erred on the side of flexibility - Python apps don't run
on Android, and for Dart afaiu the  packaging is in a flux (so we would be able
to run the app from a local host, but not from the web). Wdyt?

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/README.md
File apps/benchmark/README.md (right):

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/README.m...
apps/benchmark/README.md:1: # benchmark
On 2015/08/21 22:56:16, jamesr wrote:
> yay documentation!

Concurred:).

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/README.m...
apps/benchmark/README.md:29: Devtools offer [a helper
script](mojo/devtools/common/mojo_benchmark) allowing
On 2015/08/21 22:56:16, jamesr wrote:
> nit: "Devtools offers", perhaps?

Done.

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/benchmar...
File apps/benchmark/benchmark_app.cc (right):

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:51: // time origin trace event.
On 2015/08/21 22:56:16, jamesr wrote:
> i'm not sure why this is needed - isn't the tracing time origin the same as
> MojoGetTimeTicksNow() ? i don't think there is anything happening within this
> app that is terribly meaningful

Done.

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:52: tracing_.Initialize(app);
On 2015/08/21 22:56:16, jamesr wrote:
> you would want to do this if you wanted this app to provide tracing data to
the
> tracing service, but that's pretty useless here as you are the app that will
be
> processing the trace data.  just pass whatever data you want to collect within
> this app directly to the measurement mechanism.  if you do want to provide a
> unified trace file you can combine local data at the json level, but that
> shouldn't be needed.

Done.

https://codereview.chromium.org/1305193002/diff/20001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:148: fflush(NULL);
On 2015/08/21 22:56:16, jamesr wrote:
> use 'nullptr', this is c++ code

Done.

[ppi, 2015-08-27 14:30:08]

Added tests, ptal.

[jamesr, 2015-08-27 21:12:38]

If you want to use C++ that's OK, but it is pretty harsh about unexpected input
which makes it trickier to write (see comments below).  I imagine we'll want to
build a mojom interface for plugging in different analysis to separate that from
the collection part of the benchmark at which point it probably doesn't make too
much of a difference which language the collection portion is written in.

Comments below are mostly about being more robust.  Feel free to get a local
review once you've addressed those and land, or I can take another look if
you're OK waiting for the time zone delay.  Overall this lgtm

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/args.cc
File apps/benchmark/args.cc (right):

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/args.cc#...
apps/benchmark/args.cc:21: auto it = std::find_if(args.begin(), args.end(),
matches);
is there a particular reason to avoid base's command line processing logic here?
this file depends on //base already

doing a full scan through the args list for each lookup will be inefficient if
we have a lot of args. could you parse into a set or map at startup? how general
do you want this to be - i.e. how many things do you expect to be configured via
args?

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/args.cc#...
apps/benchmark/args.cc:70: result->duration =
base::TimeDelta::FromSeconds(std::stoi(duration_str));
what if duration_str doesn't contain a valid int? std::stoi will throw an
exception (crash in our world) if it's not valid. base:: has string->number
converters that do something more reasonable on error, or you could try to
validate if you want to avoid the base code

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/args.cc#...
apps/benchmark/args.cc:80: if (arg.substr(0, 2) == "--") {
i believe this crashes if the arg length is 0 or 1 chars, which is probably not
what you want. base:: has a StartsWith
(https://github.com/domokit/mojo/blob/master/base/strings/string_util.h#L328) or
if you want to avoid using that be sure to check the length

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/benchmar...
File apps/benchmark/benchmark_app.cc (right):

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:40: mojo::ApplicationImpl::Terminate();
maybe let the user know that the arguments didn't parse cleanly?

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:52: std::string categories_str =
JoinString(unique_categories, ',');
hmm, this uses base::'s string utilities (which seems fine IMO) so why doesn't
the rest?

[ppi, 2015-08-28 12:15:11]

ppi@chromium.org changed reviewers:
+ etiennej@chromium.org

[ppi, 2015-08-28 12:15:11]

Thanks, James!

+Etienne for another and local pair of eyes.

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/args.cc
File apps/benchmark/args.cc (right):

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/args.cc#...
apps/benchmark/args.cc:21: auto it = std::find_if(args.begin(), args.end(),
matches);
On 2015/08/27 21:12:38, jamesr wrote:
> is there a particular reason to avoid base's command line processing logic
here?
> this file depends on //base already
> 
> doing a full scan through the args list for each lookup will be inefficient if
> we have a lot of args. could you parse into a set or map at startup? how
general
> do you want this to be - i.e. how many things do you expect to be configured
via
> args?

Done and done (switched to base::command_line). Don't know if we grow further
switches, but the code is nicer this way in any case.

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/args.cc#...
apps/benchmark/args.cc:70: result->duration =
base::TimeDelta::FromSeconds(std::stoi(duration_str));
On 2015/08/27 21:12:38, jamesr wrote:
> what if duration_str doesn't contain a valid int? std::stoi will throw an
> exception (crash in our world) if it's not valid. base:: has string->number
> converters that do something more reasonable on error, or you could try to
> validate if you want to avoid the base code

Done (used the converter in base).

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/args.cc#...
apps/benchmark/args.cc:80: if (arg.substr(0, 2) == "--") {
On 2015/08/27 21:12:38, jamesr wrote:
> i believe this crashes if the arg length is 0 or 1 chars, which is probably
not
> what you want. base:: has a StartsWith
> (https://github.com/domokit/mojo/blob/master/base/strings/string_util.h#L328)
or
> if you want to avoid using that be sure to check the length

Done (n/a with switch to base::command_line).

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/benchmar...
File apps/benchmark/benchmark_app.cc (right):

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:40: mojo::ApplicationImpl::Terminate();
On 2015/08/27 21:12:38, jamesr wrote:
> maybe let the user know that the arguments didn't parse cleanly?

Done.

https://codereview.chromium.org/1305193002/diff/60001/apps/benchmark/benchmar...
apps/benchmark/benchmark_app.cc:52: std::string categories_str =
JoinString(unique_categories, ',');
On 2015/08/27 21:12:38, jamesr wrote:
> hmm, this uses base::'s string utilities (which seems fine IMO) so why doesn't
> the rest?

Done.

[etiennej, 2015-08-28 13:54:37]

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/args.h
File apps/benchmark/args.h (right):

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/args.h#n...
apps/benchmark/args.h:17: struct Args {
I would call it BenchmarkRunParameters.

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/args.h#n...
apps/benchmark/args.h:22: Args();
I don't think you need a constructor or a destructor for a struct.

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/event.h
File apps/benchmark/event.h (right):

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/event.h#...
apps/benchmark/event.h:16: struct Event {
Documentation?

[ppi, 2015-08-28 14:02:06]

Thanks, Etienne, ptal.

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/args.h
File apps/benchmark/args.h (right):

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/args.h#n...
apps/benchmark/args.h:17: struct Args {
On 2015/08/28 13:54:37, etiennej wrote:
> I would call it BenchmarkRunParameters.

We are already in the namespace benchmark. I can switch Args to Parameters, but
why go for the longer?

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/args.h#n...
apps/benchmark/args.h:22: Args();
On 2015/08/28 13:54:37, etiennej wrote:
> I don't think you need a constructor or a destructor for a struct.

You need to explicitly declare it and define out-of-line to prevent inlining.

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/event.h
File apps/benchmark/event.h (right):

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/event.h#...
apps/benchmark/event.h:16: struct Event {
On 2015/08/28 13:54:37, etiennej wrote:
> Documentation?

Done. Plmk if you think more is needed.

[etiennej, 2015-08-28 14:17:11]

lgtm

[ppi, 2015-08-28 14:17:13]

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/args.h
File apps/benchmark/args.h (right):

https://codereview.chromium.org/1305193002/diff/80001/apps/benchmark/args.h#n...
apps/benchmark/args.h:17: struct Args {
On 2015/08/28 14:02:06, ppi wrote:
> On 2015/08/28 13:54:37, etiennej wrote:
> > I would call it BenchmarkRunParameters.
> 
> We are already in the namespace benchmark. I can switch Args to Parameters,
but
> why go for the longer?

Changed to RunArgs as discussed offline.

[ppi, 2015-08-28 14:28:00]

Committed patchset #7 (id:120001) manually as
1d909941168487e5a0c7f81f6b7c73c026c63054.