worker_threads: defer worker-side stdio/Console setup to first access - #34345
worker_threads: defer worker-side stdio/Console setup to first access#34345robobun wants to merge 5 commits into
Conversation
The worker preload rebinds process.stdout/stderr/stdin and console so captured stdio reaches the parent. Doing that eagerly paid two costs on every spawn regardless of whether the worker ever touched stdio: - Object.defineProperty on process.stdout/stderr/stdin reifies the process static hash table, which invokes every PropertyCallback including constructStdout/Stderr/Stdin. Those cold-load node:stream just to build fd-backed streams the rebind immediately discards. - setupWorkerStdio then built two port-backed Writable streams and a fresh node:console Console instance. Install self-replacing lazy accessors instead, and shadow the stdio PropertyCallback slots via plain [[Set]] first so the later defineProperty does not reify them. Readable/Writable and the HeapSnapshotStream class are loaded on first use.
|
Status: spawn latency restored to 1.3.14 parity (min 6.5-6.8ms vs 6.6-7.6ms for 1.3.14, down from 22.7-24.8ms on main). All worker stdio tests pass on a local release build. Addressed review: CI: diff is green. Build 73880 has 1 |
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 7:06 AM PT - Jul 16th, 2026
❌ @robobun, your commit 42403df has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34345That installs a local version of the PR into your bun-34345 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this reworks code that runs unconditionally in every node:worker_threads worker's bootstrap and depends on a subtle JSC static-table detail (that plain [[Set]] shadows the PropertyCallback slots so the subsequent defineProperty doesn't fire constructStdout/Stderr/Stdin) — worth a human look.
What was reviewed:
defineLazyself-replacement: get/set both rewrite to a writable data property with matching enumerability; identity-stable on repeat reads.proc.stdin = proc.stdout = proc.stderr = undefinedunconditionally clears all three, but the Worker constructor always creates stdout/stderr channels so theif (stdout)/if (stderr)guards are never false in practice.- Lazy console captures the native console object before installing the accessor, so
nativeConsole.Consolecannot re-enter the getter; readingprocess.stdout/stderrinside it triggers their own lazy getters, not recursion. Readable/Writable/node:pathdeferral and theHeapSnapshotStreamfactory are mechanical??=conversions.
Extended reasoning...
Overview
The PR defers per-worker stdio/Console setup from eager to first-access to recover a ~3× spawn-latency regression. Changes are confined to src/js/node/worker_threads.ts (module-level requires made lazy, setupWorkerStdio rewritten around a new defineLazy helper, HeapSnapshotStream wrapped in a factory) plus one new test in test/js/node/worker_threads/worker_threads.test.ts.
Security risks
None identified. No new inputs are parsed and no trust boundaries change; the lazy accessors expose the same objects the eager path did, just later.
Level of scrutiny
High. setupWorkerStdio runs during preload in every node:worker_threads worker, so a mistake here breaks all worker stdio. The correctness of the fast path hinges on a JSC-internals claim: that assigning proc.stdin = proc.stdout = proc.stderr = undefined creates own properties shadowing the static-table PropertyCallback entries (verified they are PropertyCallback, not CustomAccessor, at src/jsc/bindings/BunProcess.cpp:4495-4497) without triggering table reification, so the subsequent Object.defineProperty calls in defineLazy don't invoke constructStdout/Stderr/Stdin. The benchmark and passing vendored tests support this empirically, but it's the kind of engine-coupling a maintainer should sign off on.
Other factors
- The unconditional
proc.stdin = proc.stdout = proc.stderr = undefinedis a behavior change in the (currently unreachable) branch where a stdio port is absent — previously the native fd stream survived, now it would stayundefined. Since the Worker constructor always creates stdout/stderr channels this can't happen today, but it's worth a maintainer glance. applyWorkerProcessOverridesalso callsObject.defineProperty(process, ...)aftersetupWorkerStdio; by then the stdio slots are already own properties, so any reification it triggers won't re-fire the stdio callbacks.- The new test asserts the accessors are installed and self-replace, which guards the observable contract; it does not (and cannot easily) assert that reification was avoided, so the perf claim rests on the benchmark.
- CI is still building; no prior human or bot review comments to address.
Pair each shadow assignment with the corresponding lazy getter so a (currently unreachable) missing stdout/stderr port would leave the native fd stream in place instead of undefined.
|
Addressed the review note in 7d727b7: the shadow assignments for On the |
…to own file
require('node:console') is export default globalThis.console; evaluating it
after the lazy console accessor is installed would cache a plain Console
instance that lacks .Console/.write/[Symbol.asyncIterator]. Capture via
require first so the module registry caches the native object.
Move the lazy-accessor regression test into its own file so the gate can run
it cleanly under ASAN (worker_threads.test.ts has subprocess-spawning cases
that exceed the 5s default under ASAN).
There was a problem hiding this comment.
The node:console priming fix looks right — require('node:console') now runs before the lazy accessor is installed, so the module registry caches the native object and require('console').Console/.write/[Symbol.asyncIterator] keep working (covered by the new test). No further bugs found, but this leans on non-obvious JSC static-table semantics ([[Set]] shadowing vs. defineProperty reification, ordering vs. applyWorkerProcessOverrides' delete) and changes when the worker-side stdout/stderr port listeners attach, so it's worth a human pass.
Also checked: the parent-side auto-pipe path (this.stdout.on('data', …) in the constructor) still eagerly loads Readable via makePortReadable, and a worker that never touches stdio leaves its transferred ports listener-less (so they don't ref the loop) — matches the old net ref state. proc.stdin shadow + lazy accessor are both unconditional, matching the previous unconditional replace.
Extended reasoning...
Overview
Performance fix for a ~3x node:worker_threads spawn-latency regression. Touches src/js/node/worker_threads.ts only: converts the eager per-worker stdio/Console rebind in setupWorkerStdio into self-replacing lazy accessors, defers Readable/Writable/node:path/HeapSnapshotStream loads to first use, and shadows the native process.stdout/stderr/stdin PropertyCallback slots via plain [[Set]] before defineProperty so JSC doesn't reify the process static table and cold-load node:stream. Adds a two-case regression test.
Prior review
My earlier inline finding (require('node:console').Console becoming undefined in workers) was addressed in 1806acc by priming the module registry with require('node:console') before installing the lazy globalThis.console accessor — the second fix I'd suggested. The new test asserts .Console, .write, and [Symbol.asyncIterator] are still present on require('node:console') inside a worker. That thread is resolved.
Security risks
None. No auth/crypto/permissions surface; the change is timing of internal stream/Console construction inside a worker preload.
Level of scrutiny
High. This runs in every node:worker_threads worker's preload and its correctness rests on JSC implementation details: that proc.stdout = undefined creates an own data property without invoking constructStdout, that a subsequent defineProperty on that own slot doesn't reify the remaining table, and that applyWorkerProcessOverrides (which does reify via delete/defineProperty) runs after the stdio slots are already own. The author verified this empirically, but it's exactly the kind of engine-coupling a maintainer should sign off on. It also already produced one real regression during review.
Other factors
- Deferring
makePortWritablemeans the worker-sideport.on('message', onAck)+port.unref()no longer run at preload; I traced the ref accounting and a never-touched port has no listener and so doesn't hold the loop — net-equivalent to the old attach-then-unref, but worth a second pair of eyes. defineLazy's setter path preserves user assignment toprocess.stdoutbefore first read.- Parent-side
worker.stdout/.stderrgetters and the auto-pipe in the constructor still materializeReadableon the parent immediately when auto-pipe is on (the default), so no behavior change there. - Evidence block shows the accessor test only fails on the ASAN/debug build without the fix and passes on release without the fix — expected, since the test asserts the shape of the optimization rather than the latency, but it means the test is a shape guard, not a regression guard for the perf claim itself.
There was a problem hiding this comment.
The require('node:console') regression from my earlier review is fixed (1806acc primes the module registry before installing the accessor, and the new test covers .Console/.write/[Symbol.asyncIterator]). No further issues found, but this leans on JSC static-hash-table reification semantics and changes observable descriptor shapes on process/globalThis in every worker preload, so it's worth a human look.
Checked: defineLazy self-replacement preserves identity and writability; require('node:console') now runs before the accessor so the registry caches the native object; applyWorkerProcessOverrides's later defineProperty/delete reify only after stdio slots are already own properties; lazy Readable/Writable/HeapSnapshotStream sites all guard with ??=.
Extended reasoning...
Overview
Performance fix for a ~3x worker spawn-latency regression. setupWorkerStdio in src/js/node/worker_threads.ts now shadows the native process.std{in,out,err} PropertyCallback slots via plain [[Set]] (avoiding static-table reification), then installs self-replacing lazy accessors for the three stdio streams and globalThis.console. Readable/Writable, node:path, and the HeapSnapshotStream class are deferred to first use. A new test file asserts the accessor shape and that require('node:console') still exposes .Console/.write/[Symbol.asyncIterator] inside a worker.
Prior review resolution
My earlier inline finding — that require('node:console').Console became undefined in workers because the lazy getter was installed before the module registry was primed — has been addressed exactly as suggested: nativeConsole is now captured via require('node:console') before defineLazy, so the registry caches the native object (which carries .Console/.write) and later user requires return it. The second test in worker-stdio-lazy.test.ts locks this in. src/js/node/console.ts is still export default console;, so the priming call is a bare global read with no stream construction — the perf goal is preserved (benchmark confirms 1.3.14 parity).
Security risks
None. This is startup-ordering / lazy-initialization work in a built-in module; no auth, crypto, network, or untrusted-input parsing is touched.
Level of scrutiny
Medium-high. The change runs in every node:worker_threads worker's preload and depends on a JSC implementation detail: that plain [[Set]] on a PropertyCallback slot creates an own property without reifying the static table, whereas Object.defineProperty does reify. The author verified this empirically and reasoned through the interaction with applyWorkerProcessOverrides (its delete/defineProperty calls reify later, but stdio is already an own property by then so constructStdout/etc. skip). It also makes process.stdout/etc. observably start as accessor descriptors rather than data properties inside workers — a Node-compat surface change that a maintainer should sign off on.
Other factors
- One real bug was caught and fixed during review, which argues for human eyes on the remaining subtleties.
- Test coverage is good: the new file plus the listed
test-worker-*parallel tests exercise the auto-pipe path, flush-on-exit, and safe-getters. - The one CI failure so far (
fs.watchFile.test.tssigabrt on x64-asan) is unrelated to this diff. globalThis.console.Consoleremainsundefinedin workers after materialization, but that was already the case before this PR — not a regression here.
|
Cross-reference: #38987 takes the smaller route for the same startup cost (plain assignment for the stdio slots, and it also removes the |
|
Two data points from measuring the 1.4 pre-tag multi-threaded memory regression, for whenever this gets rebased (#38987 now covers the stdio assignment and removes the
One trap for the in-place variant, in case it helps: the property has to be redefined in place. Deleting |
1 participant
node:worker_threads spawn latency regressed ~3x after #31216 (captured stdio + process overrides). The report traced it to the unconditional per-worker stdio channels and the
node:worker_threadspreload; profiling shows the actual cost is two things that run in every worker's preload regardless of whether the worker ever touches stdio:Object.defineProperty(process, "stdout"/"stderr"/"stdin", ...)reifies theprocessstatic hash table, which invokes everyPropertyCallbackincludingconstructStdout/Stderr/Stdin. Those cold-loadnode:streamjust to build fd-backed streams the rebind immediately throws away.setupWorkerStdiothen eagerly built two port-backedWritablestreams and a freshnode:consoleConsoleinstance.The MessageChannel creation and port transfer themselves measure as effectively free.
Change
setupWorkerStdionow shadows the nativePropertyCallbackslots via plain[[Set]]first (which does not reify the table), then installs self-replacing lazy accessors forprocess.stdout/stderr/stdinandglobalThis.console. The first access builds the real stream/Console and rewrites the slot to a plain writable data property, so later reads are direct and identity-stable.Readable/Writableand theHeapSnapshotStreamclass are loaded on first use instead of at module top level;node:pathis loaded insidevalidateWorkerFilename.consolecaptures the native console object up front and reads.Consoleoff it, becauserequire("node:console")isexport default globalThis.consoleand would otherwise re-enter the getter.worker.stdout/stderrremain Readables fed by the worker'sprocess.stdout/stderr(including the default auto-pipe path); the channels are still always created.Benchmark
30 sequential
new Worker("require('worker_threads').parentPort.postMessage(1)", { eval: true })round-trips, interleaved over 5 rounds on the same machine (local release build):Verification
worker process.stdio and console are installed as lazy accessorsasserts the properties are accessors before first read and self-replace to identity-stable value properties afterwards; fails on main with"value"in every slot.test/js/node/worker_threads/(107 tests) andtest/js/web/workers/pass.test/js/node/test/parallel/test-worker-{stdio,stdio-flush,stdio-flush-inflight,no-stdin-stdout-interaction,safe-getters,console-listeners,message-port-drain,on-process-exit,unsupported-things}.jsall pass.[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 1 rejected · iteration 2
evidence per changed file