Replace 4 counter track components with a single generic TrackCounter by fatadel · Pull Request #5944 · firefox-devtools/profiler

fatadel · 2026-04-10T12:18:42Z

Collapse the separate Memory, Power, ProcessCPU, and Bandwidth track
implementations into a single TrackCounter component that renders any
counter type using CounterDisplayConfig from PR #5912.

The LocalTrack union is simplified from 8 variants to 5 by replacing
'memory', 'power', 'process-cpu', and 'bandwidth' with a single
'counter' type. The graph branches on display.graphType for canvas
drawing (accumulated vs rate) and on counter.category/counter.name
for tooltip rendering, while display.unit and display.label drive
value formatting and the track label.

Counter tracks get a single dedicated slot in LOCAL_TRACK_INDEX_ORDER;
existing URLs are remapped by a new v16 URL upgrader. Display ordering
between counters uses the new display.sortWeight field, falling back
to a natural sort on counter name.

Closes #5752.

Make counters self-describing in terms of rendering by adding `display` field of `CounterDisplayConfig` type. The value is derived from a counter's `category` and `name` fields. This data is sufficient to understand how a counter should be rendered allowing us to remove hardcoded logic for each counter. This is the first PR for issue firefox-devtools#5752.

codecov · 2026-04-10T12:21:37Z

Codecov Report

❌ Patch coverage is 83.77823% with 79 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.25%. Comparing base (c155fdf) to head (a5fd146).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/components/timeline/TrackCounterGraph.tsx	89.00%	30 Missing and 1 partial ⚠️
src/app-logic/url-handling.ts	80.53%	22 Missing ⚠️
src/profile-logic/tracks.ts	63.63%	12 Missing ⚠️
src/selectors/app.tsx	0.00%	6 Missing ⚠️
src/test/fixtures/profiles/tracks.ts	50.00%	3 Missing ⚠️
src/components/timeline/TrackCounter.tsx	75.00%	2 Missing ⚠️
src/profile-logic/processed-profile-versioning.ts	50.00%	2 Missing ⚠️
src/profile-logic/process-profile.ts	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5944      +/-   ##
==========================================
- Coverage   85.33%   85.25%   -0.09%     
==========================================
  Files         323      317       -6     
  Lines       32262    31935     -327     
  Branches     8895     8842      -53     
==========================================
- Hits        27532    27226     -306     
+ Misses       4298     4277      -21     
  Partials      432      432

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

canova · 2026-04-10T13:28:38Z

Nice!

It looks like the order of the tacks have changed with this PR, is this intentional?
Before:

After:

Compositor is above the other counter tracks.

canova

I'm a bit worried about the way we decide how we figure out which tooltip to show. Currently it works, but it looks brittle considering that another counter could have similar fields. I'm still leaning towards defining tooltip fields inside the profile format. But @fqueze was saying that it was difficult to make it generic. I think I would be okay with losing the l10n part of the tooltips to make it generic, so we can define all the things like the label and what it should display. What do you think?

canova · 2026-04-10T14:28:46Z

I was thinking about something like this that we can define in the counter schema:

 type CounterTooltipDataSource =
    | 'count'            // samples.count[i]
    | 'rate'             // count / sampleTimeDelta
    | 'cpu-ratio'        // rate / maxCounterSampleCountPerMs
    | 'accumulated'      // accumulatedCounts[i] - minCount
    | 'count-range'      // total range across the visible graph
    | 'selection-total'  // sum over the preview selection
    | 'sample-number';   // samples.number[i] — row is omitted when null

  type CounterTooltipFormatter = 'bytes' | 'bytes-per-second' | 'percent' | 'number';

  type CounterTooltipRow =
    | { type: 'value'; source: CounterTooltipDataSource; format: CounterTooltipFormatter; label: string }
    | { type: 'separator' };

  // In CounterDisplayConfig:
  tooltipRows: CounterTooltipRow[];

And then we could use that like:

  tooltipRows: [
    { type: 'value', source: 'accumulated',  format: 'bytes',  label: 'relative memory at this time' },
    { type: 'value', source: 'count-range',  format: 'bytes',  label: 'memory range in graph' },
    { type: 'value', source: 'sample-number', format: 'number', label: 'allocations/deallocations since previous sample' },
  ]

