Make stackTable (sub)category derived data #5342

mstange · 2025-01-23T03:13:45Z

With this commit, we stop storing the stackTable's category and subcategory columns in the file, and compute them at runtime instead. This reduces profile sizes.

The upgrader tries to keep the profile https://share.firefox.dev/3ClJLcK from issue #5254 working, which is missing the frameTable's category + subcategory columns, but mostly just because it's easy. This profile is missing other columns too, so it may stop working in the future as we make other changes and start relying on the other columns that the profile is missing.

codecov · 2025-01-23T03:24:55Z

Codecov Report

Attention: Patch coverage is 87.50000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 85.93%. Comparing base (34710b2) to head (3c8c670).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/profile-logic/processed-profile-versioning.js	40.00%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5342      +/-   ##
==========================================
- Coverage   85.97%   85.93%   -0.04%     
==========================================
  Files         311      311              
  Lines       29831    29812      -19     
  Branches     8224     8222       -2     
==========================================
- Hits        25647    25620      -27     
- Misses       3593     3601       +8     
  Partials      591      591

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

julienw

Thanks, this looks mostly good to me.

I do have 2 questions though:

about the fix for broken profile data
about the call to computeStackTableFromRawStackTable when computing thread scores

I'm curious to see your answers!

julienw · 2025-01-27T16:38:36Z

docs-developer/CHANGELOG-formats.md

+
+The columns `category` and `subcategory` were removed from the `stackTable`, to reduce the file size of profiles. The information in these columns was fully redundant with the category information in the `frameTable`. A stack's category and subcategory are determined as follows: If the stack's frame has a non-null category, then that's the stack's category, and the frame's subcategory (or 0 if null) becomes the stack's subcategory. Otherwise, if the stack is not a root node, it inherits the category and subcategory of its prefix stack. Otherwise, it defaults to the defaultCategory, which is defined as the first category in `thread.meta.categories` whose color is `grey` - at least one such category is required to be present. And the subcategory defaults to zero - all categories are required to have a "default" subcategory as their first subcategory.
+
+The `frameTable`'s `category` column now becomes essential. It was already required in the previous profile version, but the UI would mostly work even if it wasn't present. There are some existing profiles in rotation which are missing this column in the `frameTable`. The 51->52 upgrader fixes such profiles up by inferring it from the information in the `stackTable`.


You meant this I guess? :

Suggested change

The `frameTable`'s `category` column now becomes essential. It was already required in the previous profile version, but the UI would mostly work even if it wasn't present. There are some existing profiles in rotation which are missing this column in the `frameTable`. The 51->52 upgrader fixes such profiles up by inferring it from the information in the `stackTable`.

The `frameTable`'s `category` column now becomes essential. It was already required in the previous profile version, but the UI would mostly work even if it wasn't present. There are some existing profiles in rotation which are missing this column in the `frameTable`. The 52->53 upgrader fixes such profiles up by inferring it from the information in the `stackTable`.

Oops, indeed!

julienw · 2025-01-27T16:39:18Z

profile.json.gz

I guess it's a mistake this file is here :-)

I think jj's auto-add behavior is what will finally make me change samply to not put the profile in the current directory...

julienw · 2025-01-27T16:41:02Z

src/types/profile.js

- * You could argue that the stack table's category column is derived data and as
- * such doesn't need to be stored in the profile itself. This is true, but


julienw · 2025-01-27T16:54:28Z

src/selectors/per-thread/thread.js

+    getRawStackTable,
+    getFrameTable,


(optional) similar comment than in an earlier patch, we could inline the selectors.

julienw · 2025-01-27T17:19:05Z

src/profile-logic/processed-profile-versioning.js

+      // Best-effort fix for profiles missing frameTable.category, such as the
+      // ones generated by Lean before https://github.com/leanprover/lean4/pull/6363


Would it make sense to do that in computeStackTableFromRawStackTable and make frameTable.category and frameTable.subcategory optional instead?
We wouldn't fix the frameTable but still get a good derived stack table.

The advantage would be to make it easier to generate profiles for the external tools that don't need categories.

By doing it in the upgrader you're catching existing broken past profiles but not future ones.

