worker_threads: don't build the process object's lazy properties during worker startup - #38987
worker_threads: don't build the process object's lazy properties during worker startup#38987robobun wants to merge 6 commits into
Conversation
…er startup The worker bootstrap replaced process.stdout/stderr/stdin with Object.defineProperty, which first builds the fd-backed stream it is about to discard (loading tty, net, fs and their module graphs), and then deleted process._debugProcess & co., which makes JSC reify every remaining lazy property of the process object and turn it into a dictionary. Assign the stdio streams instead, and define the four main-thread-only stubs as own properties of non-worker process objects so there is nothing to delete in a worker. Cuts node:worker_threads Worker startup by roughly 30% (release: ~33ms to ~22ms; debug: ~2.2s to ~1.6s per worker).
|
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 (3)
Comment |
|
Status: fix complete, waiting for CI on e4ef2f7 (an empty re-run commit: the code is unchanged since 49c6057, and the fix itself since 6240baa). The 49c6057 run had no test failures: 177 jobs passed and the darwin 14 aarch64 lane expired waiting for an agent, hence the re-run. Reproduced by timing The three new tests fail on main ( Earlier CI runs were green apart from one unrelated red lane each (6240baa: alpine x64 missing docker in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it moves four entries out of processObjectTable and changes Process::finishCreation — which also drops the _debugProcess & co. stubs from Web Workers, not just node workers — and overlaps with #37128 / #37336, a human sign-off would still be worthwhile.
What was reviewed
setupWorkerStdio: assignment vsdefinePropertyproduces the same{writable, enumerable, configurable}own property — asserted by the new test.applyWorkerProcessOverrides: the remainingObject.definePropertycalls (debugPort,abort,chdir, …) don't reify the table — the newhasNonReifiedStatic(process)assertion runs after them.- Main-thread coverage:
process.test.jsstill exercises the four stubs (line 1275); worker absence is covered bytest-worker-unsupported-things.js. isWorkerVM()is set fromworker_ptr != nullptrinZigGlobalObject.cpp:518, so both node and Web workers are gated.
Extended reasoning...
Overview
Three files: src/js/node/worker_threads.ts (switch setupWorkerStdio from Object.defineProperty to plain assignment; drop the delete process._debugProcess loop from applyWorkerProcessOverrides), src/jsc/bindings/BunProcess.cpp (remove four Function entries from the @begin processObjectTable lut source and install them via putDirectNativeFunction in Process::finishCreation when !clientData(vm)->isWorkerVM()), and two new tests in worker_threads.test.ts (a hasNonReifiedStatic(process) invariant test mirroring the one for the Bun object, plus a POSIX fd-aliasing test that observes the stdio streams not being built).
Security risks
None. No untrusted input, no auth/crypto/permissions surface. The change removes work from a bootstrap path and relocates four no-op stub functions from a static table to a conditional putDirect.
Level of scrutiny
Moderate-to-high. The JS-side change is straightforward and the descriptor-shape equivalence is asserted by the test. The C++ side edits the generated static hash table for process and finishCreation, and intentionally changes behavior for all worker VMs (Web Worker included), not just node workers — the PR description flags this, but it's a Bun-native observable change made for node-compat convenience that a maintainer should sign off on. The PR also overlaps with three other in-flight PRs (#37128 rewrites setupWorkerStdio, #37336 gives _debugProcess a real implementation on top of this same finishCreation site), which is a coordination point a human should weigh.
Other factors
- Existing tests cover both directions:
process.test.jsasserts the four stubs are callable and returnundefinedon the main thread; the vendoredtest-worker-unsupported-things.jsasserts they are absent in a node worker. - The new
hasNonReifiedStatictest runs afterapplyWorkerProcessOverrides(which still usesObject.definePropertyfordebugPort/abort/chdir/…), so it also guards against those calls reifying the table. .lut.hfiles are not checked in; the table change is picked up by the build's codegen.putDirectNativeFunction(..., 0, Process_stubEmptyFunction, ..., 0)matches the previousFunction 0table entries' arity and uses attributes0; the PR states the attributes match andprocess.test.js'sprocess[stub]()check would still pass.
|
Re the review's two points: both are intentional. Dropping the four stubs from web Workers as well follows from gating on the VM (node itself only has them on the main thread, and nothing in-tree references them outside the main-thread process tests); gating on node workers only would need the Worker kind at process creation for no practical gain. The overlap with #37128 and #37336 is described under Related PRs in the description: #37128's rewrite of setupWorkerStdio makes the same assignment change, and #37336 would install its real _debugProcess from the same finishCreation branch. |
…tstrap stops reifying it
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…er comment's reify-all example
There was a problem hiding this comment.
I re-reviewed after 49c6057 — both earlier inline notes (the fd test's JSON.parse(stdout) and the stale callLazyProcessBuilder comment) are addressed, and this pass found nothing new. Given that this moves four entries out of processObjectTable (regenerating Process.lut.h) and drops them from web Worker VMs as well, plus the stated overlap with #37128 / #37336, a maintainer sign-off on the isWorkerVM() gating and the table change is still worth having.
What was reviewed:
[[Set]]vs[[DefineOwnProperty]]on thePropertyCallbackstdio entries — the new test asserts the resulting descriptor shape andhasNonReifiedStatic(process), and the fd-alias test proves the old streams aren't built.- The remaining
Object.defineProperty(proc, "debugPort", …)inapplyWorkerProcessOverridesis aCustomAccessor, so assignment there would hit the shared setter — correctly left as-is. Process_stubEmptyFunctionis still live via_linkedBinding/_tickCallback; no dead code introduced.
Extended reasoning...
Overview
Three files: src/js/node/worker_threads.ts switches the worker bootstrap's stdio replacement from Object.defineProperty to plain assignment and removes the delete process._debugProcess loop; src/jsc/bindings/BunProcess.cpp moves the four main-thread-only stub functions out of processObjectTable into a !isWorkerVM() branch of Process::finishCreation and updates the callLazyProcessBuilder comment; test/js/node/worker_threads/worker_threads.test.ts adds three tests (lazy-table invariant + descriptor shape, fd-alias check under piped stdio, process.mainModule in a file worker).
Security risks
None identified. No untrusted input parsing, no auth/crypto/permission surface. The change removes work from a bootstrap path and relocates four no-op stub functions; the fd-alias test reads /proc-equivalent fstat info in-process only.
Level of scrutiny
Medium-high. The process object's static hash table is load-bearing for every JS execution context, and editing the @begin processObjectTable block regenerates Process.lut.h. The mechanism (JSC's PropertyCallback reification under [[Set]] vs [[DefineOwnProperty]], and deleteProperty → reifyAllStaticProperties) is subtle, though the PR's tests pin it empirically. The isWorkerVM() gate also changes web Worker behavior (the four stubs disappear there too), which the author defended as intentional but is a design call.
Other factors
- Both inline findings from my earlier pass were fixed in 49c6057 (the fd test now compares
stdout.trim()as text; thecallLazyProcessBuildercomment names live reify-all triggers). The comment-cop bot's paragraph-length flags were addressed in f134ea6/83166409. - Three related open PRs (#37128, #34345, #37336) overlap this code; whichever lands first determines where
_debugProcess's eventual real implementation goes. A maintainer should be aware of the merge order. - Test coverage is good: each of the three new tests is documented as failing on main with the specific pre-fix output, and the invariant test mirrors the existing
BunObject.test.tspattern. - I confirmed
Process_stubEmptyFunctionremains referenced (_linkedBinding,_tickCallback) so removing the four table entries does not orphan it, and that the remainingObject.definePropertyondebugPorttargets aCustomAccessor(where assignment would call the shared setter), so it is correctly not converted.
There was a problem hiding this comment.
Both earlier inline nits (the JSON.parse(stdout) diagnostic and the stale callLazyProcessBuilder comment) are addressed in 49c6057, and this pass found nothing new. Deferring to a human because the change edits processObjectTable and Process::finishCreation in native code, drops the four stubs from web Workers as well (intentional per the author, but a judgment call), and overlaps with #37128 / #37336 for merge ordering.
Checked: assignment vs. defineProperty on a lazy PropertyCallback slot — [[Set]] stores directly without building, so setupWorkerStdio no longer constructs the fd-backed streams; isWorkerVM() gating covers both worker kinds and is set from worker_ptr != nullptr in ZigGlobalObject.cpp; the main-thread process._debugProcess etc. remain callable (process.test.js:1275) and the vendored test-worker-unsupported-things.js still asserts they are absent in a worker; the fd-alias test now compares stdout as text so a crashed child surfaces stderr.
Extended reasoning...
Overview
The PR removes two sources of eager process property construction in the node:worker_threads bootstrap: (1) setupWorkerStdio now assigns process.stdout/stderr/stdin instead of using Object.defineProperty (which triggered the lazy fd-backed stream builders it was replacing), and (2) the four main-thread-only stubs (_debugProcess, _debugEnd, _startProfilerIdleNotifier, _stopProfilerIdleNotifier) move out of processObjectTable into a !isWorkerVM() branch of Process::finishCreation, so worker VMs never have them and the bootstrap's delete loop (which forced reifyAllStaticProperties) is removed. Three new tests cover the invariants: hasNonReifiedStatic(process) stays true in a worker, no fd aliases of 1/2 appear when stdio is piped, and process.mainModule in a file worker is the entry module (a user-visible regression fix).
Security risks
None identified. The change is startup-time property installation on process; no user input, no parsing, no privilege boundary. The stubs being moved are no-op functions.
Level of scrutiny
Medium-high. BunProcess.cpp's static hash table and finishCreation govern the shape of the global process object across every VM, and the isWorkerVM() gate applies to web Workers too, not just node workers — the author acknowledges this is intentional and matches Node (which only defines these on the main thread), but it is a small observable difference for web-Worker code that previously saw the stubs. The generated Process.lut.h changes when the table does. The overlap with #37128 (rewrites setupWorkerStdio with the same assignment change) and #37336 (real _debugProcess implementation, which would now be installed from finishCreation) means a maintainer should decide merge order.
Other factors
My two prior inline comments are resolved: the fd-alias test now asserts { stdout: stdout.trim(), stderr, exitCode } against { stdout: "[]", stderr: "", exitCode: 0 } so a crashed child surfaces its stderr in the diff, and the callLazyProcessBuilder comment names live reify-all triggers (delete process.x, Object.entries(process)) instead of the removed preload. I verified the remaining callers of the four names — process.test.js calls them on the main thread (still functions via putDirectNativeFunction), test-worker-unsupported-things.js asserts absence in a worker (now true natively), and the inspector-helper.js/test-debug-process.js uses run on the main thread. The Object.defineProperty(proc, "debugPort", ...) that remains in applyWorkerProcessOverrides targets a CustomAccessor table entry, not a PropertyCallback, so it does not build anything expensive (and the new test's lazy: true would fail if it reified the table). Given the native process table edit and the cross-PR coordination, this warrants a human look rather than auto-approval.
|
Updated 12:05 PM PT - Aug 15th, 2026
❌ @robobun, your commit e4ef2f7 has some failures in 🧪 To try this PR locally: bunx bun-pr 38987That installs a local version of the PR into your bun-38987 --bun |
|
Independently hit the same two lines while chasing the 1.4 pre-tag memory regression on multi-threaded workloads (peak RSS of Vite / rolldown style builds 1.4x to 1.9x higher than 1.3.14 while single-threaded runs are flat), so here are the memory numbers for this change, since the description only quantifies time. Same fix on my side (assignment in Where the per-worker regression comes from (bare
Measured on the main thread for reference: reading Steady-state RSS per idle worker (40 workers minus 8 workers, all alive and parked in
So this recovers roughly 1.1 MB of the 1.3 MB per worker that every worker picked up since #31216 (the RSS deltas are larger than the JS heap deltas presumably because of what module loading allocates outside the JS heap: bytecode, source strings, and so on), and the lazy console accounts for the rest and then some. For the record, the remaining gap on workers that allocate heavily is not in the bootstrap: on a 32-worker run that builds 200k objects per worker, 1.3.14 sits at ~12.1 MB/worker and main with both changes at ~14.4 MB/worker. About 1.2 MB of that is garbage (the array's previous backing stores) that happens to survive until the end of the loop on main but not on 1.3.14, which is purely where the collections land, and the rest is freed memory waiting out mimalloc's 100 ms purge delay ( |
1 participant
Problem
node:worker_threadsWorker spends a large part of its startup buildingprocessproperties that are thrown away immediately. Two lines of the worker-side bootstrap insrc/js/node/worker_threads.tswere responsible:setupWorkerStdioreplacedprocess.stdout/stderr/stdinwithObject.defineProperty.[[DefineOwnProperty]]reads the current property first, and these are lazy native properties, so the fd-backed streams were built just to be replaced: this loadsnode:tty,node:net,node:fsand the stream internals into the worker, dups fd 1 and fd 2 when stdio is piped (the dups stay open until the orphaned streams are collected), and on a tty leaves twoSIGWINCHlisteners on the worker'sprocess.applyWorkerProcessOverridesrandelete process._debugProcess(plus three more). Deleting a static-table property makes JSC reify the whole table (JSObject::deleteProperty->reifyAllStaticProperties): every remaining lazyprocessproperty (env,versions,config,release,report,allowedNodeEnvironmentFlags, ...) is built, andprocessis switched to dictionary mode for the rest of the worker's life.process.mainModuleis built from the require map, and during the bootstrap the entry module is not in it yet, so it has beenundefinedin every worker since node:worker_threads: +48 Node.js tests passing — MessagePort, stdio, SHARE_ENV, exit codes, transfer semantics, postMessageToThread + inspector #31216 (node, and Bun 1.3: the worker's entry module).worker_threads.js). This is where most of the time of the worker-spawning tests on the debug/ASAN lanes goes.Fix
setupWorkerStdioassigns the streams.JSObject::puton an unreified lazy entry stores the new value directly without building the old one; the own property it produces is{ writable, enumerable, configurable }, the same shapedefinePropertyproduced (asserted by the test)._debugProcess,_debugEnd,_startProfilerIdleNotifier,_stopProfilerIdleNotifierleaveprocessObjectTable;Process::finishCreationdefines them as own properties unlessclientData(vm)->isWorkerVM(), with the same function, name, length and attributes the table gave them. A worker'sprocessnever has them, so the bootstrap has nothing to delete. (WebWorkers lose the four no-op stubs too; node only has them on the main thread.)processhas no_debugProcess& co., its stdio properties are the port-backed streams,process.mainModuleis the entry module), the only work removed is the construction of objects nothing could reach afterwards, andprocessstays a lazy, non-dictionary object in workers like it is on the main thread.test/js/node/worker_threads/worker_threads.test.ts(133 tests) went from 439 s to 306 s on the same machine.test/js/node/worker_threads/worker_threads.test.ts, each failing on main as noted:hasNonReifiedStatic(process)(the invarianttest/js/bun/util/BunObject.test.tskeeps forBun) is true, the stdio descriptors have the previous shape, the four internals are absent, and they are still functions on the main thread (main:lazy: false);[9, 10]). This is the part the first test cannot see, since building a single property does not reify the table;process.mainModulein a file worker is the entry module (main:undefined).worker_threads.test.tsandprocess/process.test.json the debug and release builds,test/js/web/workers(release: all pass; debug: the same load-dependent timeouts as an unmodified build), the vendoredtest-worker-*.jsnode tests (110 files) on the debug build,test-worker-unsupported-things.js(the four internals are absent in a worker), and a worker writing to stdout/stderr under a pty.Related PRs
setupWorkerStdioand already switches it to assignment for the same reason; whichever of the two lands second gets a trivial conflict in that function. Thedeleteremoval and the table change here are independent of it.delete, soprocessstill ends up fully reified there; it additionally defers stream/Console construction behind accessors, which this PR does not do._debugProcessa real implementation; on top of this change that function would be installed from the same place infinishCreation(node has no_debugProcessin workers either).Background
processis a JSC object whose properties come from a generated static hash table (processObjectTableinBunProcess.cpp). APropertyCallbackentry is built the first time it is read ("reified") and then stored as an ordinary own property, so an untouched property costs nothing. A few operations cannot be answered from the table, and JSC handles them by reifying every entry and converting the object to dictionary mode:deleteof a table entry is one of them (Object.entriesand spread are others).hasNonReifiedStaticfrombun:internal-for-testingreports whether an object's table is still unreified.definePropertyon such an entry:[[Set]]replaces the entry with the new value directly;[[DefineOwnProperty]]first asks for the current descriptor, which builds the entry.JSVMClientData::isWorkerVM()is set when a global is created for a Worker thread of either kind; it is false for the main thread's VM.