Show categories breakdown in flamegraph tooltip if implementation breakdown not present

[parttimenerd]

Adds a breakdown based on categories to the tooltips if the implementation breakdown cannot be shown (no implementation data present). Showing both at the same time would clutter the UI too much. Useful especially for imported profiles where the implementation data is missing.

Related to #4189 which added an option to omit the implementation data.

Sample profile

Current production
Deploy preview
Deploy preview 2

[codecov]

Codecov Report

Base: 88.49% // Head: 88.31% // Decreases project coverage by -0.18% ⚠️

Coverage data is based on head (4213f87) compared to base (4965f7a).
Patch coverage: 17.18% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4193      +/-   ##
==========================================
- Coverage   88.49%   88.31%   -0.19%     
==========================================
  Files         282      282              
  Lines       25107    25170      +63     
  Branches     6756     6778      +22     
==========================================
+ Hits        22219    22228       +9     
- Misses       2683     2729      +46     
- Partials      205      213       +8     

Impacted Files	Coverage Δ
src/profile-logic/profile-data.js	90.41% <ø> (ø)
src/utils/index.js	75.60% <0.00%> (-12.97%)	⬇️
src/components/tooltip/CallNode.js	58.74% <18.96%> (-28.47%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mstange]

Here's a deploy preview link with a Firefox profile to which I manually added "doesNotUseFrameImplementation":true.

[mstange]

This is pretty cool!

With the Firefox profile I noticed some confusing output where categories were seemingly duplicated, e.g. there are two rows for "Graphics". I think the first row is the "Graphics" category, and the second row is the "Graphics: Other" subcategory. It would be great if the Other subcategory was spelled out here.

For categories for which only one subcategory is displayed, I wonder if it makes sense to leave out the category row and only display the single subcategory row.

And the following could be future improvements (not handled in this PR):

• Use the category colors instead of blue. (Requires both an "intense" and a "soft" version of each color)
• Improve the display for subcategories, maybe with indentation or other visual grouping so that the category name doesn't have to be repeated for each subcategory.

[parttimenerd]

With the Firefox profile I noticed some confusing output where categories were seemingly duplicated, e.g. there are two rows for "Graphics". I think the first row is the "Graphics" category, and the second row is the "Graphics: Other" subcategory. It would be great if the Other subcategory was spelled out here.

This is due to the built in mapping of the "other" category to "", but I can change it.

For categories for which only one subcategory is displayed, I wonder if it makes sense to leave out the category row and only display the single subcategory row.
Useful.

[julienw]

I haven't looked to the code or even tried it (as I'm in holidays 😁). I just wanted to give the feedback that 1/ this is pretty cool, thanks for working on this! And 2/ what do you think of showing the categories by default, and the implementation only if there's no category data ?

[parttimenerd]

And 2/ what do you think of showing the categories by default, and the implementation only if there's no category data ?

I have no real opinion on it, as in my use case there is no implementation data and introduced a flag in #4189 to essentially hide all implementation data.

I think #3713 is the place to discuss these kinds of issues.

[parttimenerd]

I updated it, it now looks as follows:

[parttimenerd]

Now with correct colors:

[canova]

Thanks for working on that! Looks pretty cool!
I didn't look at the whole code yet but while I was playing with the deploy preview, I've realized that there is this extraneous bar for the leaf-most frames:

Could you take a look at this issue?
Also we need some tests for them as there is a big chunk of code that's not covered by tests right now. We can discuss it later today if you like.

[parttimenerd]

I added tests and fixed the formatting.

[fqueze]

The vertical alignment between the blue or grey bars and the text looks like it could be improved. I think it would look better if the bottom of the colored bar was aligned with the baseline of the text. I saw this both in the screenshots here and when trying your deploy preview.

[parttimenerd]

You're right, thanks for spotting it.

[julienw]

Looking at the deploy preview, I can see the following issues that will need to be resolved:

1. We don't see the difference between self and running time anymore. I believe you could use the category color with some transparency for the running time.
2. We don't see the grey background bar, which was very useful to relate the numbers on the right with the labels, especially with the values are small (which makes the bar small too). Of course then there's a problem with the grey category; for that one I thought it could be white and light grey with a border.
3. The overview bar is now missing.
4. There's the alignment problem that Florian already reported.
5. I think we should keep the word "samples" in the table rather than the title. I understand this shows repeatition, but we believed the table is more balanced that way, otherwise it looks a bit broken.
6. We need the table to breathe some more so that we see the structure of categories/subcategories better. We could think of adding a top margin (4 or 8px) for the category title, as well as a right padding (4 or 8 px too) to move it slightly to the left. Not 100% sure but I'd like to see the result of this.

Thanks!

[parttimenerd]

Thanks for the suggestions, I'm currently working on them.

[parttimenerd]

It now looks like:

