-
Notifications
You must be signed in to change notification settings - Fork 430
Speed up createCallNodeTable by 2.3x #5248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5248 +/- ##
=======================================
Coverage 85.96% 85.97%
=======================================
Files 312 312
Lines 30331 30357 +26
Branches 8295 8296 +1
=======================================
+ Hits 26073 26098 +25
- Misses 3661 3662 +1
Partials 597 597 ☔ View full report in Codecov by Sentry. |
f7534bc
to
4104ce1
Compare
4104ce1
to
208376b
Compare
Flagging nazim because I'll be PTO the next few days. But I can look at it when I come back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me!
src/profile-logic/profile-data.js
Outdated
const funcIndex = frameTableFuncCol[frameIndex]; | ||
funcCol[callNodeIndex] = funcIndex; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: I'd group these into just one line and remove the funcIndex
constant. As it is written now, it looks like we'll reuse funcIndex
later (but we don't).
|
||
const innerWindowID = frameTableInnerWindowIDCol[frameIndex]; | ||
if (innerWindowID !== null) { | ||
innerWindowIDCol[callNodeIndex] = innerWindowID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: what do you think about adding a comment reminding that the TypedArray is initialized with "0"?
src/profile-logic/profile-data.js
Outdated
hierarchy; | ||
|
||
if (length === 0) { | ||
throw new Error('Empty call node table hierarchy'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit worried by this throw. Would it be possible to return something sensible instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, replaced this with a return.
…umns until we know the final size and order of the call node table.
05d9963
to
c64b01b
Compare
[Julien Wajsberg] Update node to v22 (#5378) [Markus Stange] Speed up createCallNodeTable by 2.3x (#5248) [Markus Stange] Remove frameTable.implementation from the processed format. (#5370) [Florian Quèze] Show size units in the timeline for profiles where profile.meta.sampleUnits.time is "bytes". (#5364) [Sean Kim] Report nsIRequest::status (nsresult) in the marker (#5375) [Markus Stange] Change the marker schema to accept a description and get rid of the notion of static fields (#5385) Also thanks to our localizers: en-CA: Paul tr: Grk
Production | Deploy preview
Built on top of #4900, but the only real dependency is on the changeset that makes the
inlinedInto
column a typed array.Before: https://share.firefox.dev/4fX1MwP (1884 samples in
C
akacomputeCallNodeTable
)After: https://share.firefox.dev/4gjkTAT (804 samples in
C
akacomputeCallNodeTable
, 2.3x faster)Main unminified: https://share.firefox.dev/4fkDOdP
After #4900 unminified: https://share.firefox.dev/4g0ujS7
After #5248 unminified: https://share.firefox.dev/41kpGhe