adb_profile_chrome: Add perf profiler

adb_profile_chrome: Add perf profiler

This patch makes it possible to run the perf profiler 
through adb_profile_chrome. The result is a trace file that 
contains samples from perf as well as events from Chrome 
trace events or systrace.

For example, to find out in which functions CPU time is 
being spent:

$ build/android/adb_profile_chrome --browser build --time 5 --perf

BUG=375754
TEST=build/android/chrome_profiler/run_tests
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274837

[Dominik Grewe, 2014-05-28 17:09:43]

Some comments below.

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
File build/android/chrome_profiler/main.py (right):

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
build/android/chrome_profiler/main.py:121: ' cycles by default. Use "list" to
see the available '
Nit: remove preceding whitespace

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
File build/android/chrome_profiler/perf_controller.py (right):

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
build/android/chrome_profiler/perf_controller.py:69: self._FailWithLog('Perf
recorded a zero-sized file')
Can we also somehow check if there are any samples in the file? Just because the
files isn't empty, it doesn't mean it has any samples in it.

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
build/android/chrome_profiler/perf_controller.py:106: perf_pids =
self._device.old_interface.ExtractPid('perf')
Why not ask the perf profiler instance directly for its pid? Or maybe combine it
with the Wait() method (SignalAndWait)?

[Sami, 2014-06-02 17:56:38]

Thanks Dominik, comments addressed. I still need to implement merging of chrome
and perf traces here so no rush with this.

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
File build/android/chrome_profiler/main.py (right):

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
build/android/chrome_profiler/main.py:121: ' cycles by default. Use "list" to
see the available '
On 2014/05/28 17:09:43, Dominik Grewe wrote:
> Nit: remove preceding whitespace

Done.

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
File build/android/chrome_profiler/perf_controller.py (right):

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
build/android/chrome_profiler/perf_controller.py:69: self._FailWithLog('Perf
recorded a zero-sized file')
On 2014/05/28 17:09:43, Dominik Grewe wrote:
> Can we also somehow check if there are any samples in the file? Just because
the
> files isn't empty, it doesn't mean it has any samples in it.

Great point, we should complain if the perf trace is empty. Of course I can't
reproduce the case when this happens for real, but by making a file without any
samples manually, we also get an empty set for the required libraries so I'll
just use that as a trigger.

https://codereview.chromium.org/293193002/diff/30005/build/android/chrome_pro...
build/android/chrome_profiler/perf_controller.py:106: perf_pids =
self._device.old_interface.ExtractPid('perf')
On 2014/05/28 17:09:43, Dominik Grewe wrote:
> Why not ask the perf profiler instance directly for its pid? Or maybe combine
it
> with the Wait() method (SignalAndWait)?

Good idea, that's much cleaner.

[Sami, 2014-06-03 19:01:08]

Okay, now we can merge json traces into a single coherent structure that the
trace viewer understands. Here's a sample file:
https://codereview.appspot.com/100970044/

Marcus, can I bribe you into having a look while Dominik is out enjoying the
weather?

[bulach, 2014-06-04 10:34:13]

lgtm, thanks sami! mostly nits, but a follow up suggestion below:

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
File build/android/chrome_profiler/main.py (right):

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/main.py:253: if __name__ == '__main__':
nit FTR: normally the main is a directory above, then |chrome_profiler| becomes
a module imported here.
also, for the future :)
there was a discussion a while ago about moving anything that's not used by the
build system / buildbots out of build/ into tools/.. i.e., changes in tools
can't possibly break the tree, whilst build/ may..

chrome_profiler seems a good example for tools/ ;)

another reason: there's some duplication between this and
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

here's a suggestion:
tools/chrome_profiler/chrome_profiler.py => top-level main. note the absence of
android :)
tools/chrome_profiler/chrome_profiler/__init__.py => a module exposing the API
tools/telemetry/core/platform/profiler/perf_profiler.py => would then just use
the chrome_profiler.py main directly and/or bits and pieces of the underlying
module...

since the main purpose of this patch is to add more functionality, I'd say feel
free to go ahead with it :) 
but it'd be truly nice to restructure it soon to make it all available beyond a
standalone tool!

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
File build/android/chrome_profiler/perf_controller.py (right):

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/perf_controller.py:37: class
_PerfProfilerInstance(object):
nit: how about just _PerfProfiler? or _PerfProfilerRunner?

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/perf_controller.py:142: perfhost_path = \
nit: avoid \, something like:

perfhost_path = os.path.abspath(
....support_binaries.FindPath('perfhost', 'linux'))

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/perf_controller.py:144: perf_script_path = \
ditto..

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/perf_controller.py:150: with open(json_file_name,
'w') as json_file:
nit: I think we're on python2.7 everywhere, so multiple clause |with| should be
fine:

with open(os.devnull, 'w') as dev_null,\
     open(json_file_name, 'w') as json_file:

