Repository navigation
Typed arrays + flags for the FuncTable - #6323
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6323 +/- ##
==========================================
+ Coverage 83.92% 84.03% +0.11%
==========================================
Files 353 354 +1
Lines 37969 38100 +131
Branches 10604 10662 +58
==========================================
+ Hits 31865 32018 +153
+ Misses 5675 5654 -21
+ Partials 429 428 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2de6033 to
7684dd5
Compare
canova
left a comment
There was a problem hiding this comment.
Thanks!
I see that this line still checks for source !== null which is always going to be true now:
https://github.com/mstange/perf.html/blob/7684dd5a8a9ff3ff7daa898d2ae37050fcca6f33/src/node-tools/profiler-edit.ts#L127-L128
We need to also change it to flag based checks.
I think this is more of a nit, but it might be also good to add an early return for this:
https://github.com/mstange/perf.html/blob/7684dd5a8a9ff3ff7daa898d2ae37050fcca6f33/src/actions/profile-view.ts#L2098-L2099
Technically it's a bug if we are here at this point with an invalid resource, so ideally it should never happen but previously we were doing a ensureExists which throws, now we will silently collapse the first resource.
| - `1 << 4` — `HasLine`: `lineNumber[i]` is meaningful. | ||
| - `1 << 5` — `HasColumn`: `columnNumber[i]` is meaningful. | ||
| - `1 << 6` — `HasOriginalLocation`: `originalLocation[i]` is meaningful. | ||
| - The `isJS` and `relevantForJS` boolean columns were removed; the `IsJS` and `RelevantForJS` flag bits carry that information. |
| resource[i] !== null && | ||
| resource[i] !== undefined | ||
| ) { | ||
| f |= 1 << 2; |
There was a problem hiding this comment.
Nit: would be nice to have constants that are defined in this upgrader for all the flags like HasResource etc.
There was a problem hiding this comment.
Probably should use flags here too instead?
| (funcTable.flags[funcIndex] & | ||
| (FuncFlag.IsJS | FuncFlag.RelevantForJS)) !== |
There was a problem hiding this comment.
Not for this PR, but now that we are having more bit flags and tests like this, I wonder if it makes sense to create a generic function for testing these flags to make things easier to read. Especially it's a footgun to forget the parenthesis before !== 0 or == 0 due to them being higher precedence (especially if people are not familiar with it).
But I'm also not sure about the perf overhead of it in hot loops in general. So I think this is perfectly fine.
7684dd5 to
a8970fd
Compare
|
Thanks for the review! I've addressed the comments. |
Changes: [fatadel] Show a category breakdown in profiler-cli (#6256) [fatadel] Show allocation data in the cli (#6246) [Florian Quèze] profiler-cli: name the process in the thread banner, and follow the queried thread (#6269) [fatadel] Drive PII sanitization from marker schemas (#6291) [fatadel] Move FileIO table labels into marker schemas (#6296) [Florian Quèze] profiler-cli: add a thread list command (#6273) [Nazım Can Altınova] Update oxfmt 0.63.0 → 0.68.0 (major) (#6316) [Markus Stange] Add missing finishRawMarkerTableBuilder. (#6318) [Markus Stange] Fix the upload error button contrast (#6315) [Markus Stange] Allow typed arrays in the nativeSymbols table (#6300) [Markus Stange] Remove brittle test. (#6322) [Markus Stange] Fix file extension typo. (#6325) [Nazım Can Altınova] Add a permalink command to profiler-cli for already published profiles (#6331) [Markus Stange] Typed arrays + flags for the FuncTable (#6323) [Florian Quèze] profiler-cli: add profile markers for cross-thread marker search (#6276) [fatadel] Convert extension text markers to structured payloads (#6314) [Florian Quèze] profiler-cli: include per-marker fields and data in --list --json (#6275) [Nazım Can Altınova] Add `--with-samply` to "profiler-cli load" (#6339) [Markus Stange] Speed up call tree sidebar by sharing work with the activity graph (#6329) [Nazım Can Altınova] 🔃 Sync: l10n -> main (Sept 22, 2026) (#6341) [Nazım Can Altınova] Bump profiler-cli version to 0.10.0 (#6342) And special thanks to our localizers: el: Jim Spentzos ro: robbp
Main | Deploy preview
Similar to what #6173 did for the frame table, but for the func table.