Support for externally enabling/disabling the profiler

RESOLVED FIXED in mozilla16

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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
Attachment #640601 - Flags: review?(bgirard)
Note that i slipped a fix when restoring the save signal handler
Assignee: nobody → mh+mozilla
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
Attachment #640601 - Flags: review?(bgirard) → review+
(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.
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.
Attachment #640616 - Flags: review?(blassey.bugs)
Attachment #640616 - Flags: review?(bgirard)
Attachment #640601 - Attachment is obsolete: true
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+
Attachment #640616 - Flags: review?(bgirard) → review+
Attachment #640616 - Flags: review?(blassey.bugs) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/31388635897c
Target Milestone: --- → mozilla16
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3d448b4f39
https://hg.mozilla.org/mozilla-central/rev/31388635897c
https://hg.mozilla.org/mozilla-central/rev/8f3d448b4f39
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.