Conversation
nodejs-github-bot
added
c++
IlyasShabi
force-pushed
the
ishabi/v8_heap_profile
branch
from
44720ea to
0221ebd
Compare
IlyasShabi
force-pushed
the
ishabi/v8_heap_profile
branch
from
0221ebd to
5861bc1
Compare
|
cc @nodejs/diagnostics |
doc/api/v8.md
Outdated
| console.log(profile); | ||
| ``` | ||
|
|
||
| ## `v8.heapProfilerConstants` |
There was a problem hiding this comment.
We very rarely expose bitfields and instead generally just let users pass us e.g. arrays of flags or options that we then translate in the JS wrapper. I could see exceptions made for highly performance-sensitive APIs, but I don't think this is an example of that
There was a problem hiding this comment.
I was looking for a proper way to do this while preserving the same naming as V8.
Fixed
Qard
left a comment
Qard
left a comment
There was a problem hiding this comment.
Some DX and future-looking recommendations, but otherwise LGTM.
| added: REPLACEME | ||
| --> | ||
|
|
||
| * Returns: {string} |
There was a problem hiding this comment.
Might be better to return a Buffer instead. It'd give us more flexibility to add binary formats in the future. I'd like to add support at some point in the future for the pprof-derived format being defined in OpenTelemetry at the moment, which is a binary format.
There was a problem hiding this comment.
IIRC worker.stopHeapProfile() already returns a JSON since v22, so changing the return type to Buffer would be a breaking change. For now this PR keeps the main thread consistent with the worker.
We could add a format option { format: 'json' | 'pprof' } in a follow up PR when binary format support is needed. wdyt?
There was a problem hiding this comment.
Ah, right, going for parity with the worker API. We could probably do something similar to fs.readFile(path, encoding, cb) where the encoding value switches the result between a string and a buffer depending on the format. I think format: 'json' | 'pprof' seems a suitable way to deal with that. 🙂
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62273 +/- ##
==========================================
+ Coverage 89.67% 89.68% +0.01%
==========================================
Files 676 677 +1
Lines 206555 206737 +182
Branches 39554 39571 +17
==========================================
+ Hits 185230 185419 +189
+ Misses 13461 13444 -17
- Partials 7864 7874 +10
🚀 New features to boost your workflow:
|
github-actions
bot
removed
the
request-ci
6 participants
This PR succeeds #60231 by @theanarkh and adds parameter support for heap sampling on both the main thread and workers.