771103 - Support for externally enabling/disabling the profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Mike Hommey [:glandium]]

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]]

Attachment: 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]]

Note that i slipped a fix when restoring the save signal handler

Comment 3

[Benoit Girard (:BenWa)]

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]]

(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]]

Attachment: 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)]

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+

Comment 7

[Mike Hommey [:glandium]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/31388635897c

Comment 8

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3d448b4f39

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/31388635897c
https://hg.mozilla.org/mozilla-central/rev/8f3d448b4f39