1008435 - [e10s] Port the built-in Gecko profiler to e10s

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

+++ This bug was initially created as a clone of Bug #995683 +++

The profiler will help us solve bugs like 995683. We need to add IPC messages to collect the sub profiles and tweak the symbolication scripts.

Comment 1

[Jim Mathies [:jimm]]

re-noming for a discussion about how important this is for m2.

Comment 2

[Benoit Girard (:BenWa)]

Blocking bug 905436 since it's the proper pre-req for the profiler extension to support profiling.

I don't believe this work is required for devtools (content) profiling.

cpeterson if you want to update arewee10syet.com a better link is:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler
which explains and as a link to the latest XPI.

Please make it depend this bug.

Comment 3

[Chris Peterson [:cpeterson]]

I updated the website to point to this bug and that MDN link.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: [WIP] Port the built-in Gecko profiler to e10s. r=?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Attachment: [WIP] Port the built-in Gecko profiler to e10s. r=?

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: [WIP] Let the Gecko Profiler work with child processes. r=?

We now allow profiling the content process for e10s, and plugin processes.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let the Gecko Profiler work with child processes. r=?

We now allow profiling the content process for e10s, and plugin processes.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let the Gecko Profiler work with child processes. r=?

We now allow profiling the content process for e10s, and plugin processes.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8524727 [details] [diff] [review]
Let the Gecko Profiler work with child processes. r=?

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

BenWa said he was comfortable reviewing most of this, but wanted a second pair of eyes for the IDL / XPCOM stuff I'm doing here. smaug, can you be that pair of eyes?

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +2365,5 @@
> +                                     const double& aInterval,
> +                                     const nsTArray<nsCString>& aFeatures,
> +                                     const nsTArray<nsCString>& aThreadNameFilters)
> +{
> +    nsTArray<const char*> featureArray;

It kinda blows that I'm copying this stuff from ContentChild.cpp. I'm not sure if it's worth creating some kind of mix-in that ContentChild.cpp / PluginModuleChild.cpp can share.

Same goes with the ContentParent / PluginModuleParent stuff.

::: tools/profiler/nsIProfiler.idl
@@ +44,5 @@
>    AString getSharedLibraryInformation();
>  };
>  
> +[scriptable, uuid(0a175ba7-8fcf-4ce9-9c4b-ccc6272f4425)]
> +interface nsIProfilerStartParams : nsISupports

smaug suggested this as the path of least resistance for passing the startup params through nsIObserverService.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8524727 [details] [diff] [review]
Let the Gecko Profiler work with child processes. r=?

