Make profiling=1 work for chromium-android

Make profiling=1 work for chromium-android
Currently if you set profiling=1 content_shell will load,
it will just load to a white screen (and attempts to sample
the binary with perf will not get proper stacks).
This adds -mapcs and -marm per the AndroidPerf wiki docs.


BUG=None


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

[abarth-chromium, 2012-11-28 21:51:59]

LGTM

[abarth-chromium, 2012-11-28 21:52:14]

Obviously we need someone who knows what they're doing to review this patch.  :)

[tony, 2012-11-28 21:53:54]

LGTM2, would be nice to have a skia OWNER look too.

[Nico, 2012-11-28 21:55:09]

Link to the android perfwiki docs?

[abarth-chromium, 2012-11-28 21:56:49]

On 2012/11/28 21:55:09, Nico wrote:
> Link to the android perfwiki docs?

Unfortunately, it's still on the Google corp network.  We're working on a public
version of it that will use this mechanism.  :)

[eseidel, 2012-11-28 22:04:06]

On 2012/11/28 21:56:49, abarth wrote:
> On 2012/11/28 21:55:09, Nico wrote:
> > Link to the android perfwiki docs?
> 
> Unfortunately, it's still on the Google corp network.  We're working on a
public
> version of it that will use this mechanism.  :)

This is the relevant bit from main-wiki/AndroidPerf:

Command: perf record

Like OProfile, perf also provides a way to store samples in a file for detailed
analysis later. The following command will collect 1 second worth of profile
samples from the spinloop process and store them under /data/perf.data:

> adb shell perf record -p 1635 -g sleep 1

Couldn't record kernel reference relocation symbol
Symbol resolution may be skewed if relocation was used (e.g. kexec).
Check /proc/kallsyms permission or run as root.
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.023 MB /data/perf.data (~1026 samples) ]
The -g option will store the sampled callgraph info in the record as well.
However, the binary has to be compiled with the the non-default
-fno-omit-frame-pointer -marm -mapcs options as listed in the Android.mk file
for the spinloop benchmark.

[Peter Beverloo, 2012-11-28 22:05:15]

+digit as he knows next to everything about flags for Android.
+tomhudson, as he's been doing a lot of Skia performance work lately (though is
in the LON timezone).

lgtm; assuming you tested this.

[digit1, 2012-11-28 22:36:49]

https://codereview.chromium.org/11316228/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/11316228/diff/1/build/common.gypi#newcode2610
build/common.gypi:2610: '-mapcs', # Seems required for -fno-omit-frame-pointer
Can I ask you to change this to -mapcs-frame, it is equivalent but slightly less
ambiguous.

Also, do you need to remove -fno-omit-frame-pointer too to ensure that stack
frames are generated for leaf functions?

[eseidel, 2012-11-28 22:39:36]

On 2012/11/28 22:36:49, digit1 wrote:
> https://codereview.chromium.org/11316228/diff/1/build/common.gypi
> File build/common.gypi (right):
> 
> https://codereview.chromium.org/11316228/diff/1/build/common.gypi#newcode2610
> build/common.gypi:2610: '-mapcs', # Seems required for -fno-omit-frame-pointer
> Can I ask you to change this to -mapcs-frame, it is equivalent but slightly
less
> ambiguous.

Sure.

> Also, do you need to remove -fno-omit-frame-pointer too to ensure that stack
> frames are generated for leaf functions?

That's already done in the common profiling=1 path.

[commit-bot: I haz the power, 2012-11-29 00:41:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/11316228/9002

[commit-bot: I haz the power, 2012-11-29 00:41:58]

Presubmit check for 11316228-9002 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    skia

[Nico, 2012-11-29 00:44:57]

https://codereview.chromium.org/11316228/diff/1/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/11316228/diff/1/build/common.gypi#newcode2610
build/common.gypi:2610: '-mapcs', # Seems required for -fno-omit-frame-pointer
On 2012/11/28 22:36:49, digit1 wrote:
> Can I ask you to change this to -mapcs-frame, it is equivalent but slightly
less
> ambiguous.

Looks like this didn't happen?

[Nico, 2012-11-29 00:47:08]

lgtm

Sorry, I don't get computers.

[commit-bot: I haz the power, 2012-11-29 00:52:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/11316228/9002

[Tom Hudson, 2012-11-29 10:56:24]

After-the-commit, but I don't even know what profiling=1 does, never needed to
use it 6 months ago when I was regularly profiling Chrome on Desktop.

+djsollen@ (in US EST) as a likely person to know about the Android/ARM compile
flags?

[djsollen1, 2012-11-29 13:16:28]

Those flags are needed to get the call-graph feature working for perf on
Android. I just recently uploaded a similar CL into the gyp files in Skia's
repository to support this.

[eseidel, 2012-11-29 16:03:36]

On 2012/11/29 13:16:28, djsollen1 wrote:
> Those flags are needed to get the call-graph feature working for perf on
> Android. I just recently uploaded a similar CL into the gyp files in Skia's
> repository to support this.

I assume that CL is still pending review?

[eseidel, 2012-11-29 16:09:05]

On 2012/11/29 10:56:24, Tom Hudson wrote:
> After-the-commit, but I don't even know what profiling=1 does, never needed to
> use it 6 months ago when I was regularly profiling Chrome on Desktop.
> 
> +djsollen@ (in US EST) as a likely person to know about the Android/ARM
compile
> flags?

My understanding is that it depends on the platform currently.  I'm told Dave
Moore added profiling=1 as an attempt to unify across platforms.  But still you
have to set mac_strip=0, or linux_no_pie=1 depending on your platform, so many
profiling=1 is currently a win-only feature. :)  This patch is just an attempt
to make it possible to get callstacks on android, and I'm using profiling=1 to
do that (instead of adding a new profiling_android=1 or whatever).

If you're still doing profiling (on desktop or android) I'd love to chat more. 
I'm sure there is much I could gather from your experience.

[djsollen1, 2012-11-29 16:09:49]

On 2012/11/29 16:03:36, Eric Seidel (Google) wrote:
> On 2012/11/29 13:16:28, djsollen1 wrote:
> > Those flags are needed to get the call-graph feature working for perf on
> > Android. I just recently uploaded a similar CL into the gyp files in Skia's
> > repository to support this.
> 
> I assume that CL is still pending review?

No it went into a gyp file in the Skia repository this morning, but that won't
impact chromium at all as it uses it's own gyp (the one modified by this
change).

[eseidel, 2012-11-29 16:12:24]

W/o this patch if you just run perf (with profiling=0) you will get hot-spot
addresses (which you can symbolicate), but perf is unable to crawl the stack and
give you backtraces.

If you add profiling=1 w/o this patch then content_shell will just get confused
and either draw a blank screen or crash.

After this patch profiling=1 is an easy way to be able to get callstacks when
you run perf with -g.

We're currently trying to lower the barrier to entry for profiling anywhere for
WebKit, and particularly on devices (where it's currently a PITA). 
https://bugs.webkit.org/show_bug.cgi?id=103587 is one such example of that
effort.

[eseidel, 2012-11-29 16:13:49]

On 2012/11/29 16:09:49, djsollen1 wrote:
> On 2012/11/29 16:03:36, Eric Seidel (Google) wrote:
> > On 2012/11/29 13:16:28, djsollen1 wrote:
> > > Those flags are needed to get the call-graph feature working for perf on
> > > Android. I just recently uploaded a similar CL into the gyp files in
Skia's
> > > repository to support this.
> > 
> > I assume that CL is still pending review?
> 
> No it went into a gyp file in the Skia repository this morning, but that won't
> impact chromium at all as it uses it's own gyp (the one modified by this
> change).

Ah... maybe this explains why it took me so long to figure out why changing
opts/opts.gypi to have these lines didn't change my build! :)

[commit-bot: I haz the power, 2012-11-29 16:14:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/eseidel@chromium.org/11316228/9002

[commit-bot: I haz the power, 2012-11-29 22:00:43]

Change committed as 170249