1102350 - Implement inverted call tree in new performance tool

Status: RESOLVED FIXED

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

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 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

Comment 2

[Victor Porof [:vporof][:vp]]

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

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1102350-invert-call-tree.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b284ec7ae22

Comment 5

[Victor Porof [:vporof][:vp]]

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.

Comment 6

[Jordan Santell [:jsantell] [@jsantell]]

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

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1102350-invert-call-tree.patch

All good to go

Comment 8

[Carsten Book [:Tomcat]]

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!

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1102350-invert-call-tree.patch

Here's a rebased patch

Comment 10

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/a74557453c80

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a74557453c80