telemetry: Improve perf profiler

telemetry: Improve perf profiler

This patch makes some improvements to the perf profiler in telemetry:

1. Add a prebuilt perf binary to be run on the Linux x86_64 host. This
   lets us rely on always having a recent perf binary present, allowing
   us to use source code annotation by default on Android. The binary
   also includes patches needed for importing perf data into Trace
   Viewer.

2. Use better defaults for running perf. Most importantly, sample at
   2000 Hz instead of the default 100 Hz.

3. On the Linux host, disable kernel symbol hiding for more complete
   symbolization. Note that this is also required by the new version of
   perf which this patch adds.

4. Turn on all CPUs on Android while running perf. This works around
   a bug in perf where samples from CPUs that were brought online while
   perf was running were not collected. The rationale for doing this
   only for perf instead of performance tests in general (to reduce noise)
   is that having all CPUs enabled for a long time is likely to lead to
   thermal throttling.

BUG=375754
TEST=tools/telemetry/run_tests --browser=android-content-shell TestAndroidProfilingHelper;
     tools/telemetry/run_tests --browser=android-content-shell TestPerfProfiler;
     build/android/chrome_profiler/run_tests

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

[Sami, 2014-06-05 11:00:21]

Here are a bunch of perf improvements -- some of which I lifted from Victor's
patch: https://codereview.chromium.org/296623002/. PTAL, thanks.

[tonyg, 2014-06-05 14:12:13]

A few nits, otherwise lg

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py
(right):

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:26:
def _GetOutDirectory():
I thought we had a better method for this in util. No?

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:195:
toolchain_config = 'arm-linux-androideabi'
 adb.system_properties['ro.product.cpu.abi']?

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:197:
host_arch = 'linux-x86_64'
I don't want to slow this patch down, but these really should come from
platform.

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:215:
online when recording is already underway.
Very interesting

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper_unittest.py
(right):

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper_unittest.py:60:
def _CheckIsAndroid(self):
Should these tests just use @test.Enabled('android')?

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:52:
os.path.abspath(support_binaries.FindPath('perfhost', 'linux'))
Don't we want to use platform.GetHostPlatform().InstallApplication('perfhost')?

[bulach, 2014-06-06 09:03:10]

drive-by :)

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py
(right):

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:215:
online when recording is already underway.
On 2014/06/05 14:12:14, tonyg wrote:
> Very interesting

indeed! :)
perhaps move this to
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

[Sami, 2014-06-06 09:53:50]

On 2014/06/06 09:03:10, bulach wrote:
> On 2014/06/05 14:12:14, tonyg wrote:
> > Very interesting
> 
> indeed! :)
> perhaps move this to
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

If you meant combining this with SetHighPerfMode, I thought about doing that but
shied away -- see point #4 in the commit message. I haven't actually tested it,
but my feeling is that pinning all cores online for a long time together with
the 'performance' governor setting could lead to Bad Things like the battery
discharging more quickly than USB can recharge it, to thermal throttling or even
the whole device shutting down. We could definitely test it, but perhaps in a
separate patch :)

If you just meant moving this code over to that class as a separate entrypoint,
then I'm all for it.

[bulach, 2014-06-06 12:30:38]

On 2014/06/06 09:53:50, Sami wrote:
> On 2014/06/06 09:03:10, bulach wrote:
> > On 2014/06/05 14:12:14, tonyg wrote:
> > > Very interesting
> > 
> > indeed! :)
> > perhaps move this to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
> 
> If you meant combining this with SetHighPerfMode, I thought about doing that
but
> shied away -- see point #4 in the commit message. I haven't actually tested
it,
> but my feeling is that pinning all cores online for a long time together with
> the 'performance' governor setting could lead to Bad Things like the battery
> discharging more quickly than USB can recharge it, to thermal throttling or
even
> the whole device shutting down. We could definitely test it, but perhaps in a
> separate patch :)
> 
> If you just meant moving this code over to that class as a separate
entrypoint,
> then I'm all for it.

yeah, I meant to keep as a separate entry point, so that all /sys/**/cpu
handling is in one place :)
sorry I wasn't clear..

[Sami, 2014-06-06 17:55:20]

Thanks, all nits eradicated. Another look please?

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py
(right):

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:26:
def _GetOutDirectory():
On 2014/06/05 14:12:14, tonyg wrote:
> I thought we had a better method for this in util. No?

Ah, there's util.GetBuildDirectories() -- thanks for the tip.

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:197:
host_arch = 'linux-x86_64'
On 2014/06/05 14:12:14, tonyg wrote:
> I don't want to slow this patch down, but these really should come from
> platform. 

