734302 - Enable the Gecko Profiler on native Fennec

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[(no longer active)]

I have patches to enable Gecko Profiler on Native Fennec using a custom version of libunwind.

Comment 1

[(no longer active)]

Attachment: Part 1: Support stack unwinding using libunwind on Android ARM

Requesting review from Kyle on the build system changes, and from Benoit on the rest.

Comment 2

[(no longer active)]

Attachment: Import libunwind into mozilla-central

This is upstream libunwind code with my changes.

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 604429 [details] [diff] [review]
Part 1: Support stack unwinding using libunwind on Android ARM

Review of attachment 604429 [details] [diff] [review]:
-----------------------------------------------------------------

Excellent! \o/ Great work Ehsan!

::: tools/profiler/TableTicker.cpp
@@ +329,5 @@
>    {
> +#if defined(USE_LIBUNWIND) && defined(ANDROID)
> +    // We don't have the Gecko Profiler add-on on Android, but we know that
> +    // libunwind is available, so we can always walk the stacks.
> +    mUseStackWalk = true;

I don't like this but I'll fix it later. Ideally I want the profiler to use non stackwalking by default because it has a very deterministic behavior.

@@ +504,5 @@
> +  unw_cursor_t cursor; unw_context_t uc;
> +  unw_word_t ip;
> +  unw_getcontext(&uc);
> +
> +  // Dirty hack: replace the registers with values from the signal handler

I don't really think it's a dirty hack :)

@@ +761,5 @@
>  
>  const char** mozilla_sampler_get_features()
>  {
>    static const char* features[] = {
> +#if defined(MOZ_PROFILING) && (defined(USE_BACKTRACE) || defined(USE_NS_STACKWALK) || defined(USE_LIBUNWIND))

I'm assuming && binds binds over ||, but I would rather see () used.

::: tools/profiler/android-signal-defs.h
@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

MPL 2.0

Comment 5

[(no longer active)]

Attachment: Part 3: Add proper UI for toggling the profiler

This adds a Toggle Profiling menu item to the profiling builds which toggles the profiler.  It's an easy way on mobile to use the profiler.  This menu item will never be in a build which we ship to users.

Asking review from mfinkle on mobile parts, and from BenWa on the profiler parts.

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 606641 [details] [diff] [review]
Part 3: Add proper UI for toggling the profiler

Review of attachment 606641 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/Makefile.in
@@ +59,5 @@
>  SYNC_RES_VALUES=$(shell cat $(topsrcdir)/mobile/android/sync/android-values-resources.mn | tr '\n' ' ';)
>  SYNC_RES_XML=res/xml/sync_authenticator.xml
>  SYNC_PP_RES_XML=res/xml/sync_syncadapter.xml res/xml/sync_options.xml
>  
> +MENU_PP_RES_XML=res/menu/gecko_menu.xml res/menu-v11/gecko_menu.xml

I don't follow the changes in this file, mfinkle?

::: mobile/android/chrome/content/browser.js
@@ +195,5 @@
>      Services.obs.addObserver(this, "Viewport:Change", false);
>      Services.obs.addObserver(this, "SearchEngines:Get", false);
>      Services.obs.addObserver(this, "Passwords:Init", false);
>      Services.obs.addObserver(this, "FormHistory:Init", false);
> +    Services.obs.addObserver(this, "ToggleProfiling", false);

remove the observer

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 606641 [details] [diff] [review]
Part 3: Add proper UI for toggling the profiler

lgtm

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 606641 [details] [diff] [review]
> Part 3: Add proper UI for toggling the profiler
> 
> lgtm

With Benoit's 'observer' change addressed

Comment 9

[(no longer active)]

Do we really want to remove the observer?  None of the other global observers are removed.  Removing weak observers is really pointless, as the observer service itself knows how to clean them up once the observer object dies.

Comment 10

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=9f5a208e56c2

Comment 11

[Benoit Girard (:BenWa)]

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> Do we really want to remove the observer?  None of the other global
> observers are removed.  Removing weak observers is really pointless, as the
> observer service itself knows how to clean them up once the observer object
> dies.

Yea, don't worry about it

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f4dfa861e10
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66090bcddbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/c61855cb4558

Comment 13

[Matt Brubeck (:mbrubeck)]

Sorry, I had to back out part 3 on inbound because it fails to build when MOZ_PROFILING is not defined.
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd849d439d6c

Looks like you need "#ifdef MOZ_PROFILING" around "case R.id.toggle_profiling" in GeckoApp.java:
https://tbpl.mozilla.org/php/getParsedLog.php?id=10255923&tree=Mozilla-Inbound

Comment 14

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1f4dfa861e10
https://hg.mozilla.org/mozilla-central/rev/c66090bcddbe

Comment 15

[(no longer active)]

Attachment: Part 3: Add proper UI for toggling the profiler

This should fix the build errors.

Comment 16

[(no longer active)]

Attachment: Part 4: Fix the profiling builds on Mac and Windows

Comment 17

[(no longer active)]

Comment on attachment 608366 [details] [diff] [review]
Part 4: Fix the profiling builds on Mac and Windows

https://hg.mozilla.org/mozilla-central/rev/5c733c42bc44

Comment 18

[(no longer active)]

Attachment: Part 3: Add proper UI for toggling the profiler

This version of the patch moves the preprocessed code from GeckoApp.java to App.java.in.

Comment 19

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11b6f37603ce

This bug can be closed when this patch gets merged.  Thanks!

Comment 20

[(no longer active)]

(In reply to Ehsan Akhgari [:ehsan] from comment #19)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/11b6f37603ce
> 
> This bug can be closed when this patch gets merged.  Thanks!

Backed out, cause I don't know much Java:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b594b5c7b0b8

Comment 21

[(no longer active)]

Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/b20113842765

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b20113842765

Comment 23

[Benoit Girard (:BenWa)]

\o\ \o/ /o/

Comment 24

[Aaron Train [:aaronmt]]

It looks like MOZ_PROFILING got auto-enabled on the branded Nightly builds http://hg.mozilla.org/mozilla-central/rev/b20113842765 is that intentional? (i.e, you can see this on Nightly (03/17).

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Aaron Train [:aaronmt] from comment #24)
> It looks like MOZ_PROFILING got auto-enabled on the branded Nightly builds
> http://hg.mozilla.org/mozilla-central/rev/b20113842765 is that intentional?
> (i.e, you can see this on Nightly (03/17).

No. We need to find what is triggering MOZ_PROFILING.

Comment 26

[(no longer active)]

(In reply to Mark Finkle (:mfinkle) from comment #25)
> (In reply to Aaron Train [:aaronmt] from comment #24)
> > It looks like MOZ_PROFILING got auto-enabled on the branded Nightly builds
> > http://hg.mozilla.org/mozilla-central/rev/b20113842765 is that intentional?
> > (i.e, you can see this on Nightly (03/17).
> 
> No. We need to find what is triggering MOZ_PROFILING.

That is a mystery to me.  I don't see anything in the mozconfig, and this is very recent.  Maybe it's a regression from the new stackwalking code somehow?

Comment 27

[Aaron Train [:aaronmt]]

Whoops forgot to update this bug, the issue turned out to be bug 851920.