-
Notifications
You must be signed in to change notification settings - Fork 7
Improve heap profile memory usage by lazily loading js objects #260
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
base: main
Are you sure you want to change the base?
Conversation
Overall package sizeSelf size: 1.91 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | pprof-format | 2.2.1 | 163.06 kB | 163.06 kB | | p-limit | 3.1.0 | 7.75 kB | 13.78 kB | | delay | 5.0.0 | 11.17 kB | 11.17 kB | | node-gyp-build | 3.9.0 | 8.81 kB | 8.81 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
BenchmarksBenchmark execution time: 2026-02-09 13:54:00 Comparing candidate commit 4c77cf6 in PR branch Found 1 performance improvements and 1 performance regressions! Performance is the same for 90 metrics, 28 unstable metrics. scenario:profiler-idle-no-wall-profiler-18
scenario:profiler-light-load-no-wall-profiler-24
|
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2982ce329
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ts/src/profile-serializer.ts
Outdated
| // Handle file:// prefix | ||
| let scriptName = node.scriptName; | ||
| if (scriptName.startsWith('file://')) { | ||
| scriptName = scriptName.slice(7); | ||
| } | ||
|
|
||
| if (ignoreSamplesPath && scriptName.indexOf(ignoreSamplesPath) > -1) { | ||
| continue; | ||
| } | ||
|
|
||
| const stack = entry.stack; | ||
| const location = getLocation( | ||
| node, | ||
| locations, | ||
| locationIdMap, | ||
| functions, | ||
| functionIdMap, | ||
| stringTable, | ||
| sourceMapper | ||
| ); |
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.
Normalize scriptName before building locations
In serializeHeapProfileV2, the file:// prefix is stripped into a local scriptName only for the ignore-path check, but getLocation(...) still receives the unmodified node whose scriptName retains the file:// prefix. That means the serialized profile will embed file:// paths (and dedup keys) instead of the normalized paths used elsewhere, which is a behavioral change from the existing serializer and can break source-mapping and path-based filtering in downstream tooling that expects plain filesystem paths. Consider mutating node.scriptName or passing the normalized path into getLocation so V2 output matches V1 normalization.
Useful? React with 👍 / 👎.
|
Some thoughts:
|
What does this PR do?:
Introduces a new lazy heap profiling API
profileV2that reduces memory usage during heap profile serialization. Instead of materializing the entire V8 allocation profile as JS object upfront, the new API traverses the profile root tree on-demandMotivation:
When collecting heap profiles, the current
profile()API callsGetAllocationProfile()to get the profile data, then recursively translate the entire profile tree into JS objects, this creates a full JS objects tree in heap memory which can be significant for large apps.The new
profileV2()API introduces a lazy traversal pattern, getting JS object is done on-demand using the new classAllocationProfileNodeWrapperwith methods likegetChildrenCountandgetChild. On the TS side, we only request for current node JS object which allow us to gradually build the tree and reduce memory usage at this operation.Additional Notes:
The current implementation copies the V8 allocation profile to C++ heap to keep it alive after GetAllocationProfile() returns. I think that this is necessary because profile object has limited lifetime (
HandleScope). While this still a copy it's lightweight compared to creating full JS object since we only store raw profile data (string, number).Additionally, in this branch I attempted to avoid copying the profile data to C++ by only store
profile->GetRootNode(). This can be called on the TS side usingv8ProfileV3(), but it crashes when accessing the first child withprofile.getChild(0)becausenode->nameis no longer availableTo reproduce the error: