Skip to content

Make profile-conversion snapshots more compact and meaningful#6152

Open
mstange wants to merge 2 commits into
firefox-devtools:mainfrom
mstange:push-lzsvmxpoywyu
Open

Make profile-conversion snapshots more compact and meaningful#6152
mstange wants to merge 2 commits into
firefox-devtools:mainfrom
mstange:push-lzsvmxpoywyu

Conversation

@mstange

@mstange mstange commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Main | Deploy preview

profile-conversion.test.ts was creating a snapshot file that was just a bunch of raw profile JSONs, with around 900 000 lines total.

There's no way to catch regressions with such a test. Whenever we change the shape of the tables in the profile format, it affects so many lines that github's diff viewer will not render the diff, and you wouldn't be able to meaningfully read it anyway.

For example, when I introduced the shared tables, I regressed the dhat importer (it was overwriting some tables with empty tables), and I missed the regression even though the test in theory "caught" it.

This commit replaces the raw profile snapshots with profile "summary" snapshots. See the new snapshot file for examples. I think the new snapshots are definitely more readable / reviewable, but there's also quite a bit of new code to generate them. I'd be happy to cut back on the amount of detail we collect when we make these summaries, to simplify that new code.
(Or maybe we can find a way to share some code with profiler-cli? Not sure.)

The new snapshot file is around 500 lines, and around 160 times smaller than the old snapshot file.

As a second measure of defense, this commit also adds a generic profile consistency checker function called assertProfileIntegrity. This would have caught the dhat regression by itself, because nativeAllocations.stack[0] = 55 would have tripped over the stack table being empty.

@mstange mstange requested a review from canova July 7, 2026 19:43
@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.71429% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.51%. Comparing base (b2c0eba) to head (fd7d04a).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/test/fixtures/profile-summary.ts 85.71% 21 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #6152    +/-   ##
========================================
  Coverage   83.50%   83.51%            
========================================
  Files         342      343     +1     
  Lines       36522    36669   +147     
  Branches    10224    10263    +39     
========================================
+ Hits        30497    30623   +126     
- Misses       5597     5618    +21     
  Partials      428      428            

☔ View full report in Codecov by Harness.
📢 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.

@mstange mstange force-pushed the push-lzsvmxpoywyu branch from 4d742c5 to a0cb4dc Compare July 7, 2026 19:52
mstange added 2 commits July 7, 2026 15:59
profile-conversion.test.ts was creating a snapshot file that was just a bunch
of raw profile JSONs, with around 900 000 lines total.

There's no way to catch regressions with such a test. Whenever we change the
shape of the tables in the profile format, it affects so many lines that
github's diff viewer will not render the diff, and you wouldn't be able to
meaningfully read it anyway.

For example, when I introduced the shared tables, I regressed the dhat
importer (it was overwriting some tables with empty tables), and I missed
the regression even though the test in theory "caught" it.

This commit replaces the raw profile snapshots with profile "summary"
snapshots. See the new snapshot file for examples. I think the new snapshots
are definitely more readable / reviewable, but there's also quite a bit of
new code to generate them. I'd be happy to cut back on the amount of detail
we collect when we make these summaries, to simplify that new code.
(Or maybe we can find a way to share some code with profiler-cli? Not sure.)

The new snapshot file is around 500 lines, and around 160 times smaller than
the old snapshot file.

As a second measure of defense, this commit also adds a generic profile
consistency checker function called `assertProfileIntegrity`. This would have
caught the dhat regression by itself, because nativeAllocations.stack[0] = 55
would have tripped over the stack table being empty.
@mstange mstange force-pushed the push-lzsvmxpoywyu branch from a0cb4dc to fd7d04a Compare July 7, 2026 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant