Closed
Bug 1077451
Opened 10 years ago
Closed 10 years ago
Show Profiler Call Tree in new Performance Tool
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: jsantell, Assigned: jsantell)
References
Details
Attachments
(2 files, 2 obsolete files)
291.89 KB,
image/png
|
Details | |
20.56 KB,
patch
|
jsantell
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
Blocks: perf-tool-v2
Depends on: 1077447
Assignee | ||
Comment 1•10 years ago
|
||
This is a crude first draft of displaying the call tree. Some notes: * The details view is a controller managing all the different subviews it contains (markers, flamegraph, call tree). Right now it's just kind of a placeholder. * Something not in the mockups is the concept of displaying a list of recordings. Not sure where we wound up with that discussion, IIRC it was only saving and importing, not having a list of previous recordings readily accessible, but I'm not sure. Reason I bring that up is, all the profiler tests and logic are based around this, and wondering if we should copy that. Maybe something to discuss on Thursday. * Little components and hooks aren't yet in there (clicking on a line and going to the debugger, for example), and all these should have follow up bugs. There's already one for rerendering based on a subselection (so details-to-overview hooks), but if you think of any, let me know and I'll make bugs for them.
Comment 2•10 years ago
|
||
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #1) > Created attachment 8525041 [details] [diff] [review] > 1077451-call-tree.patch > > * Something not in the mockups is the concept of displaying a list of > recordings. Not sure where we wound up with that discussion, IIRC it was > only saving and importing, not having a list of previous recordings readily > accessible, but I'm not sure. Reason I bring that up is, all the profiler > tests and logic are based around this, and wondering if we should copy that. > Maybe something to discuss on Thursday. IIRC, the consensus was that import/export is a must-have, and that means that we'll need to have a list of recordings somewhere from which we can choose a single one to display. Since an always-visible sidebar might occupy too much horizontal space, the decision was to either use a collapseable sidebar, or a dropdown. I'll handle all of that in bug 1077454. > * Little components and hooks aren't yet in there (clicking on a line and > going to the debugger, for example), and all these should have follow up > bugs. There's already one for rerendering based on a subselection (so > details-to-overview hooks), but if you think of any, let me know and I'll > make bugs for them. File those followups (jump to debugger, etc.)
Comment 3•10 years ago
|
||
Comment on attachment 8525041 [details] [diff] [review] 1077451-call-tree.patch Review of attachment 8525041 [details] [diff] [review]: ----------------------------------------------------------------- Needs moar tests. r+ with tests and nits below addressed. ::: browser/devtools/performance/controller.js @@ +42,5 @@ > let startupPerformance = Task.async(function*() { > yield promise.all([ > PrefObserver.register(), > PerformanceController.initialize(), > + PerformanceView.initialize(), Where is PerformanceView defined again? Is it intended to control all these subviews? If so, please move the DetailsView and CallTreeView initialization in there. @@ +57,5 @@ > PrefObserver.unregister(), > PerformanceController.destroy(), > + PerformanceView.destroy(), > + DetailsView.destroy(), > + CallTreeView.destroy() Ditto for destruction. \m/ ::: browser/devtools/performance/views/call-tree.js @@ +35,5 @@ > + */ > + _prepareCallTree: function (profilerData, beginAt, endAt, options={}) { > + let threadSamples = profilerData.profile.threads[0].samples; > + let contentOnly = !Prefs.showPlatformData; > + // TODO handle inverted tree Is there a bug filed for this? Add the bug number here. ::: browser/devtools/performance/views/details.js @@ +6,5 @@ > +/** > + * Details view containing profiler call tree. Manages > + * subviews and toggles visibility between them. > + */ > +let DetailsView = { Does this have to land in this patch? I don't see a reason for it yet.
Attachment #8525041 -
Flags: review?(vporof) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Added bugs: links to debugger bug 1102347 inverted call tree bug 1102350
Assignee | ||
Comment 5•10 years ago
|
||
Added tests, moved subview instantiation into parent views (so initializes cascades), added bug numbers in the TODO https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ad4c30491d09
Attachment #8525041 -
Attachment is obsolete: true
Attachment #8526164 -
Flags: review+
Comment 6•10 years ago
|
||
Comment on attachment 8526164 [details] [diff] [review] 1077451-call-tree.patch Review of attachment 8526164 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/performance/views/details.js @@ +11,5 @@ > + /** > + * Sets up the view with event binding, initializes > + * subviews. > + */ > + initialize: Task.async(function () { This should be a generator. Otherwise, no need to use Task. @@ +25,5 @@ > + > + /** > + * Unbinds events, destroys subviews. > + */ > + destroy: Task.async(function () { Ditto.
Comment 7•10 years ago
|
||
So many root nodes! Plz fix.
Assignee | ||
Comment 8•10 years ago
|
||
Fixed extra Task.async usages and extra root node printing
Attachment #8526164 -
Attachment is obsolete: true
Attachment #8526296 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/053710e85e28
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/053710e85e28
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•