Would it make sense to do that in computeStackTableFromRawStackTable and make frameTable.category and frameTable.subcategory optional instead? We wouldn't fix the frameTable but still get a good derived stack table.

The profiles that this is targeting do have a category + subcategory field in the stackTable. If I remove them in the upgrader, and don't change the frameTable, then the information is gone.

The advantage would be to make it easier to generate profiles for the external tools that don't need categories.

I would actually go in the other direction - I think every profile wants categories these days, and we should make profile.meta.categories mandatory, too. The added touch of color in the UI is quite compelling, I think - not many people will want to opt out of that in their profiler generators.

By doing it in the upgrader you're catching existing broken past profiles but not future ones.

I am catching future ones that are created with the old version. And future ones with the new version will have the fix.

The profiles that this is targeting do have a category + subcategory field in the stackTable. If I remove them in the upgrader, and don't change the frameTable, then the information is gone.

ah that's true.

Also thanks for the other answers, makes sense!

julienw · 2025-01-27T17:21:28Z

src/profile-logic/processed-profile-versioning.js

+        for (let stackIndex = 0; stackIndex < stackTable.length; stackIndex++) {
+          const frameIndex = stackTable.frame[stackIndex];
+          frameTable.category[frameIndex] = stackTable.category[stackIndex];
+          frameTable.subcategory[frameIndex] =
+            stackTable.subcategory[stackIndex];
+        }


Frankly I'm not super happy to see the second part of this. I'm fine creating category if it's missing, filling it with the default category, but getting some more information from the stackTable feels too much. Should we fix every broken profile out there? I don't think so...

Tell me what you think!

Well, like I said, I only did this because it's easy. I don't think we should fix every broken profile out there. This was easy enough that I didn't see much harm in adding these 8 lines of code, especially in code that's pretty much write-once read-never.

Actually it's a bit more than 8 lines. I will remove it if you ask me to remove it.

I'm going to land this with the code included, but I'm happy to create a PR to remove it if you like.

Oh, nevermind, I will not land because this PR isn't approved.

I can't decide, so let's keep it

julienw · 2025-01-27T17:34:12Z

src/profile-logic/tracks.js

  const idleCategoryIndex = meta.categories
    ? meta.categories.findIndex((c) => c.name === 'Idle')
    : -1;