truly stylistic, but reduces one indentation level. :)

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
File build/android/chrome_profiler/postprocessor.py (right):

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/postprocessor.py:32: with open(host_file, 'rb') as
input_file:
nit: single with

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/postprocessor.py:76: def
PackageTraces(trace_files, output=None, compress=False, write_json=False):
nit: postprocessor.py seems fairly generic :)
how about trace_packager.py ?

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
File build/android/chrome_profiler/postprocessor_unittest.py (right):

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/postprocessor_unittest.py:18: with
tempfile.NamedTemporaryFile(delete=False) as f2:
why delete=False? the with would only close them at the end, so perhaps just
keep the default delete=True? ...or some comment :)

[Sami, 2014-06-04 15:16:59]

Thanks Marcus, all addressed. Let me spin up another patch for the
reorganization.

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
File build/android/chrome_profiler/main.py (right):

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/main.py:253: if __name__ == '__main__':
On 2014/06/04 10:34:12, bulach wrote:
> nit FTR: normally the main is a directory above, then |chrome_profiler|
becomes
> a module imported here.

Ah, actually that's already the case: build/android/adb_profile_chrome.py
imports this file can calls main(). Looks like I just forgot this entry point
here.

> also, for the future :)
> there was a discussion a while ago about moving anything that's not used by
the
> build system / buildbots out of build/ into tools/.. i.e., changes in tools
> can't possibly break the tree, whilst build/ may..
> 
> chrome_profiler seems a good example for tools/ ;)

That's an excellent reason for moving.

> another reason: there's some duplication between this and
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...

Yeah, there's some duplication, although not a whole lot since I added
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py.

> here's a suggestion:
> tools/chrome_profiler/chrome_profiler.py => top-level main. note the absence
of
> android :)

Actually this being limited to Android is intentional: the tracing we do here is
completely Android specific (tracing with intents and systrace). I don't want to
build another telemetry here :) Admittedly perf falls a little to a gray area.

Also, there's another requirement: adb_profile_chrome needs to work as a
standalone tool for web developers: https://github.com/johnmccutchan/adb_trace.
That's also a good reason to restructure as you said, but it may affect the
layout a bit.

> tools/chrome_profiler/chrome_profiler/__init__.py => a module exposing the API
> tools/telemetry/core/platform/profiler/perf_profiler.py => would then just use
> the chrome_profiler.py main directly and/or bits and pieces of the underlying
> module...
> 
> since the main purpose of this patch is to add more functionality, I'd say
feel
> free to go ahead with it :) 
> but it'd be truly nice to restructure it soon to make it all available beyond
a
> standalone tool!

Right. Let me land this patch first and then do another pass for reorganizing.
Sound good?

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
File build/android/chrome_profiler/perf_controller.py (right):

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/perf_controller.py:37: class
_PerfProfilerInstance(object):
On 2014/06/04 10:34:12, bulach wrote:
> nit: how about just _PerfProfiler? or _PerfProfilerRunner?

_PerfProfiler sgtm.

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/perf_controller.py:142: perfhost_path = \
On 2014/06/04 10:34:12, bulach wrote:
> nit: avoid \, something like:
> 
> perfhost_path = os.path.abspath(
> ....support_binaries.FindPath('perfhost', 'linux'))

Well spotted, done. I just got used to having long lines here that wouldn't
split without '\' :)

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/perf_controller.py:144: perf_script_path = \
On 2014/06/04 10:34:12, bulach wrote:
> ditto..

Done.

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/perf_controller.py:150: with open(json_file_name,
'w') as json_file:
On 2014/06/04 10:34:12, bulach wrote:
> nit: I think we're on python2.7 everywhere, so multiple clause |with| should
be
> fine:
> 
> with open(os.devnull, 'w') as dev_null,\
>      open(json_file_name, 'w') as json_file:
> 
> truly stylistic, but reduces one indentation level. :)

Oh thanks, I forgot about that syntax!

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
File build/android/chrome_profiler/postprocessor.py (right):

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/postprocessor.py:32: with open(host_file, 'rb') as
input_file:
On 2014/06/04 10:34:12, bulach wrote:
> nit: single with

Done.

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/postprocessor.py:76: def
PackageTraces(trace_files, output=None, compress=False, write_json=False):
On 2014/06/04 10:34:12, bulach wrote:
> nit: postprocessor.py seems fairly generic :)
> how about trace_packager.py ?

Yes, much better.

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
File build/android/chrome_profiler/postprocessor_unittest.py (right):

https://codereview.chromium.org/293193002/diff/140001/build/android/chrome_pr...
build/android/chrome_profiler/postprocessor_unittest.py:18: with
tempfile.NamedTemporaryFile(delete=False) as f2:
On 2014/06/04 10:34:12, bulach wrote:
> why delete=False? the with would only close them at the end, so perhaps just
> keep the default delete=True? ...or some comment :)

Both of these files get deleted during merging, and NamedTemporaryFile doesn't
like if you delete the file before it gets to it. I've added a comment to make
this clearer.

[Sami, 2014-06-04 15:25:09]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-06-04 15:26:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/skyostil@chromium.org/293193002/180001

[commit-bot: I haz the power, 2014-06-04 15:28:08]

Change committed as 274837