telemetry: Improve symfs generation on Android

Symfs is a set of files needed by perf to turn binary 
addresses into human readable symbols. This patch improves the 
symfs generation process on Android in four ways:

1. Actually verify that the unstripped symbol library matches 
   the stripped file that was running on the device. This helps 
   to avoid wasting time looking at bogus perf reports generated
   from mismatching libraries.

2. Download the exact set of system libraries used by the
   application instead of a hardcoded set. This makes the download
   process much faster. The tool now also checks that the libraries
   we have downloaded match the ones on the device.

3. Refactor symfs construction and other Android-specific perf
   steps into a standalone helper (with tests). This helper will
   eventually be used by adb_profile_chrome for perf profiling.

4. Fix a case where perf could fail silently and we would
   download a previously recorded perf run from the device.

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

[Dominik Grewe, 2014-05-21 17:11:56]

As we talked about, it would great to be able to re-use the helper methods for
the VTune profiler. I'll check tomorrow if there's an equivalent way to collect
the required libraries in VTune.

Two random comments below.

https://codereview.chromium.org/290173006/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/profiler/android_perf_helper.py
(right):

https://codereview.chromium.org/290173006/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/profiler/android_perf_helper.py:30:
device_md5 = device_md5.split(' ')[0]
Nit: Any reason you're not passing 1 as an argument to split here as you do
above? They should do the same thing, so maybe it's better to keep it
consistent?

https://codereview.chromium.org/290173006/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/profiler/android_perf_helper.py:122:
os.stat(output_lib).st_mtime + 1):
Why the '+ 1'?

[Sami, 2014-05-21 17:15:56]

Rebased.

[Sami, 2014-05-21 17:23:21]

Great point about VTune. The bulk of symfs creation should work with that too --
we just need a way to figure out which libraries to extract.

https://codereview.chromium.org/290173006/diff/50001/tools/telemetry/telemetr...
File tools/telemetry/telemetry/core/platform/profiler/android_perf_helper.py
(right):

https://codereview.chromium.org/290173006/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/profiler/android_perf_helper.py:30:
device_md5 = device_md5.split(' ')[0]
On 2014/05/21 17:11:56, Dominik Grewe wrote:
> Nit: Any reason you're not passing 1 as an argument to split here as you do
> above? They should do the same thing, so maybe it's better to keep it
> consistent?

Ah, that was just my mistake. Fixed.

https://codereview.chromium.org/290173006/diff/50001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/profiler/android_perf_helper.py:122:
os.stat(output_lib).st_mtime + 1):
On 2014/05/21 17:11:56, Dominik Grewe wrote:
> Why the '+ 1'?

While testing I noticed that /tmp on my machine had worse timestamp precision
than the main filesystem, so timestamps ended up getting truncated to full
seconds. This is just to guard against that. Unfortunately this is filesystem
specific[1] so it's difficult to pick a good value.

[1] https://docs.python.org/2/library/os.html#os.stat

[Dominik Grewe, 2014-05-23 09:51:50]

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py
(right):

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:83:
Prepares a set of files ("symfs") to be used with the perf tool for
'perf' -> 'profiling'? We want to use it for other profilers too.

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:95:
List of arguments to be passed to perf to point it to the created symfs.
Rather than returning perf-specific arguments, can we just return the two paths?

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:108:
# If this our own library instead of a system one, look for a matching
Nit: 'If this is our own...'

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:123:
os.stat(output_lib).st_mtime + 1):
Maybe add a short comment explaining why the '+ 1' is necessary. Otherwise it
looks like it might be there by accident.

[Sami, 2014-05-27 10:14:58]

Thanks Dominik. All comments addressed.

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py
(right):

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:83:
Prepares a set of files ("symfs") to be used with the perf tool for
On 2014/05/23 09:51:51, Dominik Grewe wrote:
> 'perf' -> 'profiling'? We want to use it for other profilers too.

Good point, done.

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:95:
List of arguments to be passed to perf to point it to the created symfs.
On 2014/05/23 09:51:51, Dominik Grewe wrote:
> Rather than returning perf-specific arguments, can we just return the two
paths?

