[memory_infra] Fix trace rerun options in memory benchmarks

[memory_infra] Fix trace rerun options in memory benchmarks

BUG=593678
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq

Committed: https://crrev.com/bcb9574723963b3e2b9eec46baa8b0d1d230bf6c
Cr-Commit-Position: refs/heads/master@{#381434}

[perezju, 2016-03-10 13:42:28]

Description was changed from

==========
[memory_infra] Fix trace rerun options in memory benchmarks

BUG=593678
==========

to

==========
[memory_infra] Fix trace rerun options in memory benchmarks

BUG=593678
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq
==========

[perezju, 2016-03-10 13:45:46]

perezju@chromium.org changed reviewers:
+ eakuefner@chromium.org, nednguyen@google.com, ulan@chromium.org

[perezju, 2016-03-10 13:45:47]

https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
File tools/perf/benchmarks/memory_infra.py (right):

https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
tools/perf/benchmarks/memory_infra.py:43: def HasTraceRerunDebugOption(cls):
The bug was caused by this method having the wrong name.

[nednguyen, 2016-03-10 16:33:32]

https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
File tools/perf/benchmarks/memory_infra.py (right):

https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
tools/perf/benchmarks/memory_infra.py:146: ))
We also need to set the categories properly, I think?

[perezju, 2016-03-14 11:55:03]

perezju@chromium.org changed reviewers:
+ primiano@chromium.org

[perezju, 2016-03-14 11:55:04]

https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
File tools/perf/benchmarks/memory_infra.py (right):

https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
tools/perf/benchmarks/memory_infra.py:146: ))
On 2016/03/10 16:33:32, nednguyen wrote:
> We also need to set the categories properly, I think?

So, you mean all of the code to set category filters from
CreateTimelineBasedMeasurementOptions should be moved into
SetupBenchmarkDefaultTraceRerunOptions?

I understand the idea is that when passing --rerun-with-debug-trace (when
SetupBenchmarkDefaultTraceRerunOptions is *not* called) we don't exclude as much
categories/metrics, etc. But I'm not quite sure as to what should be passed in
either case to ensure memory infra is still always enabled.

cc'ing Primiano to help us figure this one out.

[nednguyen, 2016-03-14 15:10:49]

https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
File tools/perf/benchmarks/memory_infra.py (right):

https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
tools/perf/benchmarks/memory_infra.py:146: ))
On 2016/03/14 11:55:03, perezju wrote:
> On 2016/03/10 16:33:32, nednguyen wrote:
> > We also need to set the categories properly, I think?
> 
> So, you mean all of the code to set category filters from
> CreateTimelineBasedMeasurementOptions should be moved into
> SetupBenchmarkDefaultTraceRerunOptions?
> 
> I understand the idea is that when passing --rerun-with-debug-trace (when
> SetupBenchmarkDefaultTraceRerunOptions is *not* called) we don't exclude as
much
> categories/metrics, etc. But I'm not quite sure as to what should be passed in
> either case to ensure memory infra is still always enabled.
> 
> cc'ing Primiano to help us figure this one out.

Ah, I just look at the code. It looks like you don't need to anything else,
since this method is for adding more options to the non-debug one: 
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...

So I think you can leave this function as-is, or override it only if you want
more metrics/categories enabled.

[perezju, 2016-03-14 15:32:23]

On 2016/03/14 15:10:49, nednguyen wrote:
>
https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
> File tools/perf/benchmarks/memory_infra.py (right):
> 
>
https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
> tools/perf/benchmarks/memory_infra.py:146: ))
> On 2016/03/14 11:55:03, perezju wrote:
> > On 2016/03/10 16:33:32, nednguyen wrote:
> > > We also need to set the categories properly, I think?
> > 
> > So, you mean all of the code to set category filters from
> > CreateTimelineBasedMeasurementOptions should be moved into
> > SetupBenchmarkDefaultTraceRerunOptions?
> > 
> > I understand the idea is that when passing --rerun-with-debug-trace (when
> > SetupBenchmarkDefaultTraceRerunOptions is *not* called) we don't exclude as
> much
> > categories/metrics, etc. But I'm not quite sure as to what should be passed
in
> > either case to ensure memory infra is still always enabled.
> > 
> > cc'ing Primiano to help us figure this one out.
> 
> Ah, I just look at the code. It looks like you don't need to anything else,
> since this method is for adding more options to the non-debug one: 
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> 
> So I think you can leave this function as-is, or override it only if you want
> more metrics/categories enabled.

IIRC, at some point +Sami suggested overriding this method, so that issuing
--rerun-with-debug-trace does include tracing information from more
metrics.

