Skip to content

Give the frameTable a lib column - #6258

Merged
mstange merged 3 commits into
firefox-devtools:mainfrom
mstange:push-punossrmnotm
Aug 18, 2026
Merged

Give the frameTable a lib column#6258
mstange merged 3 commits into
firefox-devtools:mainfrom
mstange:push-punossrmnotm

Conversation

@mstange

@mstangemstange commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Main | Deploy preview

In the past, the way to get the library for a frame would be to go frame -> func -> resource -> lib. For native frames, the resource is of type Library and points at the lib.

This had the following implications:

  • Native frames cannot ever have a resource of a non-Library type.
  • If two native frames want to share a func, they must also share the same library - otherwise they'll be in a different resource, and different resource means different func.

This makes it hard to represent a few cases:

  • JS JIT frames with assembly. To have assembly, we must have a native library, but we also want JIT frames to have a JS file as their resource, and we want JITted frames for a JS function to share the func with non-JITted frames (interpreter frames) of the same JS function.
  • Comparison profiles / the "diff" thread for different builds, with assembly code. For example if you have two Firefox builds, one with and one without a patch, they'll have different libxul.so libraries, but you still want to combine C++ functions of the same name in the diffed tree.
  • Resources for Rust crates (not implemented here): We might want to assign different Rust frames from the same native binary to different resources, with one resource per Rust crate.

This commit makes it so that frames now point at their library directly, and removes the lib column from the resourceTable. A resource of type Library only carries the library's name; several libs can share one.

mergeLibs used to key on name + debugName, which collapsed two builds of the same library into one lib (and one resource). Now it keeps the libs separate but still collapses the resources by name.

For func-only contexts there is no longer a func -> lib edge. formatFunctionNameWithLibrary and the new getLibNameForFunc read the name off the func's resource, which is all they ever needed. profile-query's functionInfo, which wants the full Lib record, scans the frame table once per query.

@codecov

codecovBot commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.47368% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.79%. Comparing base (1198e50) to head (9203f11).
⚠️ Report is 50 commits behind head on main.

Files with missing linesPatch %Lines
src/profile-query/function-list.ts42.85%8 Missing ⚠️
src/profile-query/index.ts0.00%3 Missing ⚠️
src/profile-logic/symbolication.ts96.22%2 Missing ⚠️
src/profile-logic/bottom-box.ts50.00%1 Missing ⚠️
src/profile-query/formatters/marker-info.ts80.00%1 Missing ⚠️
src/profile-query/formatters/thread-info.ts80.00%1 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #6258 +/- ##
==========================================
+ Coverage 83.73% 83.79% +0.05% 
==========================================
Files 350 350 Lines 37523 37583 +60 Branches 10543 10459 -84 ==========================================
+ Hits 31420 31492 +72 + Misses 5676 5664 -12 
Partials 427 427 

☔ 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
mstangeforce-pushed the push-punossrmnotm branch 4 times, most recently from a2d6fdc to ea3fdb5CompareAugust 12, 2026 21:48
@mstange
mstange marked this pull request as ready for review August 12, 2026 21:48
@mstange
mstange requested a review from canovaAugust 12, 2026 21:48

@canovacanova 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 pretty good to me. I added some comments, it looks like the Int32Array conversion doesn't happen properly, r+ with them fixed.

Comment threadsrc/types/profile.ts Outdated
Comment on lines +264 to +272
// The library is stored per frame rather than being reached via the frame's
// resource (frame -> func -> resource -> lib) so that resources and libraries
// can vary independently. Multiple libraries can share one resource: a
// comparison profile of two libxul.so builds has one resource named
// "libxul.so" but a separate lib for each build, and a profile which combines
// several Firefox runs has one webhost resource per origin but a separate
// jitdump lib per run. Keeping them separate also leaves room for resources
// that describe something other than a library, such as a Rust crate, without
// breaking the frame-to-library association.

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, is it useful to give the historical context here? I think we can trim it down a bit.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oh dear. Trimmed down.

Comment on lines +186 to +189
// The library of this function, if any.
// Note that, these days, funcs can be associated with multiple
// libraries, so this isn't the best representation anymore. In those
// cases this will be set to one of them, but it's arbitrary which one.

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.

Could you file an issue for this?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Filed #6270

