Repository navigation
fix(core): exclude the running node executable from callgrind - #102
not-matthias wants to merge 1 commit into
Conversation
The runner only passes `--obj-skip` for the `node` it resolves on PATH. When benchmarks run another binary, such as one provided by a Bazel toolchain, callgrind records the whole V8 runtime between the JS frames: thousands of native functions that drown the flamegraph and make it expensive to process. Register `process.execPath` on callgrind's obj-skip list in `setupCore`, through the instrument-hooks client request, as pytest-codspeed does for the Python executable. It runs before any benchmark starts the instrumentation, and is a no-op outside Valgrind.
 [Medium impact] The change reaches Callgrind profiling across Node benchmark integrations. The PR appears safe to merge, with a non-blocking request for regression coverage.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // The runner only obj-skips the `node` it finds on PATH. Skip the executable | ||
| // that actually runs, since a toolchain such as Bazel's brings its own: when | ||
| // left in, callgrind records the whole V8 runtime between the JS frames. | ||
| native_core.InstrumentHooks.callgrindAddObjSkip(process.execPath); |
There was a problem hiding this comment.
No automated test covers the alternate executable this fixes. Add a regression test that runs a copied Node binary under callgrind while --obj-skip names a different binary, then checks that the running binary contributes no native functions. The manual check would not catch a later change that removes this call or moves it after instrumentation starts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/core/src/index.ts
Line: 32
Comment:
**Alternate executable lacks test coverage**
No automated test covers the alternate executable this fixes. Add a regression test that runs a copied Node binary under callgrind while `--obj-skip` names a different binary, then checks that the running binary contributes no native functions. The manual check would not catch a later change that removes this call or moves it after instrumentation starts.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
This feels a bit overkill. I'll test it manually before merging
There was a problem hiding this comment.
Fair point—the regression test would need a copied Node binary and a callgrind setup, which is likely disproportionate for this narrow setup hook. Your manual check exercises the specific failure mode, so the automated-test suggestion isn’t necessary for this PR.
Merging this PR will degrade performance by 6.72%3 warnings, 1 note
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
1 participant
In simulation mode, register the running Node.js executable (
process.execPath) on callgrind's obj-skip list whensetupCoreruns.The runner only passes
--obj-skipfor thenodeit resolves on PATH. When benchmarks run a different binary, for example one provided by a Bazel toolchain, that skip never matches and callgrind instruments the whole V8 runtime. The profile then holds thousands of native V8 functions between the JS frames: the flamegraph is drowned in runtime internals and becomes very expensive to process.This uses the existing
instrument_hooks_callgrind_add_obj_skipclient request (instrument-hooks resolves the real path), the same mechanism pytest-codspeed uses to skipsys.executable. The call happens insetupCore, before any benchmark starts the instrumentation, so the skip applies to all code the benchmark runs. Outside Valgrind it is a no-op, so walltime and memory modes are unaffected.Checked locally under valgrind-codspeed with a copy of
nodeat a path that differs from the one passed to--obj-skip: without the call, callgrind recorded 2,655 functions of that executable (1.5 MB profile). With it, it recorded none (60 KB profile). I did not test vitest workers running under Bazel.