+  const derivedStackTable = computeStackTableFromRawStackTable(


I was a bit worried about that.

This means we'll do this computation twice for each thread, just because we want to know how many samples are idle.

I wonder if we can do better, but also this would probably be premature optimization, so we might as well wait and see.

What do you think?

We'll do it twice for each thread which doesn't have CPU deltas. Most modern profiles have CPU deltas. (One exception is Java threads in profiles from Firefox.)

Yeah I agree it's not ideal. We can try to do better if we see it show up in profiles. I think at the moment, for any profile that's big enough that this would be noticeable, we'll spend much more time in JSON.parse than in computing the derived stack table.

Stop storing them in the file, compute them at runtime. This reduces profile sizes.

julienw

thanks for the reasonable answers!

Updates: [Julien Wajsberg] Some more small refactorings (#5320) [Markus Stange] Pass the correct sample index offset to getTimingsForCallNodeIndex for the flame graph tooltip. (#5328) [Nisarg Jhaveri] Update docs to include Android Studio/Simpleperf trace file support (#5309) [Markus Stange] Don't pass the preview filtered thread to getTimingsForPath/CallNodeIndex. (#5329) [Nazım Can Altınova] Add a "Sample timestamp" field to the sample tooltip in timeline (#5322) [Markus Stange] Reduce confusion between call tree summary strategy aware samples and regular samples (#5330) [Markus Stange] Rename this getCounter selector to getCounters. (#5337) [Markus Stange] Make sample indexes compatible between the unfiltered and (preview) filtered call tree summary strategy samples when using an allocation strat> [Markus Stange] Remove some code that uses the preview filtered thread (#5336) [Markus Stange] Remove getMarkerSchemaName special cases - look up marker schemas from data.type and nothing else (#5293) [Markus Stange] Remove the makeProfileSerializable step - make the raw in-memory profile match the format that's stored in the file (#5287) [Nicolas Chevobbe] Adapt FilterNavigatorBar to High Contrast Mode. (#5257) [Nicolas Chevobbe] Adapt Tracks to High Contrast Mode. (#5252) [Markus Stange] Adjust string index fields in markers when merging threads (#5344) [Theodoros Nikolaou] Localize title and aria label in ProfileName (#5345) [Julien Wajsberg] Adapt time-slice selection in High Contrast Mode. (#5259) [Markus Stange] Make stackTable (sub)category derived data (#5342) [Markus Stange] Compute cpuRatio values when computing the derived thread (#5288) [Nazım Can Altınova] Add a context menu item to open the JS scripts in DevTools debugger (#5295) Also thanks to our localizers: el: Jim Spentzos fr: Théo Chevalier it: Francesco Lodolo [:flod] zh-TW: Pin-guang Chen

mstange requested a review from julienw January 23, 2025 03:13

mstange self-assigned this Jan 23, 2025

mstange force-pushed the stacktable-derived-category branch from 38ae6a3 to b051a5e Compare January 23, 2025 03:19

mstange force-pushed the stacktable-derived-category branch from b051a5e to 413ca1e Compare January 23, 2025 03:28

julienw reviewed Jan 27, 2025

View reviewed changes

Make stackTable (sub)category derived data.

386dec7

Stop storing them in the file, compute them at runtime. This reduces profile sizes.

mstange force-pushed the stacktable-derived-category branch from 413ca1e to 386dec7 Compare January 27, 2025 19:47

julienw approved these changes Jan 29, 2025

View reviewed changes

Merge branch 'main' into stacktable-derived-category

3c8c670

mstange merged commit b4430d0 into firefox-devtools:main Jan 29, 2025
18 checks passed

canova mentioned this pull request Jan 30, 2025

Deploy Jan 30, 2025 #5348

Merged

This was referenced Feb 13, 2025

Existing profiles from vernier fail to load #5368

Closed

Extend the workaround in the v53 upgrader to generate missing subcategory columns. #5369

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make stackTable (sub)category derived data #5342

Make stackTable (sub)category derived data #5342

mstange commented Jan 23, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

julienw left a comment

julienw Jan 27, 2025

mstange Jan 27, 2025

julienw Jan 27, 2025

mstange Jan 27, 2025

julienw Jan 27, 2025

julienw Jan 27, 2025

mstange Jan 27, 2025

julienw Jan 27, 2025

mstange Jan 27, 2025

julienw Jan 29, 2025

julienw Jan 27, 2025

mstange Jan 27, 2025

mstange Jan 27, 2025

mstange Jan 27, 2025

mstange Jan 27, 2025

julienw Jan 29, 2025

julienw Jan 27, 2025

mstange Jan 27, 2025 •

edited

Loading

julienw left a comment


		The columns `category` and `subcategory` were removed from the `stackTable`, to reduce the file size of profiles. The information in these columns was fully redundant with the category information in the `frameTable`. A stack's category and subcategory are determined as follows: If the stack's frame has a non-null category, then that's the stack's category, and the frame's subcategory (or 0 if null) becomes the stack's subcategory. Otherwise, if the stack is not a root node, it inherits the category and subcategory of its prefix stack. Otherwise, it defaults to the defaultCategory, which is defined as the first category in `thread.meta.categories` whose color is `grey` - at least one such category is required to be present. And the subcategory defaults to zero - all categories are required to have a "default" subcategory as their first subcategory.

		The `frameTable`'s `category` column now becomes essential. It was already required in the previous profile version, but the UI would mostly work even if it wasn't present. There are some existing profiles in rotation which are missing this column in the `frameTable`. The 51->52 upgrader fixes such profiles up by inferring it from the information in the `stackTable`.

		* You could argue that the stack table's category column is derived data and as
		* such doesn't need to be stored in the profile itself. This is true, but

		// Best-effort fix for profiles missing frameTable.category, such as the
		// ones generated by Lean before https://github.com/leanprover/lean4/pull/6363

Make stackTable (sub)category derived data #5342

Make stackTable (sub)category derived data #5342

Conversation

mstange commented Jan 23, 2025

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

julienw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstange Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

julienw left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 23, 2025 •

edited

Loading

mstange Jan 27, 2025 •

edited

Loading