Declare node:process and node:module ESM exports lazily as well - #37726
Conversation
Both generators still called get() on every property of the object they mirror, so `import process from "node:process"` constructed process.stdout, stderr and stdin (and with them the tty and stream stack), config, release, allowedNodeEnvironmentFlags and the rest of the table, and importing node:module built the require cache object, builtinModules, the wrapper proxy and so on, all before the importer ran. They now share exportObjectProperties() with the "bun" module: a value that is already stored on the object is exported as is, everything else (static table entries nobody has read, accessors, inherited properties) is declared without a value and read off the object when something first binds to it. The export lists are unchanged: process still lists its whole prototype chain, module still lists exactly its static table.
|
Warning Review limit reached
Next review available in: 5 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 (7)
Comment |
|
Status: reproduced with the snippet in the description ( |
|
Startup numbers. All wall-clock Release 1.4.0 (eager, n=20):
So before this change the import cost about 18 ms over an empty file, which is the stdio streams ( Debug + ASAN build of this branch (n=10):
The two imports are now within noise of touching the global directly; constructing |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes the sampling semantics for node:process / node:module ESM exports (accessor-backed exports now read at first binding, and a throwing getter now throws instead of exporting undefined), a human look would still be worthwhile even though the pattern mirrors #37714.
What was reviewed:
NativeModuleList.hreordering: verified the regex-based scanner ininternal-module-registry-scanner.tssees the same file-order sequence, so generated ids are unchanged.exportObjectProperties:getDirectreturns empty for un-reified static entries and inherited names, and the GetterSetter/CustomGetterSetter guard prevents leaking accessor cells into the namespace.- Consumers of the changed generator signatures (
ModuleLoader.cppcreateWithLazyExports,InternalModuleRegistry.cpp's templatedgenerateNativeModule) both accept theJSObject*return.
Extended reasoning...
Overview
This PR extends the lazy-ESM-export pattern from #37714 (the "bun" module) to node:process and node:module. Both generators move from BUN_FOREACH_ESM_NATIVE_MODULE to BUN_FOREACH_LAZY_ESM_NATIVE_MODULE, and the three generators now share one helper, exportObjectProperties() in _NativeModule.h. The helper exports each name either as its already-stored value (getDirect finds a plain data property) or as a lazy binding that JSC materializes on first read. The old per-export get() loop and its TopExceptionScope cleanup are deleted. Three new subprocess tests are added alongside the existing "bun" cases in builtin-esm-lazy-exports.test.ts.
Security risks
None identified. The change does not touch validation, auth, or any user-input path; it only defers when existing property callbacks run.
Level of scrutiny
Medium-high. The mechanism is a straight application of the pattern already merged in #37714, and the diff is small and mostly deletion. But it sits on the module-loading path that every ESM import process from "node:process" and import { createRequire } from "node:module" goes through, and it carries a documented behavioral change: an accessor-backed export is read when first bound rather than when the module loads, and a getter that throws now throws from the binding instead of exporting undefined. The PR description argues (correctly, as far as I can tell) that for the first importer these are the same moment, and the throwing case matches what #37714 already established for "bun" — but a maintainer should confirm that contract is acceptable for node:process too, since Node compat expectations are stricter there than for the Bun object.
Other factors
I verified that the file-order-sensitive codegen in internal-module-registry-scanner.ts (a regex over macro("...", ...) lines) still sees NodeModule → NodeProcess → BunObject in the same positions after the macro-group move, so generated numeric ids are unchanged. The two remaining consumers of the generator signature — ModuleLoader.cpp's SyntheticSourceProvider::createWithLazyExports case and InternalModuleRegistry.cpp's templated generateNativeModule — both handle the JSObject*-returning form. The new helper's isGetterSetter() || isCustomGetterSetter() guard mirrors the existing cachedDataValue lambda in INIT_NATIVE_MODULE, so a reified CustomAccessor (e.g. Module._resolveFilename, Module.wrapper, Bun.main) is not leaked as a raw cell into the namespace. Test coverage is thorough (linking, listing, reading, inherited methods, accessor bindings, pre-load data-property snapshotting) and each case runs in its own subprocess. The one cosmetic nit is a missing trailing newline in _NativeModule.h.
|
On the trailing newline in |
Follow-up to #37714 (the
"bun"module) and #37525 (thesrc/jsbuiltins): the two remaining native ES modules that mirror an existing object,node:processandnode:module, still built their records eagerly.generateNativeModule_NodeProcesscalledget()on every enumerable property ofprocessand its prototype chain, andgenerateNativeModule_NodeModuleon every entry of the Module constructor's static table. Unlike"bun", these imports are not rewritten by the transpiler, so everyimport process from "node:process"(a very common line in published ESM packages) and everyimport { createRequire } from "node:module"paid for it.Repro
Before:
86 27. Forprocessthat is everything in the table that can be reified, includingstdout,stderrandstdin, which construct the stdio streams and loadnode:tty/node:streamto do it, plusconfig,release,allowedNodeEnvironmentFlags,versions, ...; forModuleit is_cache,builtinModules,globalPaths,SourceMap, thewrapperproxy and the rest of the table. After:2 3, i.e. only what is there before user code runs (Symbol.toStringTagand_exitingonprocess,lengthandnameonModule) pluscreateRequire, the one thing the file imported.On this machine (release build of 1.4.0, min of 20 runs),
import process from "node:process"added about 18 ms to the startup of an otherwise empty file, about the same as touchingprocess.stdoutdoes; with this change the import itself is free and that cost moves to whoever actually readsstdout. Measured numbers (release 1.4.0 for the eager cost, a debug build of this branch for the new one) are in a comment below.Fix
Both generators move to the
BUN_FOREACH_LAZY_ESM_NATIVE_MODULEgroup added in #37714 (the entries keep their position inNativeModuleList.h, so the generated ids do not change) and share one helper,exportObjectProperties()in_NativeModule.h, which the"bun"generator now uses too. For each name the caller wants exported it does the split #37525 does for thesrc/jsbuiltins: a value that is already stored on the object (getDirect()finds a plain value) is exported as is, anything else is declared without a value and JSC'smaterializeLazyExportreadsobject[name]the first time something binds to it. "Anything else" covers a static table entry nobody has read yet (the expensive case), an accessor (process.argv,process.title,Module._resolveFilename,Module.wrapper), and a property inherited from the prototype chain (the EventEmitter methods ofprocess). The generators themselves only decide the name list, which is what keeps the export lists identical to before:node:process:getPropertyNames()onprocess, as before, so the inherited EventEmitter methods (on,emit, ...) stay exports, and a data property assigned ontoprocessbefore the module is loaded is still exported and still snapshotted at load (process.test.jshas a test for exactly that, withdefaulton top, which keeps being skipped in favour of the object).node:module: the static table, as before, solength,nameand anything user code assigned ontoModulestay out."bun":getOwnNonIndexPropertyNames(), as in Declare the "bun" module's ESM exports lazily instead of reifying the whole Bun object #37714. The only difference from Declare the "bun" module's ESM exports lazily instead of reifying the whole Bun object #37714 is that aBun.*property something already read before the module loads is now snapshotted instead of declared lazily; it is the same object either way.The generators no longer run any getters, so the
TopExceptionScopehandling that turned a throwing getter into anundefinedexport (and the comments about not bulk-reifying because of the exception-check verifier) go away with them. A getter that throws now throws from whatever binds to that export, and a termination arriving while a binding is being materialized propagates from there, the same way #37714 describes for"bun". The sampling-time difference is also the same as there: an accessor export is read when it is first bound rather than when the module loads, which for the first importer is the same moment.Tests
Added to
test/js/bun/resolve/builtin-esm-lazy-exports.test.ts, same shape as the"bun"cases (each in its own process, readout isdescribe()of the object filtered to a watched sample of names;Object.keys/Reflect.ownKeysare used for the export-list checks becausefor...inreifies every static property of the object it enumerates):node:process: linkingimport proc, { on, release }constructs exactlyrelease;onis the inherited method; the export list equals the enumerable names ofprocessand its prototype chain plusdefault; listing it constructs nothing; readingstdoutoff the namespace constructs exactlystdoutand is the real stream;argv(an accessor) binds toprocess.argv.node:process: a data property assigned before the load is exported with its value at load time, and one assigned afterwards is not exported (the part of the behaviour this keeps from the eager version).node:module: linkingimport Module, { createRequire }constructs exactlycreateRequire(and it works); the export list equalsObject.keys(Module)plusdefault; readingbuiltinModulesconstructs exactly that and is the same array;_resolveFilenamebinds to the accessor's value.The two "linking constructs ..." cases fail on main (everything watched shows up as constructed after import); the snapshot case and the 12 existing cases in the file pass on both. Also run on this build:
test/js/node/process/(process.test.js,process-stdio,process-on,call-constructor, which importsnode:processas ESM),test/js/node/module/,test/js/node/events/event-emitter.test.ts,test/js/bun/util/BunObject.test.ts,test/js/bun/test/mock/,stubs.test.js,require-esm-transitive-tlaandimport-meta-resolve; green except for two tests that fail identically without this change in this environment (process.test.js"process" wants$USERset, andprocess-args.test.jsspawns 100 debug processes inside a 5 s timeout).BUN_JSC_validateExceptionChecks=1is clean for importing both modules and for reading every export of both namespaces.