1153473 - Idle blocks should be uniquely colored and consistent in flame graph

Status: RESOLVED WONTFIX

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

Description

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

Attachment: idlenodecolors

The flame graph renders idle blocks as "(idle)" when displaying only content (when platform data is shown, determining what is "idle" is handled in bug 1152839) - other top level frames can get the same color, making it hard to distinguish from afar what is idle and what is activity.

At the very least, idle nodes should be unique to the entire graph. Better yet, a good color, as colors are generated randomly on first use per toolbox, so users can get used to seeing "idle" nodes as an identifying color.

Comment 1

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

I don't think this is a blocker, and also we'd need more platform support on this.

Comment 2

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

I could still special case them and add an extendable mechanism in place.

Comment 3

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

Attachment: idle-colors.patch

Comment 4

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

Comment on attachment 8604683 [details] [diff] [review]
idle-colors.patch

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

Needs some tests and good to go I think if the meta category stuff was handled in Shu's patch or something. But otherwise, may need bug 1147693, and at the very least, tests for the getColor/getLabel utils, if not doing a full flame graph test

::: browser/devtools/performance/views/details-js-call-tree.js
@@ +115,5 @@
>      this.container.innerHTML = "";
>      root.attachTo(this.container);
>  
>      // When platform data isn't shown, hide the cateogry labels, since they're
> +    // only available for C++ frames. // Pass *false* to make them invisible.

cateogry
comment within a comment

@@ +116,5 @@
>      root.attachTo(this.container);
>  
>      // When platform data isn't shown, hide the cateogry labels, since they're
> +    // only available for C++ frames. // Pass *false* to make them invisible.
> +    root.toggleCategories(!options.contentOnly);

Oh. Is this bug 1152631?

::: browser/devtools/shared/widgets/FlameGraph.js
@@ +1179,5 @@
> +    if (FrameUtils.isRoot(frame)) {
> +      return SPECIAL_COLORS.ROOT;
> +    }
> +    if (frame.isMetaCategory) {
> +      return CATEGORY_MAPPINGS[frame.category].color;

Nice. Make those metacategories all look the same too! Should have a bug for using themeable "colorNames" like markers or something

@@ +1199,5 @@
>      // anyway.
>      if (frame.idle) {
>        return frame.location;
>      }
> +    if (frame.isMetaCategory) {

Doesn't this require, and all the meta category stuff, bug 1147693?

Comment 5

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 6

[Julien Wajsberg [:julienw]]

Now that we replaced the old performance panel by the Firefox Profiler (see Bug 1668219) we're closing bugs related to this. If you see similar issues on the new profiler, please file another bug, and we'll be happy to have a look.

Filter on MASSCLOSEOLDPERFTOOLBUGS.