Closed
Bug 1008435
Opened 10 years ago
Closed 9 years ago
[e10s] Port the built-in Gecko profiler to e10s
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
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)
22.24 KB,
patch
|
mconley
:
review+
|
Details | Diff | Splinter Review |
403.78 KB,
application/zip
|
Details |
+++ 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.
Reporter | ||
Updated•10 years ago
|
Component: Graphics: Layers → Gecko Profiler
Updated•10 years ago
|
Reporter | ||
Comment 2•9 years ago
|
||
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•9 years ago
|
||
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
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mconley
Assignee | ||
Comment 4•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 6•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8522502 -
Attachment is obsolete: true
Assignee | ||
Comment 7•9 years ago
|
||
We now allow profiling the content process for e10s, and plugin processes.
Assignee | ||
Updated•9 years ago
|
Attachment #8524127 -
Attachment is obsolete: true
Assignee | ||
Comment 8•9 years ago
|
||
We now allow profiling the content process for e10s, and plugin processes.
Assignee | ||
Updated•9 years ago
|
Attachment #8524672 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
We now allow profiling the content process for e10s, and plugin processes.
Assignee | ||
Updated•9 years ago
|
Attachment #8524704 -
Attachment is obsolete: true
Assignee | ||
Comment 10•9 years ago
|
||
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)
Assignee | ||
Comment 11•9 years ago
|
||
Comment 12•9 years ago
|
||
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+
Reporter | ||
Comment 13•9 years ago
|
||
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+
Comment 14•9 years ago
|
||
> 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•9 years ago
|
||
(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).
Assignee | ||
Comment 16•9 years ago
|
||
(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.
Assignee | ||
Comment 17•9 years ago
|
||
We now allow profiling the content process for e10s, and plugin processes.
Assignee | ||
Updated•9 years ago
|
Attachment #8524727 -
Attachment is obsolete: true
Assignee | ||
Comment 18•9 years ago
|
||
remote: https://hg.mozilla.org/integration/fx-team/rev/b6e38f77069f
Assignee | ||
Updated•9 years ago
|
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: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Assignee | ||
Comment 20•9 years ago
|
||
bugnotes |
http://people.mozilla.org/~mconley2/bugnotes/bug-1008435.html
You need to log in
before you can comment on or make changes to this bug.
Description
•