853358 - Add plugin profiling support

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Currently when we want to do multi-process profiling we contact and signal each process manually and run a script to combine the profiling. It should be easy to use an IPC message to do this ourselves.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch

Asking ehsan for profiler changes, benjamin for plugins.

Comment 2

[(no longer active)]

Comment on attachment 727571 [details] [diff] [review]
patch

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +1738,5 @@
> +PluginProfilerObserver::Observe(nsISupports *aSubject,
> +                                const char *aTopic,
> +                                const PRUnichar *aData)
> +{
> +    ProfileSaveEvent* pse = (ProfileSaveEvent*)aSubject;

No, you cannot guarantee that this will be safe since anyone can send this notification.  Please add an XPCOM interface and implement it properly.

::: tools/profiler/GeckoProfilerImpl.h
@@ +295,5 @@
>    }
>    stack->addMarker(aMarker);
>  }
>  
> +class ProfileSaveEvent MOZ_FINAL : public nsISupports {

Please move this to TableTicker.cpp.

::: tools/profiler/TableTicker.cpp
@@ +167,5 @@
> +  JSAObjectBuilder* mBuilder;
> +  JSCustomArray* mThreads;
> +};
> +
> +void subprocess_callback(const char* aProfile, void* aClosure)

Nit: SubProcessCallback

@@ +207,5 @@
> +  closure.mThreads = threads;
> +
> +  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +  if (os) {
> +    os->NotifyObservers(new ProfileSaveEvent(subprocess_callback, &closure), "profiler-subprocess", nullptr);

ProfileSaveEvent is an XPCOM class, so you should hold on to it in a local nsRefPtr.  Otherwise, if an Observe implementation puts it into an nsCOMPtr, for example, the object will be destroyed prematurely when that nsCOMPtr is destroyed (since Release gets called which gets the refcount to 0.)

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 4

[(no longer active)]

Comment on attachment 729556 [details] [diff] [review]
patch

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

Can you please ask somebody else to look at the plugin changes?  I'll try to get to this review some time this week but that may not be very likely.  Sorry. :(

Comment 5

[Benoit Girard (:BenWa)]

Comment on attachment 729556 [details] [diff] [review]
patch

Yes, I just wanted to let you get a first pass before asking for the plugin bits to be reviewed.

Comment 6

[Benjamin Smedberg]

Comment on attachment 729556 [details] [diff] [review]
patch

I wonder if it wouldn't make more sense to write the profile to a temporary file. But either way, r=me on the plugin bits

Comment 7

[Benoit Girard (:BenWa)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 729556 [details] [diff] [review]
> patch
> 
> I wonder if it wouldn't make more sense to write the profile to a temporary
> file.

Have the plugin process write it's data to a file and have the parent process load it back up? It feels like that would be more flaky.

Comment 8

[(no longer active)]

Comment on attachment 729556 [details] [diff] [review]
patch

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

::: tools/profiler/Makefile.in
@@ +47,5 @@
>    platform.cpp \
>    nsProfilerFactory.cpp \
>    nsProfiler.cpp \
>    TableTicker.cpp \
> +	SaveProfileTask.cpp \

Nit: use spaces please.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch rebased

Comment 10

[Benoit Girard (:BenWa)]

Attachment: patch rebased + warning fix

Comment 11

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=f438836e66a4

Comment 12

[Benoit Girard (:BenWa)]

(patch queue had a bad patch inside of it):
https://tbpl.mozilla.org/?tree=Try&rev=0f89c96118c1

Comment 13

[Benoit Girard (:BenWa)]

Attachment: patch rebased again

non trivial rebase again:
https://tbpl.mozilla.org/?tree=Try&rev=c90e6080f55a

Comment 14

[Benoit Girard (:BenWa)]

Attachment: patch rebased again

https://tbpl.mozilla.org/?tree=Try&rev=385d70d26ebb

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 741427 [details] [diff] [review]
patch rebased again

I never got review on the plugin part.

Comment 16

[Benoit Girard (:BenWa)]

Comment on attachment 741427 [details] [diff] [review]
patch rebased again

Didn't see that I already got review.

Comment 17

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c759d3eb1118

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c759d3eb1118