Closed Bug 1077451 Opened 5 years ago Closed 5 years ago

Show Profiler Call Tree in new Performance Tool

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: jsantell, Assigned: jsantell)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Blocks: perf-tool-v2
Depends on: 1077447
Depends on: 1101235
Attached patch 1077451-call-tree.patch (obsolete) — Splinter Review
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.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attachment #8525041 - Flags: review?(vporof)
(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 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+
Added bugs:
links to debugger bug 1102347
inverted call tree bug 1102350
Attached patch 1077451-call-tree.patch (obsolete) — Splinter Review
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 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.
So many root nodes! Plz fix.
Fixed extra Task.async usages and extra root node printing
Attachment #8526164 - Attachment is obsolete: true
Attachment #8526296 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/053710e85e28
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 36
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.