Enable the Gecko Profiler on native Fennec

RESOLVED FIXED in mozilla14

Status

()

Core
Gecko Profiler
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla14
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

I have patches to enable Gecko Profiler on Native Fennec using a custom version of libunwind.
Created attachment 604429 [details] [diff] [review]
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.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #604429 - Flags: review?(khuey)
Attachment #604429 - Flags: review?(bgirard)
Created attachment 604430 [details]
Import libunwind into mozilla-central

This is upstream libunwind code with my changes.
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
Attachment #604429 - Flags: review?(bgirard) → review+

Updated

6 years ago
Duplicate of this bug: 713234
Created attachment 606641 [details] [diff] [review]
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.
Attachment #606641 - Flags: review?(mark.finkle)
Attachment #606641 - Flags: review?(bgirard)
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
Attachment #606641 - Flags: review?(bgirard) → review+
Comment on attachment 606641 [details] [diff] [review]
Part 3: Add proper UI for toggling the profiler

lgtm
Attachment #606641 - Flags: review?(mark.finkle) → review+
(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
Attachment #604429 - Flags: review?(khuey) → review+
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.
https://tbpl.mozilla.org/?tree=Try&rev=9f5a208e56c2
(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
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
Target Milestone: --- → mozilla14
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
Whiteboard: [leave open after inbound merge]
Target Milestone: mozilla14 → ---
https://hg.mozilla.org/mozilla-central/rev/1f4dfa861e10
https://hg.mozilla.org/mozilla-central/rev/c66090bcddbe
Created attachment 608337 [details] [diff] [review]
Part 3: Add proper UI for toggling the profiler

This should fix the build errors.
Attachment #606641 - Attachment is obsolete: true
Attachment #608337 - Flags: review?(doug.turner)

Updated

5 years ago
Attachment #608337 - Flags: review?(doug.turner) → review?(blassey.bugs)
Created attachment 608366 [details] [diff] [review]
Part 4: Fix the profiling builds on Mac and Windows
Attachment #608366 - Flags: review?(bgirard)

Updated

5 years ago
Attachment #608366 - Flags: review?(bgirard) → review+
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
Attachment #608366 - Flags: checkin+
Whiteboard: [leave open after inbound merge]
Attachment #604429 - Flags: checkin+
Attachment #604430 - Flags: checkin+
Created attachment 608770 [details] [diff] [review]
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.
Attachment #608337 - Attachment is obsolete: true
Attachment #608770 - Flags: review?(blassey.bugs)
Attachment #608337 - Flags: review?(blassey.bugs)
Attachment #608770 - Flags: review?(blassey.bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/11b6f37603ce

This bug can be closed when this patch gets merged.  Thanks!
(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
Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/b20113842765
https://hg.mozilla.org/mozilla-central/rev/b20113842765
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
\o\ \o/ /o/
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).
(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.
(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?
Flags: needinfo?(bgirard)
Whoops forgot to update this bug, the issue turned out to be bug 851920.

Updated

4 years ago
Flags: needinfo?(bgirard)
You need to log in before you can comment on or make changes to this bug.