codegen: lay out builtin module sources in dependency order - #36101
Conversation
The embedded builtin-module source blob is one contiguous string with
per-module offsets, and modules were concatenated in registry (sorted
path) order — so a require's module and its transitive dependencies sat
on pages scattered across the whole ~2.6 MB blob.
Concatenate them in DFS post-order over the static require graph
instead: each module lands contiguous with its dependencies, and loading
a builtin reads one contiguous run of pages rather than faulting across
the blob. The graph comes from the rewritten `internalModuleRegistry, N`
lookups already present in the bundled output; a fixed list of popular
entry modules seeds the traversal so the layout is deterministic. Only
the blob's byte order changes — offsets are still emitted per module
name, so ids and lookups are untouched.
Cold-start .rodata page-cache residency, readahead on:
require("node:fs") 6.23 -> 5.71 MB, require("node:http") 7.27 -> 6.78 MB.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesThe bundler now orders JavaScript internal modules using dependency edges found in transformed Module blob layout
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the reorder is provably a permutation (roots end with ...modules, emitted guards duplicates, mark-before-recurse handles cycles), and offsets are keyed by enum name so lookup is unaffected. The inline nit about node/buffer.ts is cosmetic; the .filter(r => deps.has(r)) already drops it.
Extended reasoning...
Overview
This PR adds a layoutOrder function to src/codegen/bundle-modules.ts that reorders the physical concatenation of builtin JS module sources in the embedded .rodata blob from alphabetical to DFS post-order over the static require graph. Only the byte order in InternalModuleRegistryConstants.bin changes; the offset/length constants in the generated .h are still emitted per enum name and consumed by name in createInternalModuleById.h, so module IDs and runtime lookup are untouched.
Correctness verified
- Permutation completeness:
roots = [...hotRoots.filter(...), ...modules]guarantees every input module is visited;emitted.add(id)before recursing prevents both duplicates and cycle recursion. The output is exactly a permutation of the input — even if the dependency regex missed every edge, the result would still be correct (just suboptimally ordered). - Regex matches actual form:
codegenRequireIdininternal-module-registry-scanner.tsemits__intrinsic__internalModuleRegistry, ${n}/*...*/, which becomes@internalModuleRegistry, Npost-processing; the/internalModuleRegistry, ?(\\d+)/gpattern captures it in both debug and--minify-syntaxbuilds. - Native-module edges: requires of native modules produce indices ≥
nativeStartIndex, somodules[Number(m[1])]isundefinedand theif (dep && dep !== id)guard drops them. - Key format:
id.slice(0, -3).replaceAll("/", path.sep)is byte-identical to the existing key computation in the blob-building loop directly below, sooutputs.gethits the same entries (verified against.js-suffixed modules likenode/readline.jstoo). - Debug path: all spans are
{offset: 0, length: 0}regardless, so reorderingmoduleSpansis a no-op there.
Security risks
None. Build-time codegen only; no user input, no runtime code path change.
Level of scrutiny
Low-medium. This is a build-script change whose worst-case failure mode is a compile error in the generated header (missing/duplicate *CodeOffset constant), which the build itself catches. The permutation property means it cannot silently drop or duplicate a module's source. The perf claim (~7-8% page residency reduction) is modest and the mechanism (contiguous readahead) is sound; it's not practically CI-testable, and there is no behavioral change to write a regression test for.
Other factors
One nit was flagged inline: "node/buffer.ts" in hotRoots is dead (buffer is a native module, confirmed against _NativeModule.h and src/js/node/). It's defensively filtered out on the next line, so it's cosmetic and doesn't block.
| "node/https.ts", | ||
| "node/net.ts", | ||
| "node/url.ts", | ||
| "node/buffer.ts", |
There was a problem hiding this comment.
🟡 "node/buffer.ts" is dead here — node:buffer is a native module (declared in src/jsc/modules/_NativeModule.h), not a JS builtin under src/js/node/, so it never appears in moduleList.slice(0, nativeStartIndex) and deps.has("node/buffer.ts") is always false. The .filter(r => deps.has(r)) on line 334 silently drops it so nothing breaks, but per the "every line you add must be demonstrably live" rule this entry should be removed.
Extended reasoning...
What
The hotRoots array added in this PR includes "node/buffer.ts" as one of the "popular entry modules" seeding the DFS layout traversal. But node:buffer is not a JS builtin module — it is a native module. There is no src/js/node/buffer.ts on disk, and node:buffer is registered in src/jsc/modules/_NativeModule.h (line 31: macro("node:buffer"_s, NodeBuffer)), which means it lives after nativeStartIndex in moduleList and is served by generateNativeModule_NodeBuffer, not from the source blob this function is laying out.
Code path
layoutOrder is called with moduleList.slice(0, nativeStartIndex) — the JS-bundled modules only. The deps map is populated by iterating exactly that slice:
for (const id of modules) {
...
deps.set(id, [...edges]);
}Since "node/buffer.ts" is never in modules, deps.has("node/buffer.ts") is always false, and the very next line filters it out:
const roots = [...hotRoots.filter(r => deps.has(r)), ...modules];So the entry is provably unreachable — it is silently dropped every build.
Why nothing else prevents it
The .filter(r => deps.has(r)) guard is deliberately permissive (it lets the hot-root list survive future module removals without breaking codegen), so a nonexistent entry produces no error, no warning, and no observable difference in the generated blob. That is why this is not a functional bug — but it does mean the guard masks the fact that this line can never do anything.
Impact
No runtime or build-time breakage. The impact is purely maintainability: the entry misleads readers into thinking node:buffer participates in the JS-module source layout (and that pulling it forward would cluster its dependents), when in fact it has no source in this blob at all. Per REVIEW.md — "Every line you add must be demonstrably live … parsed-but-never-read is a red flag" — dead entries in newly-added code should be removed rather than left for the filter to swallow.
Step-by-step proof
ls src/js/node/ | grep -i buffer→ no output;src/js/node/buffer.tsdoes not exist.src/jsc/modules/_NativeModule.h:31declaresmacro("node:buffer"_s, NodeBuffer)→node:bufferis native.createInternalModuleRegistryscanssrc/js/for JS modules and appends native modules after settingnativeStartIndex, somoduleList.slice(0, nativeStartIndex)never contains abufferentry.layoutOrderbuildsdepsonly from that slice →deps.has("node/buffer.ts")isfalse.hotRoots.filter(r => deps.has(r))drops"node/buffer.ts"unconditionally → the line is dead on every build.
Fix
Delete line 327 ("node/buffer.ts",) from hotRoots.
1 participant
What does this PR do?
The embedded builtin-module source blob (
bun_internal_modules_data) is one contiguous ~2.6 MB string with per-module offsets. Modules were concatenated in registry (sorted path) order, so arequire's module and its transitive dependencies sat on pages scattered across the whole blob, and a cold start faulted all over it.This concatenates them in DFS post-order over the static require graph instead, so each module lands contiguous with its dependencies and loading a builtin reads one contiguous run of pages. The graph is built from the
internalModuleRegistry, Nlookups already present in the bundled output; a fixed list of popular entry modules seeds the traversal so the layout is deterministic. It stays a single blob with per-module offsets — only the byte order changes, and offsets are still emitted per module name (<Enum>CodeOffset), so ids and lookups are untouched.Measured (Linux x64 release, cold page cache, kernel readahead at default 128 KB,
.rodataresidency viamincoreafter the process exits):require("node:fs")require("node:http")With readahead disabled (true touched set) both are unchanged (~1.6 / 2.4 MB), as expected — the same bytes are read, they're just adjacent now, so readahead pulls in useful neighbors instead of cold ones. Modest, but free: zero runtime cost and no format change.
How did you verify your code works?
InternalModuleRegistryConstants.h: the physical order is now dependencies-before-dependents (InternalPrimordialsfirst,InternalUrl→NodeUrl,InternalUtilInspectbeforeNodeUtil, the fs subtree clustered), and every module appears exactly once.bun -e/require()of several builtins on the rebuilt binary; behavior unchanged (the blob is only reordered).mincoreprobe from a clean cwd on a real filesystem (tmpfs paths can't be evicted and would fake the "before").