firefox-devtools / profiler

Firefox Profiler — Web app for Firefox performance analysis
https://profiler.firefox.com
Mozilla Public License 2.0
1.19k stars 387 forks source link

Use the categories in the flame graph tooltip instead of the old implementation information #4966

Closed julienw closed 5 months ago

julienw commented 5 months ago

The changes

As we discussed several times, with this PR we now use the categories information in the flame graph tooltip.

This also changes some logic and CSS.

  1. This changes how they're styled, using a flex container instead of absolute positioning. This also adds a subtle border (top or bottom depending on total count / self count) for accessibility reason, so that the less visible category color still has some contrast (especially yellow and white).
  2. Meter bars for categories (not subcategories!) now use the overall total time as their denominator, previously this was always taking the full width. Happy to hear feedback about that one.

Here are some more thoughts about point 2:

Some screenshots

Before: image

Without the CSS changes: image

After, with all CSS changes: image

Here is also an example with some self time: image

Deploy preview

Deploy preview / production

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 88.45%. Comparing base (50798ee) to head (3d39ffa).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #4966 +/- ## ========================================== + Coverage 88.32% 88.45% +0.13% ========================================== Files 304 304 Lines 27330 27309 -21 Branches 7380 7372 -8 ========================================== + Hits 24139 24156 +17 + Misses 2963 2931 -32 + Partials 228 222 -6 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mstange commented 5 months ago

And yet another follow-up: Maybe we should remove the self time bars altogether. Half the bars are useless for call nodes that don't have any self time. And for call nodes that do have self time, in most cases, the "self" samples all have the same category + subcategory. This means that at most one "self" bar is non-zero.

julienw commented 5 months ago

Got another idea about the border: using a border-right instead of a border-top/bottom: image

wdyt @mstange ?

julienw commented 5 months ago

Another follow-up: I think all meter bars in the tooltip should use the same scale. In the deploy preview, the length of the bar in a subcategory is relative to its category's value. This is very misleading.

Yeah I was also wondering about that (see the PR description). 2 options:

  1. all categories + subcategories use the same scale in the whole tooltip => smaller values are super small and barely visible
  2. or like before: each category + subcategory have their own scale => it's easier to see the proportion of a subcategory within its category, but then it's difficult to compare the proportion of each category with each other.

Because it's so easy to change I'd like to get this right in this PR. So here are a few screenshots to compare all these options.

Current deploy preview: image

Option 1: image

Option 2: image

I think I'd rather go with option 1. Then there are many things to do in follow-ups (possibly):

julienw commented 5 months ago

I pushed option 1 to the PR, so that it's ready to be merged if we all agree on that.