telemetry: Clean up SurfaceFlinger statistics

PERF-SHERIFFS: This patch will change all smoothness and SurfaceFlinger metrics on Android. Some metrics may go up or down significantly, with the new values being more accurate.

This patch cleans up the integration of SurfaceFlinger statistics by pulling the
raw timestamps into the timeline and calculating all metrics in
SmoothnessMetric. This approach also improves the metrics, because the
timeline ranges can now be properly bounded (no more percentiles!) and the
existing smoothness metrics can also use the more accurate SurfaceFlinger
timestamps.

BUG=351916
Committed: https://crrev.com/ebae600f54ed1e77eeba916635310fb176c0fb6c
Cr-Commit-Position: refs/heads/master@{#308488}

[ernstm, 2014-12-05 21:02:34]

ernstm@chromium.org changed reviewers:
+ chrishenry@google.com, dtu@chromium.org, nednguyen@google.com,
skyostil@google.com, tonyg@chromium.org

[ernstm, 2014-12-05 21:03:15]

PTAL. (unit test coverage WIP).

[tonyg, 2014-12-06 00:52:56]

Awesome cleanup. I'll be happy w/ it after tests are in there.

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:105: def
IsDisplayTracingSupported(self):
Should these move into tracing_controller? A new controller? It's okay if the
answer is: eventually, but not in this patch.

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/timeline/surface_flinger_timeline_data.py
(right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/surface_flinger_timeline_data.py:20: 'tid':
9999,
Is 9999 special?

[chrishenry, 2014-12-06 01:22:38]

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:105: def
IsDisplayTracingSupported(self):
On 2014/12/06 00:52:56, tonyg wrote:
> Should these move into tracing_controller? A new controller? It's okay if the
> answer is: eventually, but not in this patch.

I think yes, this should be factored into ubertracing when that's ready, or even
earlier. In fact, might be able to get SurfaceFlinger stat from systrace
directly at that point? Annie owned the bug for ubertracing now, may want to
sync with her.

Making TracingController controls the Start/Stop has an added benefit of not
needing to add a separate Start/Stop call to TBM.

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:111: if
process.name == 'SurfaceFlinger':
I'm confused, who calls Start/StopDisplayTracing for TBM?

[nednguyen, 2014-12-08 00:31:13]

nednguyen@google.com changed reviewers:
+ sullivan@chromium.org

[nednguyen, 2014-12-08 00:31:14]

+Annie since she will work on ubertracing.

[ernstm, 2014-12-08 06:32:01]

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/__init__.py (right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/__init__.py:105: def
IsDisplayTracingSupported(self):
On 2014/12/06 01:22:38, chrishenry wrote:
> On 2014/12/06 00:52:56, tonyg wrote:
> > Should these move into tracing_controller? A new controller? It's okay if
the
> > answer is: eventually, but not in this patch.
> 
> I think yes, this should be factored into ubertracing when that's ready, or
even
> earlier. In fact, might be able to get SurfaceFlinger stat from systrace
> directly at that point? Annie owned the bug for ubertracing now, may want to
> sync with her.
> 
> Making TracingController controls the Start/Stop has an added benefit of not
> needing to add a separate Start/Stop call to TBM.

Yes, this should be factored into ubertracing as a follow-up patch.

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/timeline/surface_flinger_timeline_data.py
(right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/surface_flinger_timeline_data.py:20: 'tid':
9999,
On 2014/12/06 00:52:56, tonyg wrote:
> Is 9999 special?

Good catch! I just forgot to replace this by the actual PID of SurfaceFlinger,
which we can query via adb.

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py (right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/web_perf/metrics/rendering_stats.py:111: if
process.name == 'SurfaceFlinger':
On 2014/12/06 01:22:38, chrishenry wrote:
> I'm confused, who calls Start/StopDisplayTracing for TBM?

SurfaceFlinger stats are currently not used in TBM. Start/Stop is called from
SmoothnessController.

[Sami, 2014-12-08 13:37:34]

skyostil@chromium.org changed reviewers:
+ skyostil@chromium.org

[Sami, 2014-12-08 13:37:35]

Great cleanup! Is there a way to make build/android/surface_stats.py work in
this new world? If not, let's nuke it -- although it has been a useful utility
every now and then.

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:98: return
True
Since we removed the <JB fallback, we should check for that here. Otherwise I
think the few remaining ICS bots on the waterfall will have a bad time.

[ernstm, 2014-12-15 22:30:57]

The latest patch set
- adds tests for the importer, rendering stats and metric,
- removes surface_stats.py (--interactive mode for smoothness benchmark should
cover that)
- use milliseconds as the unit for all SurfaceFlinger data
- Check android version (SurfaceFlinger stats not supported <J)
- Get actual PID of SurfaceFlinger process for events.

This should address all comments. Please take another look.

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:98: return
True
On 2014/12/08 13:37:35, Sami (OoO U-2603) wrote:
> Since we removed the <JB fallback, we should check for that here. Otherwise I
> think the few remaining ICS bots on the waterfall will have a bad time.

Done.

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
File tools/telemetry/telemetry/timeline/surface_flinger_timeline_data.py
(right):

https://codereview.chromium.org/765773004/diff/100001/tools/telemetry/telemet...
tools/telemetry/telemetry/timeline/surface_flinger_timeline_data.py:20: 'tid':
9999,
On 2014/12/08 06:32:00, ernstm wrote:
> On 2014/12/06 00:52:56, tonyg wrote:
> > Is 9999 special?
> 
> Good catch! I just forgot to replace this by the actual PID of SurfaceFlinger,
> which we can query via adb.

Done.

[tonyg, 2014-12-15 23:30:03]

lgtm

https://codereview.chromium.org/765773004/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/765773004/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:99: if
(os_version >= 'J'):
return bool(self.GetOSVersionName() >= 'J')?

[ernstm, 2014-12-15 23:52:01]

The CQ bit was checked by ernstm@chromium.org

[ernstm, 2014-12-15 23:52:11]

https://codereview.chromium.org/765773004/diff/120001/tools/telemetry/telemet...
File tools/telemetry/telemetry/core/platform/android_platform_backend.py
(right):

https://codereview.chromium.org/765773004/diff/120001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/android_platform_backend.py:99: if
(os_version >= 'J'):
On 2014/12/15 23:30:03, tonyg wrote:
> return bool(self.GetOSVersionName() >= 'J')?

Done.

[commit-bot: I haz the power, 2014-12-15 23:53:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/765773004/140001

[commit-bot: I haz the power, 2014-12-16 00:06:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-16 00:06:17]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jbudorick, 2014-12-16 00:09:43]

jbudorick@chromium.org changed reviewers:
+ jbudorick@chromium.org

[jbudorick, 2014-12-16 00:09:44]

build/android/ lgtm

[ernstm, 2014-12-16 00:13:54]

The CQ bit was checked by ernstm@chromium.org

[commit-bot: I haz the power, 2014-12-16 00:15:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/765773004/140001

[commit-bot: I haz the power, 2014-12-16 01:10:58]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-12-16 01:11:34]

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

[bokan, 2014-12-16 16:08:52]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/809753002/ by bokan@chromium.org.

The reason for reverting is: Perf sheriff here, speculatively reverting due to
crbug.com/442752, it shows up in many blamelists (but not all as the test wasn't
running due to another bug). Sorry if unrelated..