Yep, I think we can just return the path to kallsyms.

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:108:
# If this our own library instead of a system one, look for a matching
On 2014/05/23 09:51:51, Dominik Grewe wrote:
> Nit: 'If this is our own...'

Oops, fixed.

https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:123:
os.stat(output_lib).st_mtime + 1):
On 2014/05/23 09:51:51, Dominik Grewe wrote:
> Maybe add a short comment explaining why the '+ 1' is necessary. Otherwise it
> looks like it might be there by accident.

Yeah, it does look a little out of place. Fixed.

[Dominik Grewe, 2014-05-27 10:24:48]

On 2014/05/27 10:14:58, Sami wrote:
> Thanks Dominik. All comments addressed.
> 
>
https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
> File
> tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py
> (right):
> 
>
https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
>
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:83:
> Prepares a set of files ("symfs") to be used with the perf tool for
> On 2014/05/23 09:51:51, Dominik Grewe wrote:
> > 'perf' -> 'profiling'? We want to use it for other profilers too.
> 
> Good point, done.
> 
>
https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
>
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:95:
> List of arguments to be passed to perf to point it to the created symfs.
> On 2014/05/23 09:51:51, Dominik Grewe wrote:
> > Rather than returning perf-specific arguments, can we just return the two
> paths?
> 
> Yep, I think we can just return the path to kallsyms.
> 
>
https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
>
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:108:
> # If this our own library instead of a system one, look for a matching
> On 2014/05/23 09:51:51, Dominik Grewe wrote:
> > Nit: 'If this is our own...'
> 
> Oops, fixed.
> 
>
https://codereview.chromium.org/290173006/diff/110001/tools/telemetry/telemet...
>
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:123:
> os.stat(output_lib).st_mtime + 1):
> On 2014/05/23 09:51:51, Dominik Grewe wrote:
> > Maybe add a short comment explaining why the '+ 1' is necessary. Otherwise
it
> > looks like it might be there by accident.
> 
> Yeah, it does look a little out of place. Fixed.

Thanks Sami. lgtm

[Sami, 2014-05-27 15:37:10]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-27 15:37:35]

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

[commit-bot: I haz the power, 2014-05-27 17:00:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Sami, 2014-05-27 17:03:59]

The CQ bit was unchecked by skyostil@chromium.org

[Sami, 2014-05-27 17:44:06]

I noticed after some more testing where we failed to download some libraries
because stderr and stdout output from perf was getting mixed. This version fixes
the bug and tests it with a more complete sample profile. PTAL!

[Dominik Grewe, 2014-05-28 08:51:57]

On 2014/05/27 17:44:06, Sami wrote:
> I noticed after some more testing where we failed to download some libraries
> because stderr and stdout output from perf was getting mixed. This version
fixes
> the bug and tests it with a more complete sample profile. PTAL!

lgtm

[Sami, 2014-05-28 09:22:10]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-28 09:22:27]

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

[commit-bot: I haz the power, 2014-05-28 10:54:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-28 11:07:28]

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

[commit-bot: I haz the power, 2014-05-28 11:07:29]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[Sami, 2014-05-28 12:27:14]

Looks like we can't rely on having perf on the build bots, so I've mocked it out
in the test that was using it. One more look please?

[Dominik Grewe, 2014-05-28 12:33:59]

On 2014/05/28 12:27:14, Sami wrote:
> Looks like we can't rely on having perf on the build bots, so I've mocked it
out
> in the test that was using it. One more look please?

lgtm

[Dominik Grewe, 2014-05-28 12:33:59]

On 2014/05/28 12:27:14, Sami wrote:
> Looks like we can't rely on having perf on the build bots, so I've mocked it
out
> in the test that was using it. One more look please?

lgtm

[Sami, 2014-05-28 12:36:44]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-05-28 12:37:53]

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

[commit-bot: I haz the power, 2014-05-28 16:11:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)

[commit-bot: I haz the power, 2014-05-28 16:24:40]

Change committed as 273261