Skip to content

fix(core): exclude the running node executable from callgrind #102

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.

Sign up for GitHub

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

Open
not-matthias wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(core): exclude the running node executable from callgrind #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/core/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ export const setupCore = () => {
}

native_core.InstrumentHooks.setIntegration("codspeed-node", __VERSION__);
// 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);

@greptile-apps greptile-apps Bot Oct 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

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!

Fix in Claude Code Fix in Codex

@not-matthias not-matthias Oct 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a bit overkill. I'll test it manually before merging

@greptile-apps greptile-apps Bot Oct 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// In walltime, we use node's native profiling options rather than our own, cf `getV8Flags`
if (getCodspeedRunnerMode() !== "walltime") {
linuxPerf.start();
Expand Down
4 changes: 4 additions & 0 deletions packages/core/src/native_core/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ try {
addMarker: (_pid: number, _markerType: number, _timestamp: bigint) => {
return 0;
},
// eslint-disable-next-line @typescript-eslint/no-unused-vars
callgrindAddObjSkip: (_path: string) => {
return 0;
},
__codspeed_root_frame__: <T>(callback: () => T): T => {
return callback();
},
Expand Down
9 changes: 9 additions & 0 deletions packages/core/src/native_core/instruments/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,15 @@ export interface InstrumentHooks {
*/
addMarker(pid: number, markerType: number, timestamp: bigint): number;

/**
* Add an object file to callgrind's obj-skip list, as `--obj-skip` does.
* Only affects code callgrind has not instrumented yet. No-op outside
* Valgrind.
* @param path Path of the object file, resolved to its real path
* @returns 0 on success, non-zero on error
*/
callgrindAddObjSkip(path: string): number;

/**
* Execute a callback function with __codspeed_root_frame__ in its stack trace
* @param callback Function to execute
Expand Down
23 changes: 23 additions & 0 deletions packages/core/src/native_core/instruments/hooks_wrapper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,27 @@ Napi::BigInt CurrentTimestamp(const Napi::CallbackInfo &info) {
return Napi::BigInt::New(env, ts);
}

Napi::Number CallgrindAddObjSkip(const Napi::CallbackInfo &info) {
Napi::Env env = info.Env();

if (info.Length() != 1) {
Napi::TypeError::New(env, "Expected 1 argument: path")
.ThrowAsJavaScriptException();
return Napi::Number::New(env, 1);
}

if (!info[0].IsString()) {
Napi::TypeError::New(env, "Expected string (path)")
.ThrowAsJavaScriptException();
return Napi::Number::New(env, 1);
}

std::string path = info[0].As<Napi::String>().Utf8Value();

uint8_t result = instrument_hooks_callgrind_add_obj_skip(path.c_str());
return Napi::Number::New(env, result);
}

Napi::Number AddMarker(const Napi::CallbackInfo &info) {
Napi::Env env = info.Env();

Expand Down Expand Up @@ -220,6 +241,8 @@ Napi::Object Initialize(Napi::Env env, Napi::Object exports) {
Napi::Function::New(env, CurrentTimestamp));
instrumentHooksObj.Set(Napi::String::New(env, "addMarker"),
Napi::Function::New(env, AddMarker));
instrumentHooksObj.Set(Napi::String::New(env, "callgrindAddObjSkip"),
Napi::Function::New(env, CallgrindAddObjSkip));
instrumentHooksObj.Set(Napi::String::New(env, "__codspeed_root_frame__"),
Napi::Function::New(env, __codspeed_root_frame__));

Expand Down
1 change: 1 addition & 0 deletions packages/core/src/native_core/instruments/hooks_wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Napi::Number SetEnvironment(const Napi::CallbackInfo &info);
Napi::Number WriteEnvironment(const Napi::CallbackInfo &info);
Napi::BigInt CurrentTimestamp(const Napi::CallbackInfo &info);
Napi::Number AddMarker(const Napi::CallbackInfo &info);
Napi::Number CallgrindAddObjSkip(const Napi::CallbackInfo &info);
Napi::Object Initialize(Napi::Env env, Napi::Object exports);

} // namespace hooks_wrapper
Expand Down
Loading