With automatic brightening of colors, surrounding problematic lighter color bars with a grey line.

[julienw]

Mmm from the screenshot it looks like the bars are a bigger height for the category than for the subcategory, also it doesn't seem to end at the same position either.

[parttimenerd]

also it doesn't seem to end at the same position either

That's fixed in the most recent commit.

Mmm from the screenshot it looks like the bars are a bigger height for the category than for the subcategory

That's on purpose: making the category labels bold, increases the height of the label font. This solves the centering issue

[parttimenerd]

I improved the style of categories:

[julienw]

I haven't looked too closely at the code but still left a few comments, and wanted to push them to you.

About the styling, it's definitely much better than before. I'm not a big fan of the underlines though.

Here are some examples from a profile where I forced displaying it:

(this is with the profile with the key def4zbt1tyg8w0x7nkdtq8g7s6h3ad0ndz7jbzg if you want to try locally)

I think we can make the following improvements:

• when the only subcategory is the Other subcategory, skip it
• when there's only one subcategory, merge the category with its subcategory
• when there are several subcategories with the other subcategory, it should come last
• The bar style for the "Other" category isn't very good
• The bar style for the "self" of JavaScript category isn't very good
• The underline isn't really good when there are a few subcategories only -- but maybe this will be better once you apply the 2 first items here

Colors are hard and we may want to change how we draw self vs running time.
One quick thought, maybe it's bad but you'll tell me: instead of having one bar, with 2 delimitations inside, we could have 2 bars with half height.

This means, instead of:

JavaScript    [-------|-------]##########

(last part is the grey part)

We would have:

JavaScript   [--------------]##########
             [-------]#################

(with each line being half the height than before)

Or:

JavaScript   [--------------]##########
             [-------]#######

(where the grey part for the selftime is the length of the running time)

Or:

JavaScript   [-------]#######

(the grey part is for the running time, not the root time -- we still have the overall bar at the top to give the scale)

Easiest to try could be the last option, so it's worth trying.

The good thing with all these options: you don't need to bother with light versions of each colors, which should simplify the code.

Last possibility, but IMO more difficult with all the colors and the fact we already have some categories that look like this:

JavaScript   [-------]      ]###########

That is that the running time always has a white background, with its color only for the border.

So here are my thoughts.

Tell me what you think...

[julienw]

I think I'll give it a go myself this week, so that we can maybe reach a consensus next week.

[parttimenerd]

Sorry, I focused on the other PRs (like sorting and resizing), but I'll comeback to this PR tomorrow.

[julienw]

Sorry, I focused on the other PRs (like sorting and resizing), but I'll comeback to this PR tomorrow.

no worries, my comment came after a discussion with @canova: because we're not sure of the right path, it's probably a good idea that I make my hand dirty and try the things myself :-)

[parttimenerd]

So I should not work on this tomorrow?

[parttimenerd]

About the styling, it's definitely much better than before. I'm not a big fan of the underlines though.

I replaced it my making the category headers bold.

when the only subcategory is the Other subcategory, skip it

That is reasonable.

when there's only one subcategory, merge the category with its subcategory

I merged it as "{category}: {subcategory}"

when there are several subcategories with the other subcategory, it should come last

Ok.

One quick thought, maybe it's bad but you'll tell me: instead of having one bar, with 2 delimitations inside, we could have 2 bars with half height.

That sounds quite reasonable and color agnostic:

I integrated all the other requested changes too.

[parttimenerd]

This looks reasonable (and usable) to me now.

[julienw]

This is much better than the previous style, thanks for trying my solution!

Here are a few more comments. Here is the gist of them:

• we can simplify the CSS more
• we can make the algorithm easier to follow. It would be OK if you prefer to not do it, tell me what you think here.
• there's a bug with the other subcategory (that's easy to fix)

Tell me what you think

[parttimenerd]

I integrated all your suggestions, the code is now far better readable and less complex.

[julienw]

Thanks, this is indeed a lot simpler.

However I found an important bug, I think it's related to the key in the loop, more details below.

I'd like to see the Other category at the end of all categories, I mentioned this in my previous review too, but possibly not very clearly, sorry about that.

I think there's an issue for the selftime value of the other subcategory.

Please add just one another commit with the changes so that it's easy to review again.

[julienw]

you closed the discussion but it still doesn't seem to be used :) can you remove this block then?

[julienw]

Sorry, I missed you pushed new commits, I'll try to look at this tomorrow.

[parttimenerd]

Thanks for the fixes, they look good.

[julienw]

I squashed all your commits first, so that I could rebase more easily. I fixed one small issue with the old-style implementation timings, fixed a warning in react (they want that we pass backgroundColor not "background-color"), and finally changed a few things only regarding the code style.

Waiting for the tests and then I can merge finally! Thanks for your patience.

old review