telemetry: Make trace profiler work with trace-based benchmarks.

Made trace profiler work with trace-based benchmarks.

R=tonyg
BUG=282712
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223330

[tonyg, 2013-09-11 21:31:44]

https://codereview.chromium.org/23902027/diff/1/tools/perf/measurements/raste...
File tools/perf/measurements/rasterize_and_record.py (right):

https://codereview.chromium.org/23902027/diff/1/tools/perf/measurements/raste...
tools/perf/measurements/rasterize_and_record.py:154: if self.options.profiler !=
'trace':
Making this Measurement aware of profilers which are running seems fragile to
me. It also means that any Measurements which use Start/StopTracing() will have
to be careful to do this dance.

I'd much prefer a design where the Start/StopTracing() methods themselves were
robust against multiple users.

What if StartTracing() incremented a counter and StopTracing() would decrement
but only actually stop if the count hits zero. Then GetTraceResultAndReset()
could check whether tracing is still running, and if so, just queue up a Reset
operation for after the tracing is actually stopped.

Anyway, kind of thinking aloud here, please push back if I'm crazy.

[ernstm, 2013-09-12 00:12:10]

Here is a new implementation that tracks the nesting depth of calls to
Start/StopTracing. The implementation starts tracing on the first StartTracing
call and it also stops on the first (!) StopTracing call, because results can
only be collected when tracing has stopped. Stop tracing now directly returns
the TraceResult and it resets _tracing_data on the last StopTracing call.

[tonyg, 2013-09-12 00:49:54]

Just some minor remaining questions. I really like your new approach.

https://codereview.chromium.org/23902027/diff/6001/tools/perf/measurements/ra...
File tools/perf/measurements/rasterize_and_record.py (right):

https://codereview.chromium.org/23902027/diff/6001/tools/perf/measurements/ra...
tools/perf/measurements/rasterize_and_record.py:169: timeline =
tab.browser.StopTracing().AsTimelineModel()
I suspect this line might be a little expensive which is why we bothered to
self._metrics.Stop() before AsTimelineModel().

Suggest flipping the order of this and the next line.

https://codereview.chromium.org/23902027/diff/6001/tools/perf/measurements/sm...
File tools/perf/measurements/smoothness.py (right):

https://codereview.chromium.org/23902027/diff/6001/tools/perf/measurements/sm...
tools/perf/measurements/smoothness.py:61: self._timeline =
tab.browser.StopTracing().AsTimelineModel()
Same question about whether we should do the AsTimelineModel() later.

https://codereview.chromium.org/23902027/diff/6001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py (right):

https://codereview.chromium.org/23902027/diff/6001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py:101:
self._nesting -= 1
Let's assert that _nesting doesn't go negative

[ernstm, 2013-09-12 18:06:07]

https://codereview.chromium.org/23902027/diff/6001/tools/perf/measurements/ra...
File tools/perf/measurements/rasterize_and_record.py (right):

https://codereview.chromium.org/23902027/diff/6001/tools/perf/measurements/ra...
tools/perf/measurements/rasterize_and_record.py:169: timeline =
tab.browser.StopTracing().AsTimelineModel()
On 2013/09/12 00:49:55, tonyg wrote:
> I suspect this line might be a little expensive which is why we bothered to
> self._metrics.Stop() before AsTimelineModel().
> 
> Suggest flipping the order of this and the next line.

Done.

https://codereview.chromium.org/23902027/diff/6001/tools/perf/measurements/sm...
File tools/perf/measurements/smoothness.py (right):

https://codereview.chromium.org/23902027/diff/6001/tools/perf/measurements/sm...
tools/perf/measurements/smoothness.py:61: self._timeline =
tab.browser.StopTracing().AsTimelineModel()
On 2013/09/12 00:49:55, tonyg wrote:
> Same question about whether we should do the AsTimelineModel() later.

Done.

https://codereview.chromium.org/23902027/diff/6001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py (right):

