1049825 - Option to Invert Call Tree in Profiler

Status: RESOLVED FIXED

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

Description

[Rob Campbell [:rc] (:robcee)]

The details view should have an option to "Invert" the tree. Show data from the bottom up.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: invert-trees.patch

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0766cceb3837

Comment 2

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

Comment on attachment 8502860 [details] [diff] [review]
invert-trees.patch

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

::: browser/devtools/profiler/ui-profile.js
@@ +555,5 @@
>      this._spawnTabFromSelection();
>    },
>  
> +  _onInvertTree: function() {
> +    this._zoomTreeFromSelection();

Drive-by nit: might want to rename this method from "_zoomTreeFromSelection" to "_rebuildTreeFromSelection" or something else more descriptive.

::: browser/devtools/profiler/utils/tree-model.js
@@ +170,5 @@
>     *        The delta time (in milliseconds) when the frame was sampled.
>     * @param number duration
>     *        The amount of time spent executing all functions on the stack.
> +   // * @param boolean inverted [optional]
> +   // *        Specifies if the tree should be inverted.

Weird comments, // and *

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8502860 [details] [diff] [review]
invert-trees.patch

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

I've played with the profiler with the patch applied. It seems to work fine, and is quite useful.

Swapping the self-time with the percentage seems unrelated to this patch though, can you explain why you did this here?

The only edge-case I could find was when you record a profile that turns out empty (no recorded calls), then checking the "invert call tree" box removes the "0 0% 0 0% (root)" line altogether.

Cancelling review for the css clean-ups due to removing #recordings-pane and because I think Victor should review this patch too.

::: browser/devtools/profiler/profiler.xul
@@ +18,5 @@
>    <script type="application/javascript" src="ui-recordings.js"/>
>    <script type="application/javascript" src="ui-profile.js"/>
>  
>    <hbox class="theme-body" flex="1">
> +    <vbox>

You've removed the ID "recordings-pane" but I can still see it being used in the css (profiler.inc.css).

::: browser/devtools/profiler/test/browser_profiler_tree-view-02.js
@@ +81,5 @@
>      "The .B node in the tree has the correct class name.");
>    is(container.childNodes[3].className, "call-tree-item",
>      "The .E node in the tree has the correct class name.");
>  
> +  debugger;

Left-over debugger statement.

::: browser/devtools/profiler/utils/tree-model.js
@@ +49,5 @@
>   * @param number beginAt [optional]
>   *        @see ThreadNode.prototype.insert
>   * @param number endAt [optional]
>   *        @see ThreadNode.prototype.insert
> + * @param boolean invert [optiona]

s/optiona/optional

@@ +57,5 @@
>    this.samples = 0;
>    this.duration = 0;
>    this.calls = {};
>    this._previousSampleTime = 0;
> +  this.inverted = invert;

this.inverted isn't used in this module and doesn't seem to be used in the profiler UI at all.

@@ +78,5 @@
>     * @param number beginAt [optional]
>     *        The earliest sample to start at (in milliseconds).
>     * @param number endAt [optional]
>     *        The latest sample to end at (in milliseconds).
> +   * @param boolean invert [optiona]

s/optiona/optional

@@ +170,5 @@
>     *        The delta time (in milliseconds) when the frame was sampled.
>     * @param number duration
>     *        The amount of time spent executing all functions on the stack.
> +   // * @param boolean inverted [optional]
> +   // *        Specifies if the tree should be inverted.

This looks like left-over comments that should be removed. The last param of the insert function isn't the inverted boolean but _store

::: browser/devtools/profiler/utils/tree-view.js
@@ +44,5 @@
>   *        Details about this function, like { samples, duration, calls } etc.
>   * @param number level
>   *        The indentation level in the call tree. The root node is at level 0.
>   */
> +function CallView({ autoExpandDepth, caller, frame, level, hidden, inverted }) {

You've added 3 parameters here, can you please document them in the function jsdoc comment block?

@@ +53,5 @@
> +
> +  AbstractTreeItem.call(this, {
> +    parent: caller,
> +    level: level
> +  });

Or AbstractTreeItem.call(this, {parent: caller, level});

@@ +82,5 @@
>      let framePercentage = this._getPercentage(this.frame.samples);
> +
> +    let selfPercentage;
> +    let selfDuration;
> +    if (!this._getChildCalls().length) {

I would feel more comfortable if you asked Victor or someone else who knows the profiler UI to review this bit.

::: browser/devtools/shared/widgets/AbstractTreeItem.jsm
@@ +8,5 @@
>  const Cu = Components.utils;
>  
>  Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
>  Cu.import("resource://gre/modules/devtools/event-emitter.js");
> +Cu.import("resource://gre/modules/devtools/Console.jsm");

The console API doesn't seem to be used in this module.

::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
@@ +38,5 @@
>  <!ENTITY profilerUI.clearButton "Clear">
>  
> +<!-- LOCALIZATION NOTE (profilerUI.invertTree): This is the label shown next to
> +  -  a checkbox that inverts and un-inverts the profiler's call tree. -->
> +<!ENTITY profilerUI.invertTree "Invert Call Tree">

Should we also have a tooltip on the label with a longer description of what inverting the tree does?
Maybe this is straightforward enough?

::: browser/themes/shared/devtools/profiler.inc.css
@@ +49,5 @@
>  }
>  
>  /* Recordings pane */
>  
> +#recordings-pane > tabs,

ID recordings-pane doesn't exist anymore.

@@ +58,3 @@
>  }
>  
>  .theme-dark #recordings-pane > tabs,

