Skip to content

More additions to profiler-edit, for sp3 profiles#6009

Merged
mstange merged 5 commits into
firefox-devtools:mainfrom
mstange:sp3-profiler-edit-features
Jun 15, 2026
Merged

More additions to profiler-edit, for sp3 profiles#6009
mstange merged 5 commits into
firefox-devtools:mainfrom
mstange:sp3-profiler-edit-features

Conversation

@mstange

@mstange mstange commented May 8, 2026

Copy link
Copy Markdown
Contributor

This makes it so that you can run profiler-edit with --insert-label-frames browser_labels.toml --only-keep-threads-with-markers-matching='-async,-sync' --merge-non-overlapping-threads-by-name --set-name 'Sp3 5x (with labels, combined main threads)' to turn https://share.firefox.dev/4cXQFED into https://share.firefox.dev/4ezEcsa

The label frames are useful in the JS-only view when applied to profiles from samply; they let us group work like "Paint" and "JS Parsing" (example toml). You can best see them in action in the function list deploy preview: JS-only, ordered by self, with the Self wing showing where the time is spent.

@codecov

codecov Bot commented May 8, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 17.20430% with 154 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.34%. Comparing base (2cb282f) to head (93787a0).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/profile-logic/merge-compare.ts 9.67% 112 Missing ⚠️
src/profile-logic/marker-data.ts 30.30% 23 Missing ⚠️
src/node-tools/profiler-edit.ts 32.14% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6009      +/-   ##
==========================================
- Coverage   83.69%   83.34%   -0.35%     
==========================================
  Files         338      338              
  Lines       35693    35868     +175     
  Branches     9904     9944      +40     
==========================================
+ Hits        29873    29895      +22     
- Misses       5392     5545     +153     
  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 sp3-profiler-edit-features branch from fd67c9e to f5ac119 Compare June 4, 2026 19:13
@mstange mstange marked this pull request as ready for review June 4, 2026 19:13
@mstange mstange force-pushed the sp3-profiler-edit-features branch from f5ac119 to 33ec551 Compare June 4, 2026 19:20
@mstange mstange marked this pull request as draft June 4, 2026 19:21
@mstange mstange force-pushed the sp3-profiler-edit-features branch 2 times, most recently from 508ee02 to 9d9f405 Compare June 4, 2026 20:03
@mstange mstange marked this pull request as ready for review June 4, 2026 20:03
@mstange mstange requested a review from canova June 4, 2026 20:03
@mstange mstange force-pushed the sp3-profiler-edit-features branch from 9d9f405 to e496d7c Compare June 4, 2026 20:05

@canova canova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to me! I noticed one thing though:

It looks like we are mutating profile.threads with both --only-keep-threads-with-markers-matching and --merge-non-overlapping-threads, but we also have some references to those threads array elements in counter's mainThreadIndex and profilerOverhead's mainThreadIndex. They are not being updated right now, which will reference a different thread after these operations.

