Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1391013005:
Benchmark: `--save-all-traces` argument (Closed)
DescriptionBenchmark: `--save-all-traces` argument
Benchmark: Replace `--save-trace` argument by `--save-all-traces`. It outputs a .trace file per benchmark as before, but stores all traces, without filtering by the measurements defined in the benchmarks file. fixes #458
BUG=
R=etiennej@chromium.org, ppi@chromium.org, qsr@chromium.org
Committed: https://chromium.googlesource.com/external/mojo/+/ebcc63fd341a027acd15b38924779bd63b9bc9a2
Patch Set 1 #Patch Set 2 : merge #
Total comments: 2
Patch Set 3 : Adding --trace-all argument #Patch Set 4 : on benchmarking: adding --trace-all argument #Patch Set 5 : on benchmarking: adding "--trace-all" argument #Patch Set 6 : benchmark #
Total comments: 11
Patch Set 7 : benchmark #Patch Set 8 : benchmark #Patch Set 9 : Benchmark #
Total comments: 4
Patch Set 10 : Benchmark #
Messages
Total messages: 33 (8 generated)
nellyv@chromium.org changed reviewers: + etiennej@chromium.org
etiennej@chromium.org changed reviewers: + qsr@chromium.org
Adding qsr@ as a reviewer; otherwise, lgtm
lgtm
ppi@chromium.org changed reviewers: + ppi@chromium.org
https://codereview.chromium.org/1391013005/diff/20001/apps/benchmark/benchmar... File apps/benchmark/benchmark_app.cc (right): https://codereview.chromium.org/1391013005/diff/20001/apps/benchmark/benchmar... apps/benchmark/benchmark_app.cc:50: if (!args_.write_output_file) { I'd argue that we should record all categories indiscriminately. --save_traces should continue to save exactly the same trace that is being processed in a benchmark run, otherwise it won't be that useful in debugging benchmarking issues.
https://codereview.chromium.org/1391013005/diff/20001/apps/benchmark/benchmar... File apps/benchmark/benchmark_app.cc (left): https://codereview.chromium.org/1391013005/diff/20001/apps/benchmark/benchmar... apps/benchmark/benchmark_app.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. nits about the commit message: please break the first line at 80 characters please make the first line an imperative statement: "trace all events ..." rather than "tracing all events ..."
On 2015/10/12 19:30:52, ppi wrote: > https://codereview.chromium.org/1391013005/diff/20001/apps/benchmark/benchmar... > File apps/benchmark/benchmark_app.cc (right): > > https://codereview.chromium.org/1391013005/diff/20001/apps/benchmark/benchmar... > apps/benchmark/benchmark_app.cc:50: if (!args_.write_output_file) { > I'd argue that we should record all categories indiscriminately. --save_traces > should continue to save exactly the same trace that is being processed in a > benchmark run, otherwise it won't be that useful in debugging benchmarking > issues. I'm not sure to understand this. My understanding is that the benchmark code will ignore all the other events, so capturing those when not saving the trace to disk is not useful. What use case are you thinking about?
Consider the following scenario: one of the tracing provider impls is broken and doesn't handle correctly the category filter. Or we add a new measurement type and it doesn't seem to match the events as expected. To debug either of these issues we want to see exactly the events that the benchmarking app gets to work with. We don't want the fact of observing this data magically make it different. If restricting the category filter is useful, we can put it behind a separate flag (`--filter-categories`?), but I wouldn't tie it to `--save-traces`.
On 2015/10/13 16:58:23, ppi wrote: > Consider the following scenario: one of the tracing provider impls is broken and > doesn't handle correctly the category filter. Or we add a new measurement type > and it doesn't seem to match the events as expected. > > To debug either of these issues we want to see exactly the events that the > benchmarking app gets to work with. We don't want the fact of observing this > data magically make it different. > > If restricting the category filter is useful, we can put it behind a separate > flag (`--filter-categories`?), but I wouldn't tie it to `--save-traces`. Should we then instead filter by category event in the benchmark app, so that we detect those broken provider? And then it doesn't matter. Having the overhead of asking all the categories all the time doesn't seem optional.
On 2015/10/15 08:04:26, qsr wrote: > On 2015/10/13 16:58:23, ppi wrote: > > Consider the following scenario: one of the tracing provider impls is broken > and > > doesn't handle correctly the category filter. Or we add a new measurement type > > and it doesn't seem to match the events as expected. > > > > To debug either of these issues we want to see exactly the events that the > > benchmarking app gets to work with. We don't want the fact of observing this > > data magically make it different. > > > > If restricting the category filter is useful, we can put it behind a separate > > flag (`--filter-categories`?), but I wouldn't tie it to `--save-traces`. > > Should we then instead filter by category event in the benchmark app, so that > we detect those broken provider? And then it doesn't matter. Having the overhead > of asking all the categories all the time doesn't seem optional. Any update on the CL?
On 2015/10/28 09:16:17, etiennej wrote: > On 2015/10/15 08:04:26, qsr wrote: > > On 2015/10/13 16:58:23, ppi wrote: > > > Consider the following scenario: one of the tracing provider impls is broken > > and > > > doesn't handle correctly the category filter. Or we add a new measurement > type > > > and it doesn't seem to match the events as expected. > > > > > > To debug either of these issues we want to see exactly the events that the > > > benchmarking app gets to work with. We don't want the fact of observing this > > > data magically make it different. > > > > > > If restricting the category filter is useful, we can put it behind a > separate > > > flag (`--filter-categories`?), but I wouldn't tie it to `--save-traces`. > > > > Should we then instead filter by category event in the benchmark app, so that > > we detect those broken provider? And then it doesn't matter. Having the > overhead > > of asking all the categories all the time doesn't seem optional. > > Any update on the CL? From the previous discussion, we can either trace all categories by default and have a `--filter-categories` argument to filter, or do the opposite (filter categories by default and have a `--trace-all argument`). I think that the second one is clearer.
+1
On 2015/10/28 09:34:28, ppi wrote: > +1 --trace-all argument seems fine.
Description was changed from ========== On benchmarking: tracing all events when the "--save-traces" argument is given. fixes #458 ========== to ========== On benchmarking: Adding "--trace-all" argument to avoid filtering by category (ignored when "--save-traces" argument is not given). fixes #458 ==========
nellyv@google.com changed reviewers: + nellyv@google.com
On benchmarking: tracing all events when the "--trace-all" argument is given (ignored when not used with "--save-traces" argument). fixes #458
Description was changed from ========== On benchmarking: Adding "--trace-all" argument to avoid filtering by category (ignored when "--save-traces" argument is not given). fixes #458 ========== to ========== Benchmarking: adding "--trace-all" argument. Add "--trace-all" argument to avoid filtering by category (ignored when "--save-traces" argument is not given). fixes #458 ==========
Description was changed from ========== Benchmarking: adding "--trace-all" argument. Add "--trace-all" argument to avoid filtering by category (ignored when "--save-traces" argument is not given). fixes #458 ========== to ========== Benchmark: Add "--trace-all" argument. Add "--trace-all" argument to avoid filtering by category (ignored when "--save-traces" argument is not given). fixes #458 ==========
On 2015/10/28 15:48:55, nelly wrote: > On benchmarking: tracing all events when the "--trace-all" argument is given > (ignored when not used with "--save-traces" argument). fixes #458 It seems that the bots fail to test your code. Can you look at it?
On 2015/10/29 09:40:47, etiennej wrote: > On 2015/10/28 15:48:55, nelly wrote: > > On benchmarking: tracing all events when the "--trace-all" argument is given > > (ignored when not used with "--save-traces" argument). fixes #458 > > It seems that the bots fail to test your code. Can you look at it? Done!
https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... File apps/benchmark/benchmark_app.cc (right): https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:47: // Don't compute the categories string if all categories should be traced End the comment with a period. https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:69: // Compute the string of trace categories we want to collect: a union of all Function comments should be "third-person", ie. "Computes ...". https://codereview.chromium.org/1391013005/diff/100001/mojo/devtools/common/m... File mojo/devtools/common/mojo_benchmark (left): https://codereview.chromium.org/1391013005/diff/100001/mojo/devtools/common/m... mojo/devtools/common/mojo_benchmark:78: Keep this blank line, Python style guide wants two blank lines between functions. https://codereview.chromium.org/1391013005/diff/100001/mojo/devtools/common/m... File mojo/devtools/common/mojo_benchmark (right): https://codereview.chromium.org/1391013005/diff/100001/mojo/devtools/common/m... mojo/devtools/common/mojo_benchmark:156: help='trace all categories; ignored if not used with ' + Why ignore this if not used with --save-traces?
https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... File apps/benchmark/benchmark_app.cc (right): https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:58: new TraceCollectorClient(this, trace_collector.Pass())); Beware of your whitespace.
https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... File apps/benchmark/benchmark_app.cc (right): https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:58: new TraceCollectorClient(this, trace_collector.Pass())); On 2015/10/29 13:35:51, etiennej wrote: > Beware of your whitespace. Using 'git cl format' should use clang to format your CLs and allow you to think about something else than whitespace.
https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... File apps/benchmark/benchmark_app.cc (right): https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:47: // Don't compute the categories string if all categories should be traced On 2015/10/29 13:33:43, ppi wrote: > End the comment with a period. Done. https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:58: new TraceCollectorClient(this, trace_collector.Pass())); On 2015/10/29 13:40:49, qsr wrote: > On 2015/10/29 13:35:51, etiennej wrote: > > Beware of your whitespace. > > Using 'git cl format' should use clang to format your CLs and allow you to think > about something else than whitespace. Done. https://codereview.chromium.org/1391013005/diff/100001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:69: // Compute the string of trace categories we want to collect: a union of all On 2015/10/29 13:33:43, ppi wrote: > Function comments should be "third-person", ie. "Computes ...". Done. https://codereview.chromium.org/1391013005/diff/100001/mojo/devtools/common/m... File mojo/devtools/common/mojo_benchmark (left): https://codereview.chromium.org/1391013005/diff/100001/mojo/devtools/common/m... mojo/devtools/common/mojo_benchmark:78: On 2015/10/29 13:33:43, ppi wrote: > Keep this blank line, Python style guide wants two blank lines between > functions. Done. https://codereview.chromium.org/1391013005/diff/100001/mojo/devtools/common/m... File mojo/devtools/common/mojo_benchmark (right): https://codereview.chromium.org/1391013005/diff/100001/mojo/devtools/common/m... mojo/devtools/common/mojo_benchmark:156: help='trace all categories; ignored if not used with ' + On 2015/10/29 13:33:43, ppi wrote: > Why ignore this if not used with --save-traces? With the current implementation we always print in stdout the results of the measurements as specified in the (e.g. mojo/tools/data/)benchmarks file, even when the --trace-all argument is given. The role of --trace-all is to print all traces (not only the specified measurements) in a .trace file. - If we decide that we want to print in stdout all results, the output will be too noisy (not really readable). - If we decide to keep the semantics and allow the --trace-all anyway this means that we produce and store traces we will never use.
After a discussion with Przemek, I've removed the `--trace-all` argument and replaced `--save-traces` with `--save all traces`
lgtm
https://codereview.chromium.org/1391013005/diff/160001/apps/benchmark/benchma... File apps/benchmark/benchmark_app.cc (right): https://codereview.chromium.org/1391013005/diff/160001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:49: if (!args_.write_output_file) { nit: it's a bit weird to set the initial value to a special case, and then flip the value in the default case Can you declare the variable without setting anything, and then to: if (args_.write_output_file) { one thing } else { another thing } ? Alternatively you can use the ternary operator. https://codereview.chromium.org/1391013005/diff/160001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:65: base::TimeDelta::FromMilliseconds(1000)); Why the change from s to ms?
https://codereview.chromium.org/1391013005/diff/160001/apps/benchmark/benchma... File apps/benchmark/benchmark_app.cc (right): https://codereview.chromium.org/1391013005/diff/160001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:49: if (!args_.write_output_file) { On 2015/10/29 16:02:09, ppi wrote: > nit: it's a bit weird to set the initial value to a special case, and then flip > the value in the default case > > Can you declare the variable without setting anything, and then to: > > if (args_.write_output_file) { > one thing > } else { > another thing > } > > ? > > Alternatively you can use the ternary operator. Done. https://codereview.chromium.org/1391013005/diff/160001/apps/benchmark/benchma... apps/benchmark/benchmark_app.cc:65: base::TimeDelta::FromMilliseconds(1000)); On 2015/10/29 16:02:09, ppi wrote: > Why the change from s to ms? Done.
lgtm
Description was changed from ========== On benchmarking: Adding "--trace-all" argument to avoid filtering by category (ignored when "--save-traces" argument is not given). fixes #458 BUG= ========== to ========== Benchmark: `--save-all-traces` argument Benchmark: Replace `--save-trace` argument by `--save-all-traces`. It outputs a .trace file per benchmark as before, but stores all traces, without filtering by the measurements defined in the benchmarks file. fixes #458 BUG= ==========
Message was sent while issue was closed.
Committed patchset #10 (id:170001) manually as ebcc63fd341a027acd15b38924779bd63b9bc9a2 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
