Implement inverted call tree in new performance tool

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

36 Branch
Firefox 38
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Blocks: 1102347
(Assignee)

Updated

3 years ago
Blocks: 1075567
No longer blocks: 1102347
(Assignee)

Updated

3 years ago
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Depends on: 1121700
(Assignee)

Updated

3 years ago
Blocks: 1087477, 1087473
Created attachment 8550455 [details] [diff] [review]
1102350-invert-call-tree.patch

Also fixes bug 1087473 (invert call tree by default) and bug 1087477 (back invert by a pref)

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7afcf26c944e
Attachment #8550455 - Flags: review?(vporof)
Comment on attachment 8550455 [details] [diff] [review]
1102350-invert-call-tree.patch

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

Would love to see a ToolbarView for the options view.

::: browser/app/profile/firefox.js
@@ +1434,5 @@
>  pref("devtools.profiler.ui.show-platform-data", false);
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
> +// The default Performance UI settings
> +pref("devtools.performance.invert-call-tree", true);

Group this with devtools.performance.ui.show-timeline-memory, and rename it to devtools.performance.ui.invert-call-tree

::: browser/devtools/performance/performance-controller.js
@@ +173,5 @@
>     * Listen for events emitted by the current tab target and
>     * main UI events.
>     */
> +  initialize: Task.async(function* () {
> +    this.optionsView = new OptionsView({

Kinda weird to have an options view inside a controller. How about creating a ToolbarView, alongside the OverviewView etc.?

@@ +175,5 @@
>     */
> +  initialize: Task.async(function* () {
> +    this.optionsView = new OptionsView({
> +      branchName: BRANCH_NAME,
> +      window: window,

Instead of passing in the window, you can get it via menupopup.ownerDocument.defaultView. Would make the OptionsView smarter.

::: browser/devtools/performance/performance.xul
@@ +65,5 @@
>                           class="devtools-toolbarbutton"
>                           data-view="flamegraph" />
>          </hbox>
>          <spacer flex="1"></spacer>
> +        <hbox id="performance-toolbar-control-optionss" class="devtools-toolbarbutton-group">

optionssssss
Attachment #8550455 - Flags: review?(vporof) → review-
Comment on attachment 8550455 [details] [diff] [review]
1102350-invert-call-tree.patch

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

::: browser/devtools/performance/performance-controller.js
@@ +175,5 @@
>     */
> +  initialize: Task.async(function* () {
> +    this.optionsView = new OptionsView({
> +      branchName: BRANCH_NAME,
> +      window: window,

Debated the same thing with a "PerformanceOptionsView", but it was pretty tiny, which I guess is fine. Done.

A+ on the ownerDocument.defaultView
Created attachment 8550568 [details] [diff] [review]
1102350-invert-call-tree.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b284ec7ae22
Attachment #8550455 - Attachment is obsolete: true
Attachment #8550568 - Flags: review?(vporof)
Comment on attachment 8550568 [details] [diff] [review]
1102350-invert-call-tree.patch

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

::: browser/app/profile/firefox.js
@@ +1434,5 @@
>  pref("devtools.profiler.ui.show-platform-data", false);
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
> +// The default Performance UI settings
> +pref("devtools.performance.invert-call-tree", true);

Previous comment: devtools.performance.ui.invert-call-tree :)

::: browser/devtools/performance/performance-controller.js
@@ +179,5 @@
>      this.importRecording = this.importRecording.bind(this);
>      this.exportRecording = this.exportRecording.bind(this);
>      this._onTimelineData = this._onTimelineData.bind(this);
>      this._onRecordingSelectFromView = this._onRecordingSelectFromView.bind(this);
> +    this._onPrefChanged = this._onPrefChanged.bind(this);

How does this clash with PrefObserver in this file btw? Is that deprecated now? Should remove it if so.
Attachment #8550568 - Flags: review?(vporof) → review+
Comment on attachment 8550568 [details] [diff] [review]
1102350-invert-call-tree.patch

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

::: browser/app/profile/firefox.js
@@ +1434,5 @@
>  pref("devtools.profiler.ui.show-platform-data", false);
>  pref("devtools.profiler.ui.show-idle-blocks", true);
>  
> +// The default Performance UI settings
> +pref("devtools.performance.invert-call-tree", true);

Ah yes adding that now

::: browser/devtools/performance/performance-controller.js
@@ +179,5 @@
>      this.importRecording = this.importRecording.bind(this);
>      this.exportRecording = this.exportRecording.bind(this);
>      this._onTimelineData = this._onTimelineData.bind(this);
>      this._onRecordingSelectFromView = this._onRecordingSelectFromView.bind(this);
> +    this._onPrefChanged = this._onPrefChanged.bind(this);

It doesn't clash as this pref isn't tracked by that. Leaving it in for now, and made bug 1122639 to migrate these properties over to the new pref/expose on the options button
Created attachment 8550644 [details] [diff] [review]
1102350-invert-call-tree.patch

All good to go
Attachment #8550568 - Attachment is obsolete: true
Attachment #8550644 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Hi,

this patch failed to apply:

applying 1102350-invert-call-tree.patch
patching file browser/devtools/performance/views/details-call-tree.js
Hunk #1 FAILED at 8
Hunk #2 FAILED at 44
2 out of 4 hunks FAILED -- saving rejects to file browser/devtools/performance/views/details-call-tree.js.rej
patching file browser/devtools/performance/views/details-flamegraph.js
Hunk #1 FAILED at 49
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/performance/views/details-flamegraph.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 1102350-invert-call-tree.patch

could you take a look, thanks!
Flags: needinfo?(jsantell)
Keywords: checkin-needed
Created attachment 8551416 [details] [diff] [review]
1102350-invert-call-tree.patch

Here's a rebased patch
Attachment #8550644 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Attachment #8551416 - Flags: review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a74557453c80
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.