That was the intent above (in _MemoryInfra), but due to my mistake in
the method name this hasn't been working as expected (and memory-infra
benchmarks have been gathering data for all metrics all this time;
except we don't see them in results because we filter them out in
ValueCanBeAddedPredicate).

[Primiano Tucci (use gerrit), 2016-03-14 16:24:12]

On 2016/03/14 15:32:23, perezju wrote:
> On 2016/03/14 15:10:49, nednguyen wrote:
> >
>
https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
> > File tools/perf/benchmarks/memory_infra.py (right):
> > 
> >
>
https://codereview.chromium.org/1785683002/diff/1/tools/perf/benchmarks/memor...
> > tools/perf/benchmarks/memory_infra.py:146: ))
> > On 2016/03/14 11:55:03, perezju wrote:
> > > On 2016/03/10 16:33:32, nednguyen wrote:
> > > > We also need to set the categories properly, I think?
> > > 
> > > So, you mean all of the code to set category filters from
> > > CreateTimelineBasedMeasurementOptions should be moved into
> > > SetupBenchmarkDefaultTraceRerunOptions?
> > > 
> > > I understand the idea is that when passing --rerun-with-debug-trace (when
> > > SetupBenchmarkDefaultTraceRerunOptions is *not* called) we don't exclude
as
> > much
> > > categories/metrics, etc. But I'm not quite sure as to what should be
passed
> in
> > > either case to ensure memory infra is still always enabled.
> > > 
> > > cc'ing Primiano to help us figure this one out.
> > 
> > Ah, I just look at the code. It looks like you don't need to anything else,
> > since this method is for adding more options to the non-debug one: 
> >
>
https://github.com/catapult-project/catapult/blob/master/telemetry/telemetry/...
> > 
> > So I think you can leave this function as-is, or override it only if you
want
> > more metrics/categories enabled.
> 
> IIRC, at some point +Sami suggested overriding this method, so that issuing
> --rerun-with-debug-trace does include tracing information from more
> metrics.
> 
> That was the intent above (in _MemoryInfra), but due to my mistake in
> the method name this hasn't been working as expected (and memory-infra
> benchmarks have been gathering data for all metrics all this time;
> except we don't see them in results because we filter them out in
> ValueCanBeAddedPredicate).

So my understanding is that:
today, before this CL, we are running all the metrics in our benchmark (Which I
guess might cause fluctuations in memory if somebody enables a new metric
unrelated with memory). This seems bad.
Juan, with this CL, will enforce to run only our metric. This seems bad.
Although, after this CL --rerun-with-debug-trace will still run all the metric.
This seems bad, but not worse than the current situation.

Concretely I never used --rerun-with-debug-trace nor know what problem that is
trying to solve. Is that used by bisect/trybot or what?
In general this CL looks an improvement to me

[Sami, 2016-03-14 18:00:53]

> Concretely I never used --rerun-with-debug-trace nor know what problem that is
> trying to solve. Is that used by bisect/trybot or what?
> In general this CL looks an improvement to me

--rerun-with-debug-trace is there for the cases where you want a trace from a
benchmark but want to see more categories than the ones used by the metric
itself. Kind of like a replacement for --profiler=trace.

[perezju, 2016-03-15 13:12:36]

After the discussion I think my CL achieves the intended effect (respectively
with and without --rerun-with-debug-trace), and is landable as-is.

Ned, what do you think? Would be anything else needed?

[nednguyen, 2016-03-15 17:27:32]

On 2016/03/15 13:12:36, perezju wrote:
> After the discussion I think my CL achieves the intended effect (respectively
> with and without --rerun-with-debug-trace), and is landable as-is.
> 
> Ned, what do you think? Would be anything else needed?

lgtm, thanks for the context

[perezju, 2016-03-16 09:44:50]

The CQ bit was checked by perezju@chromium.org

[commit-bot: I haz the power, 2016-03-16 09:45:00]

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

[commit-bot: I haz the power, 2016-03-16 11:27:00]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-16 11:28:20]

Description was changed from

==========
[memory_infra] Fix trace rerun options in memory benchmarks

BUG=593678
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq
==========

to

==========
[memory_infra] Fix trace rerun options in memory benchmarks

BUG=593678
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:android_s5_perf_cq;tryserver.chromium.perf:winx64_10_perf_cq;tryserver.chromium.perf:mac_retina_perf_cq;tryserver.chromium.perf:linux_perf_cq

Committed: https://crrev.com/bcb9574723963b3e2b9eec46baa8b0d1d230bf6c
Cr-Commit-Position: refs/heads/master@{#381434}
==========

[commit-bot: I haz the power, 2016-03-16 11:28:21]

Patchset 1 (id:??) landed as
https://crrev.com/bcb9574723963b3e2b9eec46baa8b0d1d230bf6c
Cr-Commit-Position: refs/heads/master@{#381434}