Make profile-conversion snapshots more compact and meaningful - #6152
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6152 +/- ##
==========================================
+ Coverage 83.50% 83.53% +0.02%
==========================================
Files 342 343 +1
Lines 36522 36776 +254
Branches 10125 10321 +196
==========================================
+ Hits 30497 30720 +223
- Misses 5597 5629 +32
+ Partials 428 427 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a0cb4dc to
fd7d04a
Compare
canova
left a comment
There was a problem hiding this comment.
Nice! I like the new assertProfileIntegrity! Also being able to see the full stacks etc. will make it easier to catch stuff.
| type ThreadSummary = { | ||
| name: string; | ||
| pid: string | number; | ||
| tid: string | number; | ||
| isMainThread: boolean; | ||
| sampleCount: number; | ||
| sampleWeightTotal: number | null; | ||
| markerCount: number; | ||
| markerNamesTop: string[]; | ||
| jsAllocationCount: number; | ||
| nativeAllocationCount: number; | ||
| nativeAllocationWeightTotal: number | null; | ||
| weightType: string; | ||
| }; | ||
|
|
||
| export type ProfileSummary = { | ||
| meta: { | ||
| product: string; | ||
| importedFrom: string | undefined; | ||
| interval: number; | ||
| version: number; | ||
| preprocessedProfileVersion: number; | ||
| symbolicated: boolean | undefined; | ||
| categoryNames: string[]; | ||
| markerSchemaNames: string[]; | ||
| }; | ||
| sharedCounts: { | ||
| funcs: number; | ||
| frames: number; | ||
| stacks: number; | ||
| resources: number; | ||
| nativeSymbols: number; | ||
| strings: number; | ||
| libs: number; | ||
| }; | ||
| threads: ThreadSummary[]; | ||
| }; |
There was a problem hiding this comment.
Ah yeah, they kinda look like the summaries that we have in the profiler-cli. But I think it makes sense to keep them split. For example, I don't think sharedCounts makes sense for the cli.
| "nativeAllocationCount": 0, | ||
| "nativeAllocationWeightTotal": null, | ||
| "pid": "7567", | ||
| "sampleCount": 5, |
There was a problem hiding this comment.
It looks like for some cases these stacks can get pretty long in a line. I don't have an idea to improve it though. I think that's still an improvement over the ones that we had before, especially since we couldn't see the stacks easily.
There was a problem hiding this comment.
Yeah. We can change it once it becomes a problem.
| ); | ||
| } | ||
| const cat = markers.category[i]; | ||
| if (cat < 0 || cat >= numCategories) { |
There was a problem hiding this comment.
I think we should also check for null/undefined here?
There was a problem hiding this comment.
I went ahead and added checks for Number.isInteger everywhere.
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.
fd7d04a to
b33aa23
Compare
Changes: [Sky Ning] Skip preview links for non-main PRs (#6161) [spokodev] fix(gecko-upgrade): don't crash on a counter with empty sample_groups (#6160) [fatadel] Show counter values over time in profiler-cli (#6136) [Markus Stange] Make profile-conversion snapshots more compact and meaningful (#6152) [Markus Stange] More typed arrays: sample + counter times, some frametable columns (#6139) [Nazım Can Altınova] Only render a marker url field as a link when the whole value is a URL (#6163) [fatadel] Show each counter's owning process in profiler-cli (#6164) [Nazım Can Altınova] Document the pre-existing thread info and network JSON schemas in the cli (#6171) [Markus Stange] Copy column contents in getRawSamplesTableBuilderFromExisting for consistency (#6168) [Markus Stange] Convert eligible columns to typed arrays when outputting from profiler-edit (#6167) [Markus Stange] Remove unused samples.thread column (#6151) [Markus Stange] Fixed botched merge which broke 'yarn ts' (#6174) [Nazım Can Altınova] Add marker handles to `profiler-cli thread network` (#6172) [Markus Stange] Update json-slabs 0.3.0 → 0.4.0 (major) (#6176) [Nazım Can Altınova] Surface network activity across profiler-cli (#6175) [Nazım Can Altınova] Add `profile meta` command to profiler-cli (#6177) [Markus Stange] Allow raw marker table's `startTime` and `endTime` columns to be Float64Array (#6169) [nightcityblade] Fix light theme text selection colors (#6186) [Nazım Can Altınova] Import source map URLs from Chrome DevTools traces (#6190) [Nazım Can Altınova] Rename yarn `build-profiler-cli` script to `build-cli` (#6191) [Nazım Can Altınova] Migrate husky to version 9 (#6201) [Nazım Can Altınova] Fix horizontal overflow when the transform navigator is long (#6199) [fatadel] Add a 'hexadecimal' marker schema field format (#6197) [Nazım Can Altınova] Bump source-map to 0.8.0 and remove the old type workaround (#6202) [Nazım Can Altınova] 🔃 Sync: l10n -> main (July 21, 2026) (#6209) And special thanks to our localizers: fr: parmegiani.thomas fr: Théo Chevalier sr: Марко Костић (Marko Kostić) sv-SE: Luna Jernberg tr: Grk zh-CN: Ariel zh-CN: Olvcpr423
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.