ditto

@@ +62,4 @@
>    -moz-border-end-color: #000; /* Splitters */
>  }
>  
>  .theme-light #recordings-pane > tabs,

ditto

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Thanks for the review, Patrick!

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #3)
> Comment on attachment 8502860 [details] [diff] [review]
> invert-trees.patch
> 
> Review of attachment 8502860 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've played with the profiler with the patch applied. It seems to work fine,
> and is quite useful.
> 
> Swapping the self-time with the percentage seems unrelated to this patch
> though, can you explain why you did this here?

Because Victor and I agreed it was the best thing to do at the London meet up, and splitting it out felt like too much work at the time when I was just hack-hack-hacking away willy nilly. I can split it out if its hindering your review.

> The only edge-case I could find was when you record a profile that turns out
> empty (no recorded calls), then checking the "invert call tree" box removes
> the "0 0% 0 0% (root)" line altogether.

What behavior do you think would be best in this situation? Special case the hiding of the fictional root sample and show it?

> Cancelling review for the css clean-ups due to removing #recordings-pane and
> because I think Victor should review this patch too.

Ah good catch on the CSS stuff -- I should have caught that myself, sorry.

I flagged you because I thought Victor was on vacation for the next couple weeks.

> @@ +82,5 @@
> >      let framePercentage = this._getPercentage(this.frame.samples);
> > +
> > +    let selfPercentage;
> > +    let selfDuration;
> > +    if (!this._getChildCalls().length) {
> 
> I would feel more comfortable if you asked Victor or someone else who knows
> the profiler UI to review this bit.

I don't think anyone else knows the profiler UI... :-/

Should I just wait for Victor to come back from vacation?

> ::: browser/devtools/shared/widgets/AbstractTreeItem.jsm
> @@ +8,5 @@
> >  const Cu = Components.utils;
> >  
> >  Cu.import("resource:///modules/devtools/ViewHelpers.jsm");
> >  Cu.import("resource://gre/modules/devtools/event-emitter.js");
> > +Cu.import("resource://gre/modules/devtools/Console.jsm");
> 
> The console API doesn't seem to be used in this module.

I used it for debugging, but we usually leave these imports because it sucks to start adding console calls and then realize you're in a JSM and don't have access to console unless you import it and then you have to remember/hunt down where the heck the console module actually lives, etc etc. It's just a horrible experience.

I think it is best to just leave it, although I will definitely make it lazily loaded.

> ::: browser/locales/en-US/chrome/browser/devtools/profiler.dtd
> @@ +38,5 @@
> >  <!ENTITY profilerUI.clearButton "Clear">
> >  
> > +<!-- LOCALIZATION NOTE (profilerUI.invertTree): This is the label shown next to
> > +  -  a checkbox that inverts and un-inverts the profiler's call tree. -->
> > +<!ENTITY profilerUI.invertTree "Invert Call Tree">
> 
> Should we also have a tooltip on the label with a longer description of what
> inverting the tree does?
> Maybe this is straightforward enough?

Good idea, I will add a tooltip.

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: invert-trees.patch

Updated based on last review's comments.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8fcc61b629f0

If you feel we should just wait for Victor to come back, just slide the review over to him.

Comment 6

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

Comment on attachment 8507460 [details] [diff] [review]
invert-trees.patch

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

I'M BACK

::: browser/devtools/profiler/utils/tree-model.js
@@ +104,5 @@
> +    if (inverted) {
> +      sampleFrames.reverse();
> +    }
> +
> +    let rootIndex = (inverted || contentOnly) ? 0 : 1;

Does this end up displaying the (root) node as a leaf when showing platform data? (i.e., not filtering based on `isContent`)?

::: browser/themes/shared/devtools/profiler.inc.css
@@ +51,5 @@
>  /* Recordings pane */
>  
> +#recordings-pane > tabs,
> +#recordings-toolbar,
> +#recordings-pane .devtools-toolbar,

Simply from looking at this, the difference between `#recordings-toolbar` and `#recordings-pane .devtools-toolbar` is not clear. How about some better descriptive selectors, or removing any redundant ones from here, if any.

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Comment on attachment 8507460 [details] [diff] [review]
> invert-trees.patch
> 
> Review of attachment 8507460 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'M BACK
> 
> ::: browser/devtools/profiler/utils/tree-model.js
> @@ +104,5 @@
> > +    if (inverted) {
> > +      sampleFrames.reverse();
> > +    }
> > +
> > +    let rootIndex = (inverted || contentOnly) ? 0 : 1;
> 
> Does this end up displaying the (root) node as a leaf when showing platform
> data? (i.e., not filtering based on `isContent`)?

Good catch.

> ::: browser/themes/shared/devtools/profiler.inc.css
> @@ +51,5 @@
> >  /* Recordings pane */
> >  
> > +#recordings-pane > tabs,
> > +#recordings-toolbar,
> > +#recordings-pane .devtools-toolbar,
> 
> Simply from looking at this, the difference between `#recordings-toolbar`
> and `#recordings-pane .devtools-toolbar` is not clear. How about some better
> descriptive selectors, or removing any redundant ones from here, if any.

Another good catch. Left over from when I accidentally deleted the id attribute >.<

New patch + try push incoming.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: invert-trees.patch

Carrying over r+.

Try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b0ddd4f34201

Here's to hoping those leaks from the last try push were unrelated.

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6e500121fb42