Comment threadsrc/profile-logic/profile-data.ts Outdated
@@ -4466,12 +4467,8 @@ export function findAddressProofForFile(
if (address === null) {

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.

Oh noticed while looking at this function, but I think we missed this check while changing address. I think this check should be if (address === -1) too now. But it's unrelated to this PR.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good catch, fixed in a separate commit.

Comment threadsrc/profile-logic/bottom-box.ts Outdated
// comparison profile). Use the library of the first frame that has one.
let libIndex: IndexIntoLibs | null = null;
for (const frameIndex of callNodeFramePerStack.values()) {
const frameLib = frameTable.lib[frameIndex];

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.

frameIndex can be -1 for the stacks outside the call node's subtree. We should check that too otherwise frameLib will be undefined for those

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I realized that this libIndex was completely unused. I added a commit to remove it.


This decouples resources from libraries.

The `lib` column can optionally be stored as an `Int32Array`, for profiles loaded from [JsonSlabs](https://github.com/mstange/json-slabs/) files (.jslb, .jslb.gz). Regular JS / JSON arrays are still accepted - but note that `-1` (not `null`) must be used regardless of format.

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 was checking where we do this lib array to Int32Array conversion and couldn't find it. I think this needs to happen in finishRawFrameTableBuilder and convertSharedTablesEligibleColumns.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Oops. Added.

In the past, the way to get the library for a frame would be to go
frame -> func -> resource -> lib. For native frames, the resource is
of type Library and points at the lib.
This had the following implications:
- Native frames cannot ever have a resource of a non-Library type.
- If two native frames want to share a func, they must also share the
same library - otherwise they'll be in a different resource, and
different resource means different func.
This makes it hard to represent a few cases:
- JS JIT frames with assembly. To have assembly, we must have a native
library, but we also want JIT frames to have a JS file as their
resource, and we want JITted frames for a JS function to share the
func with non-JITted frames (interpreter frames) of the same JS function.
- Comparison profiles / the "diff" thread for different builds, with
assembly code. For example if you have two Firefox builds, one with and
one without a patch, they'll have different libxul.so libraries, but
you still want to combine C++ functions of the same name in the diffed
tree.
- Resources for Rust crates (not implemented here): We might want to assign
different Rust frames from the same native binary to different resources,
with one resource per Rust crate.
This commit makes it so that frames now point at their library directly,
and removes the lib column from the resourceTable. A resource of type
Library only carries the library's name; several libs can share one.
mergeLibs used to key on name + debugName, which collapsed two builds of
the same library into one lib (and one resource). Now it keeps the libs
separate but still collapses the resources by name.
For func-only contexts there is no longer a func -> lib edge.
formatFunctionNameWithLibrary and the new getLibNameForFunc read the name
off the func's resource, which is all they ever needed. profile-query's
functionInfo, which wants the full Lib record, scans the frame table once
per query.
frameTable.address uses -1 rather than null for "no address", so the
existing null check was always useless.
@mstange
mstange enabled auto-merge August 18, 2026 18:29
@mstange
mstange merged commit e156f70 into firefox-devtools:mainAug 18, 2026
21 checks passed
@fatadelfatadel mentioned this pull request Sep 3, 2026
fatadel added a commit that referenced this pull request Sep 3, 2026
Changes:
[Nazım Can Altınova] Update oxfmt 0.59.0 → 0.63.0 (major) (#6262)
[Andrew Creskey] Show which network requests were prefetched (#6259)
[Florian Quèze] profiler-cli: document the marker field:value search
syntax (#6265)
[Florian Quèze] profiler-cli: report one time base for text and JSON
output (#6266)
[Florian Quèze] profiler-cli: avoid a stack overflow on large marker
threads (#6264)
[Markus Stange] Give the frameTable a lib column (#6258)
[Florian Quèze] profiler-cli: accept --limit 0 as unlimited, and make
truncation loud (#6267)
[Florian Quèze] profiler-cli: report the network request count the
filters ran against (#6274)
[Nazım Can Altınova] Fix dark mode contrast of the warning icons in the
publish panel (#6280)
[Nazım Can Altınova] Extract the publish panel warning indicator into a
small component (#6282)
[Markus Stange] More typed arrays in the FrameTable (and a flags column)
(#6173)
[Nazım Can Altınova] Improve the profiler-cli publish script and
document the whole deployment in a better way (#6260)
[fatadel] 🔃 Sync: l10n -> main (September 3, 2026) (#6301)
[fatadel] Bump profiler-cli version to 0.9.0 (#6302)
And special thanks to our localizers:
es-CL: ravmn
nl: Mark Heijl
sv-SE: Andreas Pettersson
sv-SE: Luna Jernberg
sv-SE: Peter Kihlstedt
tr: Selim Şumlu
tr: giray
zh-CN: 高乐喆
Sign up for freeto 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

@mstange@canova