Uh oh!
There was an error while loading. Please reload this page.
More typed arrays in the FrameTable (and a flags column) - #6173
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@## main #6173 +/- ##
==========================================
+ Coverage 83.79% 83.82% +0.02%
==========================================
Files 350 350 Lines 37583 37744 +161 Branches 10459 10646 +187 ==========================================
+ Hits 31492 31638 +146 - Misses 5664 5677 +13 - Partials 427 429 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b9ad978 to
c49cdaeComparec369b1d to
ab6901bCompare
canova
left a comment
There was a problem hiding this comment.
Thanks! Looks mostly good to me, but I have a question, it looks like we are changing the old v70 upgrader. Was that intentional?
Uh oh!
There was an error while loading. Please reload this page.
| length, | ||
| } = frameTable; | ||
| const flags = new Array<number>(length); | ||
| for (let i = 0; i < length; i++) { |
There was a problem hiding this comment.
The following code is pretty hard to read, can we maybe add an inline FrameFlag enum snapshot inside this upgrader?
| if (inlineDepth[i] > 0) { | ||
| f |= 1 << 0; | ||
| } | ||
| if (address[i] !== -1 && lib[i] !== null) { |
There was a problem hiding this comment.
shouldn't this be something like if (address[i] !== -1 && lib[i] !== -1 && lib[i] !== null)? v70 upgrader puts -1 as a "no library" sentinel before this PR. I don't know why we are changing the v70 also, so maybe we don't need null?
There was a problem hiding this comment.
Oops. Restored the 70 upgrader and fixed the 71 upgrader to do things properly.
| // Frames with no library use the sentinel value -1. | ||
| const { frameTable, funcTable, resourceTable } = profile.shared; | ||
| const libForFunc = new Int32Array(funcTable.length).fill(-1); | ||
| const libForFunc = new Array(funcTable.length).fill(null); | ||
| for (let funcIndex = 0; funcIndex < funcTable.length; funcIndex++) { | ||
| const resourceIndex = funcTable.resource[funcIndex]; | ||
| if (resourceIndex !== -1) { | ||
| libForFunc[funcIndex] = resourceTable.lib[resourceIndex] ?? -1; | ||
| libForFunc[funcIndex] = resourceTable.lib[resourceIndex] ?? null; | ||
| } | ||
| } | ||
| const lib = new Array(frameTable.length).fill(-1); | ||
| const lib = new Array(frameTable.length).fill(null); |
There was a problem hiding this comment.
Is this intentional? Why are we changing the older v70 upgrader?
| } else { | ||
| originalLocation[i] = 0; | ||
| } | ||
| flags[i] = f; |
There was a problem hiding this comment.
I also don't see innerWindowID check for null here. I think we should add it too.
There was a problem hiding this comment.
This commented function is outdated now right? I think we should either remove or update with the new style.
So far, the columns we've converted to typed arrays were either always non-empty, or they used -1 as the "no value" sentinel. For the FrameTable, I've opted to not to use -1 sentinel values, and instead have a flags column with a "HasXYZ" flag for each column (e.g. FrameFlags.HasAddress), for the following reasons: - I'd like to use U8 and U16 arrays for the category and subcategory columns. That's already what we use in the derived StackTable. With an external flag we can go up to 256 subcategories without having to use 16 bits. - I'm changing the inlineDepth column into just a "is inline" bool. This is a good fit for a flag. - The address column was using -1 as the "no address" sentinel; having the flag lets us use U32 instead of I32 for the address column and have relative addresses for 4GiB binaries rather than just 2GiB. An alternative I considered (but discarded) was to have a bitset for each column. E.g. one "isInlined" bitset where, in the JSON, each item is a U8 with the bits for 8 different frames. And similarly we could have a "hasCategory" bitset. But that was too many extra columns in my opinion.
Uh oh!
There was an error while loading. Please reload this page.
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: 高乐喆
Main | Deploy preview
This allows all columns of the frame table to be stored as typed arrays, and reduces the amount of JSON parsing when loading a JSLB profile.
Size profile before: https://share.firefox.dev/469V8zL
Size profile after: https://share.firefox.dev/4ijXlQp