https://codereview.chromium.org/23902027/diff/6001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py:101:
self._nesting -= 1
On 2013/09/12 00:49:55, tonyg wrote:
> Let's assert that _nesting doesn't go negative

Done.

[ernstm, 2013-09-13 18:13:38]

Tony, did you get a chance to look at the latest changes in this CL?

[tonyg, 2013-09-13 18:16:04]

Oops, thanks for the ping.

Just one more comment, else lg

https://codereview.chromium.org/23902027/diff/12001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py (right):

https://codereview.chromium.org/23902027/diff/12001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/backends/chrome/tracing_backend.py:78: def
StartTracing(self, custom_categories=None, timeout=10):
Just thought of one more edge case.

Since custom_categories is only respected on the first call to StartTracing, we
should assert that subsequent calls to StartTracing do not include a broader set
of custom_categories.

[ernstm, 2013-09-13 20:29:54]

Given the way the filters are designed, it is not possible to determine with
certainty that one filter is a subset of another. For example A="" and B=<list
of all default categories> are identical, but we can't tell, because the list of
all default categories is unknown. The best we can do is approximate the subset
calculation and catch the most common cases.

[ernstm, 2013-09-13 22:55:26]

The test in patch set #4 works for category filters that don't contain
wildcards. It throws an exception if the new filter is definitely not a subset
of the existing one. It issues a warning if it cannot tell for sure.

[tonyg, 2013-09-16 03:20:08]

lgtm

[commit-bot: I haz the power, 2013-09-16 05:43:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ernstm@chromium.org/23902027/19001

[commit-bot: I haz the power, 2013-09-16 11:49:47]

Change committed as 223330

[tonyg, 2013-09-16 18:07:12]

Looks like this broke the smoothness_measurement on all the bots with
the reference builds. I recommend we revert unless there is a trivial
fix.

Example:
http://build.chromium.org/p/chromium.perf/builders/Win7%20GPU%20%28ATI%29%20P...


On Mon, Sep 16, 2013 at 4:49 AM,  <commit-bot@chromium.org> wrote:
> Change committed as 223330
>
> https://chromiumcodereview.appspot.com/23902027/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2013-09-16 18:44:02]

This is probably related to this 8.3% regression.

https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=chromium-rel-...

Is there a bug open for this?

On Mon, Sep 16, 2013 at 12:06 PM, Tony Gentilcore <tonyg@chromium.org> wrote:
> Looks like this broke the smoothness_measurement on all the bots with
> the reference builds. I recommend we revert unless there is a trivial
> fix.
>
> Example:
>
http://build.chromium.org/p/chromium.perf/builders/Win7%20GPU%20%28ATI%29%20P...
>
>
> On Mon, Sep 16, 2013 at 4:49 AM,  <commit-bot@chromium.org> wrote:
>> Change committed as 223330
>>
>> https://chromiumcodereview.appspot.com/23902027/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ernstm1, 2013-09-16 19:31:12]

I think there is a trivial fix: adding back the 'webkit' category should
solve the problem. Only recent builds will support the more specific
'webkit.console' category.


On Mon, Sep 16, 2013 at 11:06 AM, Tony Gentilcore <tonyg@chromium.org>wrote:

> Looks like this broke the smoothness_measurement on all the bots with
> the reference builds. I recommend we revert unless there is a trivial
> fix.
>
> Example:
>
>
http://build.chromium.org/p/chromium.perf/builders/Win7%20GPU%20%28ATI%29%20P...
>
>
> On Mon, Sep 16, 2013 at 4:49 AM,  <commit-bot@chromium.org> wrote:
> > Change committed as 223330
> >
> > https://chromiumcodereview.appspot.com/23902027/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ernstm, 2013-09-16 19:35:39]

On 2013/09/16 18:44:02, teravest wrote:
> This is probably related to this 8.3% regression.
> 
>
https://chromeperf.appspot.com/report?masters=ChromiumPerf&bots=chromium-rel-...

It looks like the perf regression happend before this CL landed.

[nduca, 2013-09-17 19:25:18]

Belatedly, this is a really nice patch. Really nicely done!