Agreed, this is all kinds of ugly. I've now replaced all of the constants with
values derived from an ELF library.

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:215:
online when recording is already underway.
On 2014/06/06 09:03:10, bulach wrote:
> On 2014/06/05 14:12:14, tonyg wrote:
> > Very interesting
> 
> indeed! :)
> perhaps move this to
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
> 
> 

Done.

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper_unittest.py
(right):

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper_unittest.py:60:
def _CheckIsAndroid(self):
On 2014/06/05 14:12:14, tonyg wrote:
> Should these tests just use @test.Enabled('android')?

Neat, thanks!

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
File tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py (right):

https://codereview.chromium.org/316143002/diff/1/tools/telemetry/telemetry/co...
tools/telemetry/telemetry/core/platform/profiler/perf_profiler.py:52:
os.path.abspath(support_binaries.FindPath('perfhost', 'linux'))
On 2014/06/05 14:12:14, tonyg wrote:
> Don't we want to use
platform.GetHostPlatform().InstallApplication('perfhost')?

Man, there's a function in telemetry for everything :) Done.

[bulach, 2014-06-09 10:06:52]

lgtm, but no longer an owner... just a small suggestion below:

https://codereview.chromium.org/316143002/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py
(right):

https://codereview.chromium.org/316143002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:204:
architecture and toolchain.
hmm...
I suppose we require md5sum_device to be built, so maybe omit this param and
just do the readelf on os.path.join(constants.GetOutDirectory(),
md5sum_device_bin) ?

alternatively, could also inspect getprop ro.product.cpu.abi?

[Sami, 2014-06-09 11:39:02]

Thanks Marcus!

https://codereview.chromium.org/316143002/diff/40001/tools/telemetry/telemetr...
File
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py
(right):

https://codereview.chromium.org/316143002/diff/40001/tools/telemetry/telemetr...
tools/telemetry/telemetry/core/platform/profiler/android_profiling_helper.py:204:
architecture and toolchain.
On 2014/06/09 10:06:52, bulach wrote:
> hmm...
> I suppose we require md5sum_device to be built, so maybe omit this param and
> just do the readelf on os.path.join(constants.GetOutDirectory(),
> md5sum_device_bin) ?
>
> alternatively, could also inspect getprop ro.product.cpu.abi?

That's a great idea, but I think it has the same problem that made me decide not
to use ro.product.cpu.abi: the device can support multiple ABIs and its not
clear which one to return here. For instance on x86 devices ro.product.cpu.abi
is most likely ARM and ro.product.cpu.abi2 is x86.

Since you generally want to use a toolchain binary with something that came out
of the toolchain (i.e., an ELF file), I think it makes sense to make this
function dependent on that information.

By the way, I'm not if we need to build our own md5sum anymore since we have
/system/bin/md5. Probably not on ICS though:
https://android.googlesource.com/platform/system/core/+/d0455c952d61d36e662f4...

[tonyg, 2014-06-09 15:58:09]

lgtm

https://codereview.chromium.org/316143002/diff/40001/build/android/pylib/perf...
File build/android/pylib/perf/perf_control.py (right):

https://codereview.chromium.org/316143002/diff/40001/build/android/pylib/perf...
build/android/pylib/perf/perf_control.py:87: time.sleep(1)
Should we consider polling more often? 1s delay is noticeable.

[Sami, 2014-06-09 17:11:11]

On 2014/06/09 at 15:58:09, tonyg wrote:
> lgtm
> 
>
https://codereview.chromium.org/316143002/diff/40001/build/android/pylib/perf...
> File build/android/pylib/perf/perf_control.py (right):
> 
>
https://codereview.chromium.org/316143002/diff/40001/build/android/pylib/perf...
> build/android/pylib/perf/perf_control.py:87: time.sleep(1)
> Should we consider polling more often? 1s delay is noticeable.

Yes, good idea. Bumped this to 10 Hz.

[Sami, 2014-06-09 17:11:45]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-06-09 17:12:29]

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

[commit-bot: I haz the power, 2014-06-10 08:56:16]

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

[commit-bot: I haz the power, 2014-06-10 11:31:08]

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

[commit-bot: I haz the power, 2014-06-10 11:31:09]

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

[Sami, 2014-06-10 12:27:36]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-06-10 12:29:18]

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

[commit-bot: I haz the power, 2014-06-10 14:11:08]

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

[commit-bot: I haz the power, 2014-06-10 14:34:34]

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

[commit-bot: I haz the power, 2014-06-10 14:34:35]

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

[Sami, 2014-06-10 15:26:54]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2014-06-10 15:28:43]

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

[commit-bot: I haz the power, 2014-06-10 16:02:04]

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

[commit-bot: I haz the power, 2014-06-10 16:18:44]

Change committed as 276049