>+ContentChild::AnswerGetProfile(nsCString* aProfile)
>+{
>+    char* profile = profiler_get_profile();
>+    if (profile != nullptr) {
if (profile)

>+        *aProfile = nsCString(profile, strlen(profile));
>+        free(profile);
>+    } else {
>+        *aProfile = nsCString("", 0);
EmptyCString();

>+[scriptable, uuid(0a175ba7-8fcf-4ce9-9c4b-ccc6272f4425)]
>+interface nsIProfilerStartParams : nsISupports

drop scriptable?


>+NS_IMETHODIMP nsProfilerStartParams::GetEntries(uint32_t *aEntries)
* goes with the type.
I'd prefer
NS_IMETHODIMP
nsProfilerStartParams::GetEntries(uint32_t* aEntries)
Same also elsewhere.

>+      nsTArray<nsCString> featuresArray;
>+      nsTArray<nsCString> threadNameFiltersArray;
>+
>+      for (size_t i = 0; i < aFeatureCount; ++i) {
>+        featuresArray.AppendElement(aFeatures[i]);
>+      }
>+
>+      for (size_t i = 0; i < aFilterCount; ++i) {
>+        threadNameFiltersArray.AppendElement(aThreadNameFilters[i]);
>+      }
>+
>+      nsCOMPtr<nsIProfilerStartParams> params =
>+        new nsProfilerStartParams(aProfileEntries, aInterval, featuresArray,
>+                                  threadNameFiltersArray);
We could use a bit less memory if nsProfilerStartParams ctor wouldn't take const arrays, 
but non-const and would then swap, but the arrays should be short anyway.

Comment 13

[Benoit Girard (:BenWa)]

Comment on attachment 8524727 [details] [diff] [review]
Let the Gecko Profiler work with child processes. r=?

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

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +2365,5 @@
> +                                     const double& aInterval,
> +                                     const nsTArray<nsCString>& aFeatures,
> +                                     const nsTArray<nsCString>& aThreadNameFilters)
> +{
> +    nsTArray<const char*> featureArray;

This could probably use templating on the object instance and we'd only create two instances of the code and share the code. I don't feel that strongly about sharing. Smaug?

::: tools/profiler/nsIProfiler.idl
@@ +44,5 @@
>    AString getSharedLibraryInformation();
>  };
>  
> +[scriptable, uuid(0a175ba7-8fcf-4ce9-9c4b-ccc6272f4425)]
> +interface nsIProfilerStartParams : nsISupports

Really? We need the do this to have an observable event? That's sad. Maybe write a class comment to explain this. I'm sure others are going to think this is overkill.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

> This could probably use templating on the object instance and we'd only
> create two instances of the code and share the code. I don't feel that
> strongly about sharing. Smaug?
Don't feel strongly.


> > +[scriptable, uuid(0a175ba7-8fcf-4ce9-9c4b-ccc6272f4425)]
> > +interface nsIProfilerStartParams : nsISupports
> 
> Really? We need the do this to have an observable event?
(ObserverService things aren't usually called event).
That is what ObserverService unfortunately deals with.
If you want to pass complex data, you need to pass it as nsISupports object.


> That's sad. Maybe
> write a class comment to explain this.
Yeah, a comment might be useful, though this is just about using the API we have.
Another, probably nicer option if profiler has some nsISupports implementing object, would be to expose some
features/thread/etc getters on it, and then just pass the profiler object as a subject to NotifyObservers
...
and it looks like we have nsIProfiler.
Maybe nsIProfilerStartParams could be merged to nsIProfiler?


> I'm sure others are going to think
> this is overkill.
If we had non-scriptable version of observerservice, we could more easily pass any kind of data.
In theory one could do some evil casting and pass random data as the 3rd param even now, but that is error prone.

Comment 15

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(In reply to Olli Pettay [:smaug] from comment #14)
> If we had non-scriptable version of observerservice, we could more easily
> pass any kind of data.
> In theory one could do some evil casting and pass random data as the 3rd
> param even now, but that is error prone.

The other alternative is formatting/parsing a string, which is how the memory reporter's observation works (it used to pass a uint32_t serial number cast to void*, but I needed to add more fields).

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to Olli Pettay [:smaug] from comment #14)
> > This could probably use templating on the object instance and we'd only
> > create two instances of the code and share the code. I don't feel that
> > strongly about sharing. Smaug?
> Don't feel strongly.
> 
> 

Ok, I'll just live with the duping. :)

> > > +[scriptable, uuid(0a175ba7-8fcf-4ce9-9c4b-ccc6272f4425)]
> > > +interface nsIProfilerStartParams : nsISupports
> > 
> > Really? We need the do this to have an observable event?
> (ObserverService things aren't usually called event).
> That is what ObserverService unfortunately deals with.
> If you want to pass complex data, you need to pass it as nsISupports object.
> 
> 
> > That's sad. Maybe
> > write a class comment to explain this.
> Yeah, a comment might be useful, though this is just about using the API we
> have.

Yeah, I'll add an explanation.

> Another, probably nicer option if profiler has some nsISupports implementing
> object, would be to expose some
> features/thread/etc getters on it, and then just pass the profiler object as
> a subject to NotifyObservers
> ...
> and it looks like we have nsIProfiler.
> Maybe nsIProfilerStartParams could be merged to nsIProfiler?
> 
> 

I'd do that, except that I think BenWa has expressed a desire to be able to send different feature / thread filtering rules to different processes down the road. I guess nsIProfilerStartParams is more in tune with that goal, assuming the end-result ends up using nsIObserverService to notify all of the process parents somehow.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Let the Gecko Profiler work with child processes. r=BenWa,smaug.

We now allow profiling the content process for e10s, and plugin processes.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/b6e38f77069f

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1008435.html