1108843 - Show meta category nodes instead of filtering them out in the profiler tree

Status: RESOLVED FIXED

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

Description

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

Was just profiling shumway with :mbx. With "show platform data" some function was taking ~2.5% of time. When we disabled "show platform data", it became ~11%.

The percentages can't be computed after we filter out platform data, or the numbers get skewed way off.

Comment 1

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

As discussed, will dupe all the other microbugs to this one.

Comment 8

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

Thanks for assigning yourself to this Jordan.

Comment 9

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

Attachment: Screen Shot 2015-03-20 at 1.49.51 PM.png

BOOM

Comment 10

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

Attachment: WIP

Comment 11

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

(Handing over to Jordan now)

Comment 12

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

Comment on attachment 8580899 [details] [diff] [review]
WIP

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

::: browser/devtools/shared/profiler/tree-model.js
@@ +85,5 @@
> +//     After:  ContentA -> ContentB
> +//
> +//     Before: PlatformA -> ContentA -> PlatformB -> PlatformC
> +//     After:  ContentA -> Category(PlatformC)
> +function simplyAndTranslatePlatformData(frames) {

Nit: please move this method at the end of this file, where all the utils (e.g. isContent and nsIURL) are defined.

@@ +88,5 @@
> +//     After:  ContentA -> Category(PlatformC)
> +function simplyAndTranslatePlatformData(frames) {
> +  var lastIdx = frames.length - 1;
> +  return frames
> +    .map((frame, idx) => {

Nit: since this is incredibly hot code, let's avoid a double array traversal and use a more boring imperative style for this method.

@@ +95,5 @@
> +      }
> +
> +      return idx === lastIdx
> +        ? synthesizeCategoryFrame(frame)
> +        : null;

I don't like us creating a fake "category" frame here, since it'll be hard to style it in the view. A category node is always created on the view anyway [0], and that *is* styled properly.

Let's keep the existing C++ frame here instead of creating a new object, since it has the category information anyway, and hide the frame label in the view.

[0] https://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/profiler/tree-view.js?from=tree-view.js#349

@@ +104,5 @@
> +function synthesizeCategoryFrame(frame) {
> +  let categoryData = CATEGORY_MAPPINGS[frame.category];
> +  if (!categoryData) {
> +    // Unknown category, just put it in the "other" category.
> +    categoryData = CATEGORIES[0];

This should never happen for C++ frames afaik, and using CATEGORIES[0] is incorrect because the bitmasks may change in the future. CATEGORY_MASK('other') or CATEGORY_OTHER is better, but that's beside the point because creating a fake frame isn't really necessary in the first place.

Comment 13

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

(In reply to Victor Porof [:vporof][:vp] from comment #12)
> @@ +104,5 @@
> > +function synthesizeCategoryFrame(frame) {
> > +  let categoryData = CATEGORY_MAPPINGS[frame.category];
> > +  if (!categoryData) {
> > +    // Unknown category, just put it in the "other" category.
> > +    categoryData = CATEGORIES[0];
> 
> This should never happen for C++ frames afaik, and using CATEGORIES[0] is
> incorrect because the bitmasks may change in the future.
> CATEGORY_MASK('other') or CATEGORY_OTHER is better, but that's beside the
> point because creating a fake frame isn't really necessary in the first
> place.

What about chrome JS?

Comment 14

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

Which !isContent() but has no category

Comment 15

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

this will be behind a different flag, as the current profiler is using this tree view, and don't want to change this shortly before uplift, as no one's going to be testing the old profiler with this change, unless there's a good reason to

Comment 16

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

I'd say either ship it after the merge, or ship it for both. Another flag is too much unnecessary complexity...

Comment 17

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

Attachment: 1108843-platformdata.patch

Cleaned up version -- not sure if this seems right. Check both inverted/non-inverted trees, etc

Comment 18

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

Waiting until after uplift sounds fine

Comment 19

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

Comment on attachment 8580985 [details] [diff] [review]
1108843-platformdata.patch

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

::: browser/devtools/shared/profiler/tree-model.js
@@ +66,5 @@
>      this.insert(sample, options);
>    }
>  }
>  
> +

nit

Comment 20

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

Attachment: Screen Shot 2015-03-20 at 5.40.43 PM.png

Running a webgl page, 90% of the work in is Graphics (obviously), but no clue of what's calling it or what a web developer could do to improve

Comment 21

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

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #20)
> Created attachment 8580998 [details]
> Screen Shot 2015-03-20 at 5.40.43 PM.png
> 
> Running a webgl page, 90% of the work in is Graphics (obviously), but no
> clue of what's calling it or what a web developer could do to improve

But before you didn't even know you were spending 90% of time there (and in fact all percentages that you _did_ see would be calculated incorrectly), so that is a pretty big improvement in itself.

Comment 22

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #13)
> 
> What about chrome JS?

You're right, I forgot about that. Maybe we need a separate category for it, that actually says it's chrome js, not "other" (which can mean a lot of things).

Comment 23

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #16)
> I'd say either ship it after the merge, or ship it for both. Another flag is
> too much unnecessary complexity...

Agree with Nick on this one.

Comment 24

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

(In reply to Nick Fitzgerald [:fitzgen] from comment #21)
> 
> But before you didn't even know you were spending 90% of time there (and in
> fact all percentages that you _did_ see would be calculated incorrectly), so
> that is a pretty big improvement in itself.

Tooltips are your friend. We could let devs know that these things aren't optimizeable by them.

Comment 25

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

Comment on attachment 8580985 [details] [diff] [review]
1108843-platformdata.patch

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

Love this

::: browser/devtools/shared/profiler/tree-view.js
@@ +306,5 @@
>      cell.appendChild(arrowNode);
>  
> +    if (frameInfo.generalize) {
> +      cell.classList.add("call-tree-generalized");
> +    }

It might be a good idea to either avoid building the .call-tree-category here altogether, instead of hiding it behind a css rule like in the current profiler.

Comment 26

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

So, holding back until Fx40, gives us time to experiment a bit with this, which I think we could use.

Implementation details:
* not build additional elements, rather than hiding, for generalized frames
* don't put behind a flag

User visible changes:
* Tooltips explaining generalized nodes
* Additional categories, like Chrome JS? We also suspect some of our idle time is being grouped under "Gecko" or something, looking like things are less performant than the really are

Comment 27

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

Attachment: 1108843-platformdata.patch

Scope this out. Won't land until Fx40, but we can make changes after landing this (category edge-cases, etc)

Comment 28

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

Comment on attachment 8583365 [details] [diff] [review]
1108843-platformdata.patch

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

shipit.mkv

Comment 29

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

Comment on attachment 8583365 [details] [diff] [review]
1108843-platformdata.patch

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

r+ using CATEGORY_MASK

::: browser/devtools/performance/test/browser_profiler_tree-model-07.js
@@ +59,5 @@
> +  frames: [
> +    { location: "(root)" },
> +    { location: "http://content/A" },
> +    { location: "http://content/B" },
> +    { location: "contentY", category: 32 },

Don't use numbers for categories. Use CATEGORY_MASK("name").

::: browser/devtools/performance/test/browser_profiler_tree-view-08.js
@@ +74,5 @@
> +  frames: [
> +    { location: "(root)" },
> +    { location: "http://content/A" },
> +    { location: "http://content/B" },
> +    { location: "contentY", category: 32 },

Ditto.

::: browser/devtools/shared/profiler/tree-model.js
@@ +160,5 @@
>    this.sampleTimes = [];
>    this.samples = 0;
>    this.duration = 0;
>    this.calls = {};
> +  this.generalize = generalize;

Nit: "generalize" sounds a little weird to me. Something like isMetaCategory might be more descriptive, but meh.

::: browser/devtools/shared/profiler/tree-view.js
@@ +187,5 @@
>      targetNode.className = "call-tree-item";
>      targetNode.setAttribute("origin", frameInfo.isContent ? "content" : "chrome");
>      targetNode.setAttribute("category", frameInfo.categoryData.abbrev || "");
> +    targetNode.setAttribute("tooltiptext", frameInfo.generalize ? frameInfo.categoryData.label :
> +                                           this.frame.location || "");

Nit:

frameInfo.generalize
  ? rameInfo.categoryData.label
  : this.frame.location || ""

Comment 30

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

Attachment: 1108843-platformdata.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4add57f7322b

Comment 31

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

Looks good, intermittent failure in animation tool taken care of

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7555f34e3ba9