Last Comment Bug 734302 - Enable the Gecko Profiler on native Fennec
: Enable the Gecko Profiler on native Fennec
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla14
Assigned To: :Ehsan Akhgari
:
Mentors:
: 713234 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-08 18:03 PST by :Ehsan Akhgari
Modified: 2013-03-19 08:42 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Support stack unwinding using libunwind on Android ARM (24.66 KB, patch)
2012-03-09 09:02 PST, :Ehsan Akhgari
bgirard: review+
khuey: review+
ehsan: checkin+
Details | Diff | Splinter Review
Import libunwind into mozilla-central (1013.55 KB, application/gzip)
2012-03-09 09:05 PST, :Ehsan Akhgari
ehsan: checkin+
Details
Part 3: Add proper UI for toggling the profiler (13.45 KB, patch)
2012-03-16 11:10 PDT, :Ehsan Akhgari
mark.finkle: review+
bgirard: review+
Details | Diff | Splinter Review
Part 3: Add proper UI for toggling the profiler (12.88 KB, patch)
2012-03-22 08:14 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Part 4: Fix the profiling builds on Mac and Windows (895 bytes, patch)
2012-03-22 09:39 PDT, :Ehsan Akhgari
bgirard: review+
ehsan: checkin+
Details | Diff | Splinter Review
Part 3: Add proper UI for toggling the profiler (11.93 KB, patch)
2012-03-23 10:53 PDT, :Ehsan Akhgari
blassey.bugs: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-03-08 18:03:11 PST
I have patches to enable Gecko Profiler on Native Fennec using a custom version of libunwind.
Comment 1 :Ehsan Akhgari 2012-03-09 09:02:34 PST
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.
Comment 2 :Ehsan Akhgari 2012-03-09 09:05:00 PST
Created attachment 604430 [details]
Import libunwind into mozilla-central

This is upstream libunwind code with my changes.
Comment 3 Benoit Girard (:BenWa) 2012-03-09 14:38:27 PST
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 4 Benoit Girard (:BenWa) 2012-03-10 17:32:39 PST
*** Bug 713234 has been marked as a duplicate of this bug. ***
Comment 5 :Ehsan Akhgari 2012-03-16 11:10:07 PDT
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.
Comment 6 Benoit Girard (:BenWa) 2012-03-16 11:32:35 PDT
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?) 2012-03-16 12:07:15 PDT
Comment on attachment 606641 [details] [diff] [review]
Part 3: Add proper UI for toggling the profiler

lgtm
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-16 12:08:31 PDT
(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 :Ehsan Akhgari 2012-03-16 15:07:38 PDT
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 11 Benoit Girard (:BenWa) 2012-03-19 10:23:01 PDT
(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 13 Matt Brubeck (:mbrubeck) 2012-03-21 16:30:29 PDT
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 15 :Ehsan Akhgari 2012-03-22 08:14:43 PDT
Created attachment 608337 [details] [diff] [review]
Part 3: Add proper UI for toggling the profiler

This should fix the build errors.
Comment 16 :Ehsan Akhgari 2012-03-22 09:39:24 PDT
Created attachment 608366 [details] [diff] [review]
Part 4: Fix the profiling builds on Mac and Windows
Comment 17 :Ehsan Akhgari 2012-03-22 09:46:25 PDT
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 :Ehsan Akhgari 2012-03-23 10:53:53 PDT
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.
Comment 19 :Ehsan Akhgari 2012-03-23 11:14:34 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/11b6f37603ce

This bug can be closed when this patch gets merged.  Thanks!
Comment 20 :Ehsan Akhgari 2012-03-23 12:04:36 PDT
(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 22 Ed Morley [:emorley] 2012-03-24 13:43:43 PDT
https://hg.mozilla.org/mozilla-central/rev/b20113842765
Comment 23 Benoit Girard (:BenWa) 2012-03-24 13:58:10 PDT
\o\ \o/ /o/
Comment 24 Aaron Train [:aaronmt] 2013-03-17 08:10:45 PDT
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?) 2013-03-17 08:26:22 PDT
(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 :Ehsan Akhgari 2013-03-19 08:41:20 PDT
(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] 2013-03-19 08:42:29 PDT
Whoops forgot to update this bug, the issue turned out to be bug 851920.

Note You need to log in before you can comment on or make changes to this bug.