I guess it's not super crucial for the sp3 profiles as they most likely don't contain counters (and we don't care about profilerOverhead) but it's good to be correct, so it's up to you if you want to land it now and follow-up later.

Comment thread src/profile-logic/marker-data.ts Outdated
Comment on lines +1032 to +1040
const frontEndSchemaNames = new Set(
markerSchemaFrontEndOnly.map((schema) => schema.name)
);
const schemaList = [
...(profile.meta.markerSchema ?? []).filter(
(schema) => !frontEndSchemaNames.has(schema.name)
),
...markerSchemaFrontEndOnly,
];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This is the same logic as the one in the getMarkerSchema selector. It would be good to extract that into a function and reuse here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a computeCombinedMarkerSchemaList function.

Comment thread src/profile-logic/marker-data.ts Outdated
Comment on lines +1041 to +1044
const markerSchemaByName: MarkerSchemaByName = Object.create(null);
for (const schema of schemaList) {
markerSchemaByName[schema.name] = schema;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Similarly this is the same logic as getMarkerSchemaByName.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a computeMarkerSchemaByName function.

Comment thread src/node-tools/profiler-edit.ts
Comment thread src/profile-logic/merge-compare.ts Outdated
Comment on lines +1678 to +1680
console.log(
`Matched ${mergedProcessBundles} non-overlapping process bundles. Merged ${mergedIndexes.size + mergeReplacements.size} threads into ${mergeReplacements.size}, going from ${threads.length} to ${newThreads.length} threads.`
);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to leave the console.log out of src/profile-logic/. Can we move that to the profiler-edit side?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll just remove it for now.

Comment thread src/profile-logic/merge-compare.ts Outdated
Comment on lines +1364 to +1367
// If every source thread has threadCPUDelta, carry the per-sample values
// through unchanged. For non-overlapping inputs the resulting deltas remain
// meaningful; for overlapping inputs the values are nonsensical but harmless
// (still numerically valid).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm not super sure about this. I guess it makes sense for non-overlapping threads. But for overlapping ones, I'm afraid that the data will be too weird. Have you tried it with overlapping threads? How do they look?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I haven't tried it but I agree that it's likely going to be garbage. Added a check for non-overlappingness and preserved the old behavior if overlapping.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, yeah that sounds better to me! Thanks!

@mstange mstange force-pushed the sp3-profiler-edit-features branch from e496d7c to d249c12 Compare June 15, 2026 21:46
@mstange

mstange commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

It looks like we are mutating profile.threads with both --only-keep-threads-with-markers-matching and --merge-non-overlapping-threads, but we also have some references to those threads array elements in counter's mainThreadIndex and profilerOverhead's mainThreadIndex. They are not being updated right now, which will reference a different thread after these operations.

Ah, good catch. I've added a commit at the end which updates those values.

@mstange mstange merged commit 1a7c76c into firefox-devtools:main Jun 15, 2026
21 of 23 checks passed
@canova canova mentioned this pull request Jun 16, 2026
canova added a commit that referenced this pull request Jun 16, 2026
Changes:

[Nazım Can Altınova] Fix call node context menu being hidden behind
source view bottom box (#6045)
[Nazım Can Altınova] Pass `--use-env-proxy` only when the node version
is >= 24 (#6064)
[fatadel] Upgrade @firefox-devtools/react-contextmenu to 5.2.4 (#6066)
[Markus Stange] Switch profiler-edit from minimist to commander (#6065)
[Markus Stange] Support reading profiles from JsonSlabs files (#6037)
[Florian Quèze] Don't fail profile processing when a marker's stack
field is not a backtrace (#6069)
[fatadel] Replace the footer-links overlay with a settings menu (#6042)
[fatadel] Upgrade @types/node to match Node 24 (#6070)
[fatadel] Remove unused undici-types package (#6074)
[cathaysia] Update isLocalURL to include LAN addresses, .local domains,
and hostn… (#5973)
[Markus Stange] Fix from-url with binary profiles (#6072)
[fatadel] Upgrade to React 19 (#6067)
[Markus Stange] Add an insertStackLabels helper. (#6076)
[fatadel] Drive counter tooltips from a tooltipRows schema (#6023)
[fatadel] Add TrackPower--tooltip-average-power-microwatt (#6080)
[Markus Stange] Downgrade to React 19.1 to fix unusable dev build
performance. (#6082)
[Nazım Can Altınova] Add source map symbolication and source view
support (#6018)
[spokodev] fix(FilterNavigatorBar): clip overflow so many breadcrumbs do
not expand the parent (#6085)
[Markus Stange] Move paddings inside the tree header cells. (#6002)
[Markus Stange] Add an --insert-label-frames argument to the
profiler-edit tool (#5966)
[Markus Stange] Stop printing "error: too many arguments" during tests.
(#6088)
[Markus Stange] More additions to profiler-edit, for sp3 profiles
(#6009)
[Nazım Can Altınova] Do not rely on localized texts in the settings menu
tests (#6101)

And special thanks to our localizers:

be: Andrei Mukamolau
de: Ger
de: Michael Köhler
de: Ralf Duehnfahr
el: Jim Spentzos
en-CA: chutten
en-GB: Ian Neal
es-CL: ravmn
fr: Théo Chevalier
fr: wy
fur: Fabio Tomat
fy-NL: Fjoerfoks
ia: Melo46
it: Francesco Lodolo [:flod]
nl: Mark Heijl
ru: Valery Ledovskoy
sr: Марко Костић (Marko Kostić)
sv-SE: Andreas Pettersson
tr: Grk
tr: Selim Şumlu
zh-CN: Olvcpr423
zh-TW: Pin-guang Chen
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.

2 participants