But not so sure if it works with all the counters that we have.

fqueze · 2026-04-10T14:49:11Z

I'm a bit worried about the way we decide how we figure out which tooltip to show. Currently it works, but it looks brittle considering that another counter could have similar fields. I'm still leaning towards defining tooltip fields inside the profile format. But @fqueze was saying that it was difficult to make it generic. I think I would be okay with losing the l10n part of the tooltips to make it generic, so we can define all the things like the label and what it should display. What do you think?

Ideally we would have a generic implementation defined in the counter schema inside the profile JSON, and we can have specific overrides for specific counters (eg power, to show the CO2e values), similar to how network markers are handled differently.

canova · 2026-04-10T15:33:26Z

Yeah that sounds good. Also, adding context here after talking about it in sync. It would make sense to keep a display.tooltipType (or named something like this), that holds 'memory', 'power' etc. as a first step. And then we can think about making it more generic as a follow-up.

Collapse the separate Memory, Power, ProcessCPU, and Bandwidth track implementations into a single TrackCounter component that renders any counter type using CounterDisplayConfig from PR firefox-devtools#5912. The LocalTrack union type is simplified from 8 variants to 5 by replacing 'memory', 'power', 'process-cpu', and 'bandwidth' with a single 'counter' type. The component branches on display.graphType for canvas drawing (accumulated vs rate) and on display.unit for tooltip rendering (bytes, pWh, percent, etc.). Track index ordering (for URL backward compatibility) is handled by a category-based mapping function. Display ordering uses the new display.sortWeight field. Part of firefox-devtools#5752.

fatadel · 2026-04-13T09:09:03Z

Nice!

It looks like the order of the tacks have changed with this PR, is this intentional? Before: %img% After: %img%

Compositor is above the other counter tracks.

No, this was not intentional. I have realized that I missed it because the profile I've used did not have the Compositor track.
The issue was that the new display.sortWeight for counters uses values in multiples of 10 (10, 20, 30, etc.), however, the old values (LOCAL_TRACK_DISPLAY_ORDER) for everything else were all < 10 (basically, from a different value space). Now I've fixed it by bringing everything to the same value space (multiples of 10).

fatadel · 2026-04-13T14:06:26Z

Thanks for the feedback on tooltip rendering, @fqueze and @canova ! After thinking about it and consulting with Claude, here's the approach I'd like to propose. It combines @fqueze's idea of "generic default + specific overrides" with @canova's suggestion of a tooltipType field.

The concept: generic by default, with opt-in overrides for rich tooltips

We add an optional tooltipType field to CounterDisplayConfig:

export type CounterDisplayConfig = {
// ... existing fields ...

// When set, the frontend uses a dedicated tooltip implementation for this                                                                                                                                                                              
// counter type. When absent/null, the frontend renders a generic tooltip
// based on the counter's unit (e.g., bytes → formatBytes, percent →                                                                                                                                                                                    
// formatPercent).                                                                                                                                                                                                                                      
tooltipType?: 'memory' | 'power' | 'bandwidth' | 'process-cpu' | null;

};

The logic in the component is:

If tooltipType is set → use the dedicated tooltip implementation (these are the existing tooltips we already have, preserved as-is)
If tooltipType is absent/null → render a generic tooltip that formats the value based on unit

Wdyt about it @fqueze @canova ?

canova · 2026-04-14T14:15:58Z

