Deprecate Benchmark.SetupBenchmarkDefaultTraceRerunOptions

[nedn]

Looking at https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/benchmark.py#L133, this method will never be called since HasTraceRerunDebugOption() & browser_options.rerun_with_debug_trace evaluate to the same value. @perezju

[perezju]

Not true.

HasTraceRerunDebugOption() may be overriden by a benchmark to indicate that the command line option --trace-rerun-option should be exposed to the user. https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/benchmark.py#L106

While browser_options.rerun_with_debug_trace is only true iff the user actually supplied --trace-rerun-option when running the benchmark.

[nedn]

You 're correct, Juan. Sorry for my silly reading. Though I think SetupBenchmarkDefaultTraceRerunOptions seems to be overlapping with CreateTimelineBasedMeasurementOptions(). Also the naming of this method is confusing, because this only runs when benchmark is not run with " --trace-rerun-option" flag.

[perezju]

Yes, I agree, and that was also the main source of confusion on my CL (and my even earlier work that introduced the bug this CL is trying to fix).

As I understand things we have three methods doing similar things:

• CreateTimelineBasedMeasurementOptions called always
• SetupBenchmarkDebugTraceRerunOptions called only if --trace-rerun-option is given
• SetupBenchmarkDefaultTraceRerunOptions called only if --trace-rerun-option is not given

Additionally, for the later two to be called, HasTraceRerunDebugOption must have been overriden to return True.

[nedn]

This situation is more confusing that it needs to be, I think we can: 1) Remove SetupBenchmarkDefaultTraceRerunOptions. There is no need to mutate the options any further if --trace-rerun-option is not given. 2) Remove HasTraceRerunDebugOption and always call SetupBenchmarkDebugTraceRerunOptions when --trace-rerun-option is given. By default, SetupBenchmarkDebugTraceRerunOptions will do nothing. Maybe we add a warning to the default implementation saying "this method is not overriden, will use the the same options as non debug run".

Thoughts?

[perezju]

Sadly that doesn't work because (1) is not True. For TBM metrics, when neither of the SetupBenchmark*RerunOptions options is overriden, the default behavior is to do tracing with all metrics enabled.

The memory-infra benchmarks, in fact, override SetupBenchmarkDefaultTraceRerunOptions to specify a reduced set of metrics, and does not override SetupBenchmarkDebugTraceRerunOptions to leave the “all metrics” behavior when --trace-rerun-option is given.

@skyostil suggested doing this in https://codereview.chromium.org/1481043002

To simplify things, how about the following suggestion:

• Expect benchmarks to override only CreateTimelineBasedMeasurementOptions to set the tracing filters and metrics that they need. (Most benchmarks already do this).
• Get rid of SetupBenchmarkDefaultTraceRerunOptions. The default options are the ones set by the benchmark on the method above.
• Make the default implementation of SetupBenchmarkDebugTraceRerunOptions override tracing options to include a (fixed) larger set of tracing categories and metrics. Most benchmarks won't need to override this. But they still could if they so wished.
• Get rid of HasTraceRerunDebugOption. All benchmarks can how handle this option.

Not sure how this affects non-TBM benchmarks, though.

[perezju]

• @simonhatch @anniesullie who introduced the --trace-rerun option at https://codereview.chromium.org/840043004

[nedn]

Your plan is making the default implementation of SetupBenchmarkDebugTraceRerunOptions show few other important metrics along with a larger set of tracing categories sounds good to me.

On another hand, I can says that the default behavior of running all metrics is faulty and no existing benchmark does it. We should also remove that default behavior in TimelineBasedMeasurement @eakuefner.

[simonhatch]

It's been a while since I added this, so memory might be a bit fuzzy. I think the suggestions sound fine, except that HasTraceRerunDebugOption affects whether that option is exported into the chartjson data, which is used by the dashboard to determine if it can rerun a benchmark with debug tracing categories enabled (and how to run it). You'll just need to make sure that's taken care of for non-TBM benchmarks so that they don't upload an invalid rerun flag.

[eakuefner]

Filed #2143 about forcing legacy timeline-based benchmarks to call SetLegacyTimelineBasedMetrics.