Last Comment Bug 771103 - Support for externally enabling/disabling the profiler
: Support for externally enabling/disabling the profiler
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Mike Hommey [:glandium]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-05 05:22 PDT by Mike Hommey [:glandium]
Modified: 2012-07-12 09:39 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allow to start the profiler with a signal on Linux/Android (3.75 KB, patch)
2012-07-10 07:41 PDT, Mike Hommey [:glandium]
bgirard: review+
Details | Diff | Review
Allow to start the profiler with a signal on Android (3.98 KB, patch)
2012-07-10 08:48 PDT, Mike Hommey [:glandium]
bgirard: review+
blassey.bugs: review+
Details | Diff | Review

Description Mike Hommey [:glandium] 2012-07-05 05:22:15 PDT
There is a signal handler to start/stop the profiler for linux (which, actually, only starts the profiler), and it's only enabled on android with libunwind. I think this should be generalized, and that the signal handler should actually be a switch.
Comment 1 Mike Hommey [:glandium] 2012-07-10 07:41:44 PDT
Created attachment 640601 [details] [diff] [review]
Allow to start the profiler with a signal on Linux/Android

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=35f87330272a
Comment 2 Mike Hommey [:glandium] 2012-07-10 07:43:17 PDT
Note that i slipped a fix when restoring the save signal handler
Comment 3 Benoit Girard (:BenWa) 2012-07-10 07:57:49 PDT
Comment on attachment 640601 [details] [diff] [review]
Allow to start the profiler with a signal on Linux/Android

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

r+ with the return statement restored for libunwind. But this needs additional review for adding the signal handler for release builds.

::: tools/profiler/TableTicker.cpp
@@ +837,3 @@
>  
> +  // Allow the profiler to be started using signals
> +  OS::RegisterStartHandler();

We may want super review on adding a signal handler on linux + mobile to release builds.

@@ -838,5 @@
> -  OS::RegisterStartStopHandlers();
> -
> -  // On Android, this is too soon in order to start up the
> -  // profiler.
> -  return;

You remove the return here for android + libunwind but it's not safe to check MOZ_PROFILER_STARTUP. See the comment below. Maybe we should just move the signal handler inside the #ifdef
Comment 4 Mike Hommey [:glandium] 2012-07-10 08:08:32 PDT
(In reply to Benoit Girard (:BenWa) from comment #3)
> Comment on attachment 640601 [details] [diff] [review]
> Allow to start the profiler with a signal on Linux/Android
> 
> Review of attachment 640601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the return statement restored for libunwind. But this needs
> additional review for adding the signal handler for release builds.
> 
> ::: tools/profiler/TableTicker.cpp
> @@ +837,3 @@
> >  
> > +  // Allow the profiler to be started using signals
> > +  OS::RegisterStartHandler();
> 
> We may want super review on adding a signal handler on linux + mobile to
> release builds.
> 
> @@ -838,5 @@
> > -  OS::RegisterStartStopHandlers();
> > -
> > -  // On Android, this is too soon in order to start up the
> > -  // profiler.
> > -  return;
> 
> You remove the return here for android + libunwind but it's not safe to
> check MOZ_PROFILER_STARTUP. See the comment below.

The only comment below talks about it not being possible to use the prefs service to get a pref, so it reads an env variable instead. That doesn't say anything about it being unsafe for android + libunwind.

> Maybe we should just move the signal handler inside the #ifdef

The point was to move it out of the ifdef, where it currently is. It could be limited to mobile, though.
Comment 5 Mike Hommey [:glandium] 2012-07-10 08:48:40 PDT
Created attachment 640616 [details] [diff] [review]
Allow to start the profiler with a signal on Android

Make it Android-only, and reinstate the return where it actually matters, with a more precise comment as to why this is required.

Adding blassey to the loop for the addition of a signal handler for SIGUSR1 on Android.
Comment 6 Benoit Girard (:BenWa) 2012-07-10 09:41:23 PDT
Comment on attachment 640616 [details] [diff] [review]
Allow to start the profiler with a signal on Android

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

r+

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