(apparently the comment I wrote in the morning wasn't sent, and I lost the message. rewriting)

I've thought about it a bit more and discussed with @fqueze and I think it might make sense to not add tooltipType at all actually. Because this field will be replaced by the more generic version of tooltip representation soon (either my solution above or a better one), so we might as well not add this for tooltipType for now.

In the code where we decide which tooltip to show, instead of this field, we can look at the counter.category and counter.name to decide which tooltip to share. Since we already deduct this tooltipType by looking at the counter names and categories, we can skip tooltipType all together. And once we have more generic ways to handle the tooltips, this custom handling would be removed.

This would prevent us from adding an intermediate display field that will be removed soon. WDYT?

fatadel · 2026-04-15T12:42:59Z

(apparently the comment I wrote in the morning wasn't sent, and I lost the message. rewriting)

I've thought about it a bit more and discussed with @fqueze and I think it might make sense to not add tooltipType at all actually. Because this field will be replaced by the more generic version of tooltip representation soon (either my solution above or a better one), so we might as well not add this for tooltipType for now.

In the code where we decide which tooltip to show, instead of this field, we can look at the counter.category and counter.name to decide which tooltip to share. Since we already deduct this tooltipType by looking at the counter names and categories, we can skip tooltipType all together. And once we have more generic ways to handle the tooltips, this custom handling would be removed.

This would prevent us from adding an intermediate display field that will be removed soon. WDYT?

Yeah, it makes sense. Instead of adding a tooltipType field to the format that we'd just remove later, we can use counter.category and counter.name directly in the tooltip rendering code to decide which custom tooltip to show.
I will change it now!

# Conflicts: # src/app-logic/constants.ts # src/components/timeline/TrackBandwidthGraph.tsx # src/components/timeline/TrackMemoryGraph.tsx # src/components/timeline/TrackPowerGraph.tsx # src/profile-logic/process-profile.ts # src/profile-logic/processed-profile-versioning.ts # src/test/components/TrackBandwidth.test.tsx # src/test/components/TrackMemory.test.tsx # src/test/fixtures/profiles/processed-profile.ts

…play prop

canova

Nice!

I spent quite some time reviewing this but unfortunately I'm not done yet. Pushing my review comments that I added so far so you can look at them sooner.

canova · 2026-04-20T10:11:27Z

-    // Power tracks are there only if the power feature is enabled. So they should
-    // be visible by default whenever they're included in a profile. (fallthrough)


How do we decide which track to make it visible by default now?

Could you elaborate on your question, please? Here specifically we were returning true for all the counter tracks, the same happens now.

canova · 2026-04-20T10:17:44Z

+            counters !== null &&
+            counters[track.counterIndex].display.markerSchemaLocation ===
+              'timeline-memory'


Previously we only checked for track.type === 'memory, why extra checks now?

Also, ideally getProcessesWithMemoryTrack should go away to make this generic. Can be done in a follow-up.

Oh I've missed this one. Yeah this can be simplified. Pushing now!

Regarding getProcessesWithMemoryTrack selector, I've created the following issue - #5962.

canova · 2026-04-20T10:51:09Z

+
+    const { category, name } = counter;
+
+    // Power tooltip — delegate to the dedicated component.


Could you file an issue for making the tooltip generic as a follow-up so we can discuss how we can make it generic and think about the counter schema for this one?

Good? #5961

Reshape LOCAL_TRACK_INDEX_ORDER and LOCAL_TRACK_DISPLAY_ORDER so that 'counter' occupies a real slot alongside the other local-track types. LOCAL_TRACK_DISPLAY_ORDER becomes the single source of truth for cross-type ordering; display.sortWeight is reduced to a tie-breaker between two counters. Add a v16 URL upgrader that remaps localTrackOrderByPid and hiddenLocalTracksByPid from the old per-counter-type indexes (memory=2, power=6, process-cpu=5, bandwidth=8) to the new single counter slot so existing URLs keep pointing at the same tracks.

fatadel · 2026-04-21T16:10:35Z

  Array [
    "moveTo",
    0,
-    24,


The single line-rate getY uses the zero-special-case from the old bandwidth code (if (!rawY) return deviceHeight + deviceLineHalfWidth), which pushes exact zeros just below the visible canvas so near-zero values are noticeable. Old TrackProcessCPUGraph didn't have this case - it drew zero samples on the baseline - which is why here the snapshot now shows moveTo(0, 26) instead of moveTo(0, 24). This keeps the rate path uniform across all counters. Happy to revert to per-counter behaviour if you'd rather preserve the old ProcessCPU rendering.

I see. I think it's okay to keep this behavior. And let's see if people are going to like it or not. It might be an improvement in general.

fatadel · 2026-04-21T16:18:39Z

Thanks for the early feedback, @canova! Please, take another look when you have time 🙏🏻

- Pass a boolean hasMarkers prop to TrackCounterGraph instead of the whole CounterDisplayConfig, and move the track wrapper's sizing into CSS so the TSX stays free of inline style. - Use a switch with assertExhaustiveCheck(display.graphType) in both the canvas draw and the hovered-sample dot, so adding a new graph type surfaces as a TypeScript error at those sites. - Replace the optional-chain + null-and-undefined check on counters with ensureExists, both in getTimelineHeight and in getProcessesWithMemoryTrack. - Drop a stray Math.round from the rate path's y-coordinate so rate counter snapshots match their pre-unification values.

canova

I'm done doing a full review now! Looks pretty good to me! I added some nits, but I have one blocking comment which is about using memory specific markers when markerSchemaLocation is specified. I think this will result us seeing memory related markers for all the counters that specify a markerSchemaLocation, so it would be good to make this part generic as well. See my comment below too.

Also I found another thing. This won't work anymore after this PR:

profiler/src/components/timeline/TrackContextMenu.tsx

Line 939 in edba2fd

'memory',

We can add this context menu item for all the counter tracks.

Also, I noticed that we don't actually touch the profile upgraders. So I think there is nothing that prevents us from doing a deploy, right?

fatadel · 2026-04-22T15:53:03Z

Also, I noticed that we don't actually touch the profile upgraders. So I think there is nothing that prevents us from doing a deploy, right?

Oh actually you're right. I was confusing src/profile-logic/process-profile.ts with src/profile-logic/processed-profile-versioning.ts.

canova · 2026-04-22T15:58:53Z

Also, I noticed that we don't actually touch the profile upgraders. So I think there is nothing that prevents us from doing a deploy, right?

Oh actually you're right. I was confusing src/profile-logic/process-profile.ts with src/profile-logic/processed-profile-versioning.ts.

And even the changes in src/profile-logic/process-profile.ts are just moving the if branches around, looks purely cosmetic, so it shouldn't produce any observable changes. You can also do that in the upgrader as long as you make sure that the behavior doesn't change.

fatadel · 2026-04-22T16:23:20Z

Also, I noticed that we don't actually touch the profile upgraders. So I think there is nothing that prevents us from doing a deploy, right?

Oh actually you're right. I was confusing src/profile-logic/process-profile.ts with src/profile-logic/processed-profile-versioning.ts.

And even the changes in src/profile-logic/process-profile.ts are just moving the if branches around, looks purely cosmetic, so it shouldn't produce any observable changes. You can also do that in the upgrader as long as you make sure that the behavior doesn't change.

Oh indeed! I now remember - I was reordering that by the sortWeight 😅 Which of the following would you prefer?

Keep everything as-is now.
Revert changes in src/profile-logic/process-profile.ts.
Keep changes in src/profile-logic/process-profile.ts and align src/profile-logic/processed-profile-versioning.ts with it (also re-order by sortWeight).

I vote for 3.

fatadel · 2026-04-22T16:24:34Z

By the way, thanks a lot for your review @canova! I will soon address all the issues.

canova · 2026-04-22T16:45:24Z

@fatadel Yeah, I would prefer 3 too. But still there is no reason to block the deploy :)

Rework TrackCounter so its optional marker row filters by the counter's own display.markerSchemaLocation rather than reusing the memory-specific TimelineMarkersMemory component. Introduces a per-thread selector factory getTimelineMarkerIndexesBySchemaLocation(location) with a location-keyed cache of memoized selectors, plus a new TimelineMarkersCounter component that plugs it in. TimelineMarkersMemory stays untouched for TrackThread's "show memory markers on the main thread when there's no memory track" fallback; the corresponding CSS rule keeps its height so that path is unaffected until the follow-up issue firefox-devtools#5962 retires it.

- computeLocalTracksByPid no longer drops power counters with at most two samples. - Counter-counter tie-break: keepProfileThreadOrder takes precedence over display.sortWeight in _getDefaultLocalTrackOrder. - Reorder the v62 processed-profile upgrader branches to match the sortWeight-ascending layout in _deriveCounterDisplay. - Drive --graph-height and --markers-height on the TrackCounter wrapper from the corresponding TS constants. - Accept 'counter' in TrackContextMenu's ALLOWED_TYPES. - Document why event-delay and process-cpu have no entries in the v16 upgrader's slot maps.

canova

Looks good! I added two more comments below but I think we can land after.

…ma-location selector Tightens the marker schema location field to MarkerDisplayLocation throughout the counter display config and consumers, removing the lingering string cast in the per-thread selector. The dedicated memory/IPC marker selectors are now built on top of getTimelineMarkerIndexesBySchemaLocation so there is a single source of truth for the schema-location filtering.

fatadel · 2026-04-27T08:21:08Z

@canova, thank you for the thorough review! I've addressed those issues now.

canova

Looks good to me, thanks!

fatadel changed the title ~~Generic counters 5752 pr 2~~ Replace 4 counter track components with a single generic TrackCounter Apr 10, 2026

fatadel requested a review from canova April 10, 2026 12:58

canova reviewed Apr 10, 2026

View reviewed changes

Comment thread src/components/timeline/TrackCounterGraph.tsx

Comment thread src/components/timeline/TrackCounterGraph.tsx Outdated

fatadel force-pushed the generic-counters-5752-pr-2 branch from cc362ba to 925b5f7 Compare April 13, 2026 08:33

fatadel added 2 commits April 15, 2026 14:51

Use counter.category/name for tooltip selection, remove redundant dis…

56a8355

…play prop

fatadel requested a review from canova April 16, 2026 11:17

canova reviewed Apr 20, 2026

View reviewed changes

fatadel commented Apr 21, 2026

View reviewed changes

fatadel requested a review from canova April 21, 2026 16:18

fatadel force-pushed the generic-counters-5752-pr-2 branch from c79f4a7 to 53d42a2 Compare April 22, 2026 09:31

canova requested changes Apr 22, 2026

View reviewed changes

fatadel mentioned this pull request Apr 24, 2026

Check the counter track activity to decide if it should be visible #5967

Open

fatadel requested a review from canova April 24, 2026 09:47

canova reviewed Apr 24, 2026

View reviewed changes

Comment thread src/selectors/per-thread/markers.ts

Comment thread src/selectors/per-thread/markers.ts Outdated

fatadel requested a review from canova April 27, 2026 08:21

canova approved these changes Apr 27, 2026

View reviewed changes

fatadel added 2 commits April 27, 2026 12:06

Merge branch 'main' into generic-counters-5752-pr-2

8113c76

Merge branch 'main' into generic-counters-5752-pr-2

a5fd146

fatadel merged commit 354d108 into firefox-devtools:main Apr 27, 2026
19 of 21 checks passed

fatadel deleted the generic-counters-5752-pr-2 branch April 27, 2026 10:57

		// Power tracks are there only if the power feature is enabled. So they should
		// be visible by default whenever they're included in a profile. (fallthrough)


		const { category, name } = counter;

		// Power tooltip — delegate to the dedicated component.

Conversation

fatadel commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

canova commented Apr 10, 2026

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

canova commented Apr 10, 2026

Uh oh!

fqueze commented Apr 10, 2026

Uh oh!

canova commented Apr 10, 2026

Uh oh!

fatadel commented Apr 13, 2026

Uh oh!

fatadel commented Apr 13, 2026

Uh oh!

canova commented Apr 14, 2026

Uh oh!

fatadel commented Apr 15, 2026

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fatadel commented Apr 21, 2026

Uh oh!

canova left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fatadel commented Apr 22, 2026

Uh oh!

canova commented Apr 22, 2026

Uh oh!

fatadel commented Apr 22, 2026

Uh oh!

fatadel commented Apr 22, 2026

Uh oh!

fatadel commented Apr 10, 2026 •

edited

Loading

codecov Bot commented Apr 10, 2026 •

edited

Loading