Repository navigation
fix(core): restart node with the required V8 flags - #113
not-matthias wants to merge 5 commits into
Conversation
Merging this PR will regress 3 benchmarks4 warnings, 1 note
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
66b266e to
f19b2b3
Compare
Importing the native module used to create the instrument-hooks instance, which opens the runner FIFO and writes environment data. Create it on the first hook call instead, so loading the module has no instrumentation side effects.
Launchers that start node by absolute path, such as Bazel's rules_js, never run the runner's node wrapper on PATH. tinybench and benchmark.js then measured without the flags getV8Flags() requires: --no-opt, --predictable, the seed flags, and the GC flags. ensureV8Flags() now replaces the process before any benchmark or instrument-hooks state exists, inserting only the missing flags before the current execArgv so user flags keep precedence, as with the runner's wrapper. The restart reuses the running executable: /proc/self/exe on Linux because rules_js replaces process.execPath with its shell shim, which would re-add --require. A native execv binding keeps the PID, environment and open descriptors, so cancellation and the runner FIFO are unaffected; it clears FD_CLOEXEC on stdio like process.execve, otherwise the restarted process writes to /dev/null. The PID marker stops a restart loop when the launcher strips the flags again. Worker threads, IPC children, unsupported platforms, unbound addons and a failed exec keep the previous warning behavior instead of aborting the benchmark. Closes COD-4038
The existing jobs reach node through PATH, so the runner wrapper always injects the V8 flags and the plugin's restart never runs. Launch node by absolute path, as Bazel's rules_js does, in simulation, walltime and memory mode, and fail when the plugin reports missing flags or a failed restart. Refs COD-4038
f19b2b3 to
3a489b4
Compare
[Medium impact] The restart changes startup behavior in the two benchmark plugin workflows. Fix stdin entry points before merging; they can exit successfully without running any benchmarks.
|
A script piped into node (`node -`, or node without a script argument) is consumed before the plugin loads. The restarted process then read an empty stdin and exited successfully without running any benchmark. Keep the process and its missing-flags warning in that case. `-e`/`-p` code is part of execArgv and still restarts.
|
@GuillaumeLagrange lmk if you want to discuss the approach. i didn't find any real alternatives other than letting users manually pass the v8 flags which isn't really ideal imo. but considering the implementation adds complexity, we might want to consider it. |
Restart node with the required V8 flags when the launcher bypassed the runner's wrapper
The runner passes the flags from
getV8Flags()through anodescript onPATH. Launchers that start node by absolute path never consult it: Bazel'srules_js runs its toolchain binary through its own shim, so tinybench and
benchmark.js measured without
--no-opt,--predictable, the seed flags andthe GC flags.
NODE_OPTIONScannot carry them (node rejects several).ensureV8Flags()now replaces the process at plugin load, before any benchmarkor instrument-hooks state exists. It appends only the flags missing from
process.execArgvand reuses the current argv, so launcher arguments such asrules_js's
--require .../register.cjsand--preserve-symlinks-mainsurvive.On Linux it execs
/proc/self/exerather thanprocess.execPath, whichrules_js points at its shell shim; on macOS it uses
process.execPath.The restart runs in a small
execvbinding in the existing native addon, whichkeeps the PID, environment and open descriptors. That matters because the
runner, perf and eBPF attach by PID, and cancellation signals the benchmark
process directly. The binding clears
FD_CLOEXECon stdio first, like node'sown
process.execve, otherwise the restarted process writes to/dev/null.A PID marker prevents a restart loop if the launcher strips the flags again;
worker threads, IPC children, unsupported platforms and an unbound addon keep
the previous warning behavior.
Instrument-hooks now initializes on first use instead of at module import, so
the discarded image cannot leave a half-initialized FIFO connection behind.
Manually verified against a rules_js 2.1.3 project (Bazel 7.4.1, Node 18.20.4)
with
bazel runandbazel test: the restarted process keeps its PID, reportsthe complete analysis flag set in
execArgv, and stdout/stderr still reach thetest log. Outside Bazel,
--test_env=CODSPEED_ENV --test_env=CODSPEED_RUNNER_MODEis still required, since Bazel strips
CODSPEED_*from tests; without it theintegration runs disabled.
Closes COD-4038
The runner mirror of
getV8Flags()needs the same flag set: CodSpeedHQ/codspeed#598 adds--always-sparkplug, so the restart does not fire on the normalcodspeed execpath.