Closed Bug 1008435 Opened 6 years ago Closed 5 years ago

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

Categories

(Core :: Gecko Profiler, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m4+ ---

People

(Reporter: BenWa, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat)

Attachments

(2 files, 5 obsolete files)

+++ 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.
Component: Graphics: Layers → Gecko Profiler
Blocks: e10s
tracking-e10s: --- → +
OS: Linux → All
Hardware: x86_64 → All
re-noming for a discussion about how important this is for m2.
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.
Blocks: e10s-addons
Flags: needinfo?(cpeterson)
Keywords: addon-compat
I updated the website to point to this bug and that MDN link.
Flags: needinfo?(cpeterson)
Summary: [e10s] Port the profiler to e10s → [e10s] Port the built-in Gecko profiler to e10s
Blocks: e10s-perf
Blocks: 1062713
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Blocks: 1067135
We now allow profiling the content process for e10s, and plugin processes.
We now allow profiling the content process for e10s, and plugin processes.
We now allow profiling the content process for e10s, and plugin processes.
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.
Attachment #8524727 - Flags: review?(bugs)
Attachment #8524727 - Flags: review?(bgirard)
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.
Attachment #8524727 - Flags: review?(bugs) → review+
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.
Attachment #8524727 - Flags: review?(bgirard) → review+
> 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.
(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).
(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.
We now allow profiling the content process for e10s, and plugin processes.
Attachment #8525478 - Attachment description: Let the Gecko Profiler work with child processes. smaug → Let the Gecko Profiler work with child processes. r=BenWa,smaug.
Attachment #8525478 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/b6e38f77069f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1112442
You need to log in before you can comment on or make changes to this bug.