Repository navigation
Conversation
generate_markdown re-parsed the metafile JSON and, for every import edge, linearly scanned the full inputs object to find the matching input key (once via JsonObject::contains, again via the suffix-match fallback). The Full Module Graph section did the same: sorted input paths, then re-fetched each value with a linear JsonObject::get. Both are quadratic in the input count. Build a StringHashMap exact-match set and a basename->keys index once before the reverse-dependency pass, and carry the value reference through the sorted-inputs iteration. Both match criteria in the fallback require basename equality, so the basename bucket is the complete candidate set and iteration order within it preserves first-match semantics. Also reuse a single sort-index scratch Vec across match_imports_with_exports_for_file calls instead of allocating a fresh one per reachable file, matching the existing cycle_detector pattern.
|
Warning Review limit reached
Next review available in: 6 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 |
|
Updated 12:05 AM PT - Jul 18th, 2026
❌ @robobun, your commit eb1f4df has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34531That installs a local version of the PR into your bun-34531 --bun |
|
Status: ready for review. Self-review complete; two findings addressed in eb1f4df (flattened the many-inputs fixture for Windows, gated the basename-collision fixture with CI on eb1f4df (build 74888):
The new tests are correctness checks for the refactored lookup path; output is byte-identical to the previous implementation, so the fail-before/pass-after gate will not distinguish them. The performance claim is backed by the measurements in the PR body. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ase on Windows The byte-level suffix match in generate_markdown compares separators literally, so a Windows backslash import path never matches a forward-slash input key that has a directory component. That limitation is pre-existing; keep the basename-collision test posix-only and use a flat layout for the many-inputs test so both platforms exercise the indexed lookup. Also drop the empty-stderr assertions to match the sibling tests in this block.
There was a problem hiding this comment.
LGTM — both earlier review findings are addressed and the refactored lookup is behavior-preserving.
What was reviewed:
- Verified the basename-bucket restriction is equivalence-preserving: both original suffix-match criteria already required
basename(target) == basename(input_key)(criterion 2 explicitly; criterion 1 by implication since neither side contains a mixed separator on any given platform), and bucket vecs are populated ininputs_obj.iter()order so first-match-wins is unchanged. - The Full Module Graph refactor filters non-Object entries before sorting instead of
continue-ing after; those produced no output either way. import_order_scratchtake/restore inmatch_imports_with_exports_for_file— no?or early return between take and restore; mirrors the neighboringcycle_detectorpattern.- Confirmed
bun_paths::basename→bun_core::strings::basenameis platform-gated (posix-only/on non-Windows), which matches the separator conventions of both.prettyinput keys and.textimport targets.
Extended reasoning...
Overview
This PR eliminates two O(N²) scans in generate_markdown (the --metafile-md bundle-analysis report) and hoists a per-file Vec<usize> allocation in LinkerContext::match_imports_with_exports_for_file to a reusable scratch buffer on the struct. Files touched: src/bundler/linker_context/MetafileBuilder.rs (index build + carry-value-through-sort), src/bundler/LinkerContext.rs (scratch field + take/restore), test/bundler/metafile.test.ts (two new correctness tests for the refactored lookup).
Security risks
None. generate_markdown post-processes bundler-generated JSON; no untrusted input, no FFI, no filesystem writes. The LinkerContext change is a pure allocation-lifetime hoist with no change to what is sorted or how.
Level of scrutiny
Medium. --metafile-md is a diagnostic report — a wrong attribution there is a UX bug, not a miscompile. The match_imports_with_exports_for_file change is in the bundler's linking hot path, but it's mechanically identical to the adjacent cycle_detector reuse pattern: take → clear → extend → use → restore, with no fallible operations or early returns between take and restore (I read the full loop body — every arm is either .expect("unreachable") or a log call). Even under a hypothetical panic the field would just hold an empty Vec and the next call re-allocates.
Correctness of the basename-bucket equivalence
I traced this carefully since it's the load-bearing claim. bun_paths::basename resolves to basename_posix (splits only on /) on non-Windows and basename_windows on Windows. On any given platform, both record.path.text (import target) and source.path.pretty (input key) use that platform's convention consistently within the string, so target.ends_with(input_key) ⇒ basename(target) == basename(input_key). Criterion 2 already checked basename equality explicitly. Bucket vecs preserve insertion order, and only one bucket is consulted per target, so first-match semantics are unchanged. Hoisting target_base/target_has_dots/target_without_dots outside the candidates loop is safe since they depend only on target. The now-dead PathOnly struct and JsonObject::contains are deleted in the same commit.
Other factors
Two prior review rounds surfaced (1) expect(stderr).toBe("") in the new tests and (2) Windows separator mismatch in the fixtures; both were fixed in eb1f4df (stderr assertions dropped; first test flattened to bare basenames, second gated with test.skipIf(isWindows) and a comment naming the pre-existing limitation). Both threads are resolved. The author reports byte-identical 127 KB output on a 301-input graph and 41 pre-existing metafile tests + bundler_edgecase/bundler_cjs2esm passing.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-18, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
…#42596) ### Problem - With `--splitting`, `--metafile-md` reports an `import()` of a bundled file as external. It prints `[EXTERNAL: entry.js imports ./lazy-6ngx7bj8.js]`, counts the edge under "External imports", and lists the imported module as "(entry point or orphan)". A split `require()` (`--target=bun`) has the same problem. - `generate_markdown` (`src/bundler/linker_context/MetafileBuilder.rs:801`) reads only `path` and `external` of each `inputs[file].imports[]` edge. For a split import these hold the output chunk and `true`. #42423 added `entryPoint` (the bundled input) to the edge but did not change `--metafile-md`. ### Fix - The new helper `import_target` reads one edge. An edge that has `entryPoint` is an import of that input and is not external. All other edges keep `path` and `external`. - All three readers of an edge in `generate_markdown` call the helper. The metafile JSON does not change. - Correct because `entryPoint` is a key of `inputs`, and the bundler writes it only for a record that the linker pointed at a chunk. The chunk stays visible under "Loads these chunks". - Verified: `test/bundler/metafile.test.ts` (the new test fails on main, 50/50 pass with the change). Also `test/bundler/esbuild/metafile.test.ts`. Self-reviewed: 8 concerns raised, all about code outside this diff (see Notes). ### Background - The metafile is the esbuild-format build report. `inputs` maps each source file to its import edges. - `--metafile-md` renders that JSON as markdown. `generate_markdown` sees only the JSON, not the linker state. - With `--splitting`, the target of an `import()` becomes an entry point with its own chunk. The linker points the import record at that chunk. So the JSON edge has the chunk in `path` and `"external": true`. <details><summary>Notes</summary> - Follow-up to #42423, whose body says `--metafile-md` does not use the field yet. #38465 (closed in favor of #42423) had the same markdown test idea. - The three readers are the reverse dependency map with the external count, the "Imports" list of each module, and the raw `[IMPORT:` / `[EXTERNAL:` lines. - Test fixture output before the change: `| External imports | 3 |`, `[EXTERNAL: entry.js imports ./lazy-6ngx7bj8.js]`, `[EXTERNAL: entry.js imports external-pkg]`, `[EXTERNAL: entry.js imports ./required-0dd5m4wr.js]`, no `[IMPORTED_BY:` line. - Test fixture output after the change: `| External imports | 1 |`, `[IMPORT: entry.js -> lazy.js]`, `[EXTERNAL: entry.js imports external-pkg]`, `[IMPORT: entry.js -> required.js]`, `[IMPORTED_BY: lazy.js <- entry.js]`, `[IMPORTED_BY: required.js <- entry.js]`. - The "Imports" list of `entry.js` now reads `lazy.js (dynamic-import, contributes 15 bytes, specifier: ./lazy.js)`. Before, it read `./lazy-6ngx7bj8.js (dynamic-import, **external**, specifier: ./lazy.js)`. - The chunk also stays visible in `[ENTRY: lazy.js -> ./lazy-6ngx7bj8.js (54 bytes)]`. Those sections read `outputs`, which this diff does not touch. - `cargo clippy -p bun_bundler --no-deps` and `cargo fmt --check` are clean. `prettier --check` is clean on the test file. Not changed here. These are in the JSON writer (`generate`) or in code next to this diff: - The JSON edge of a split `import()` of a JSON, TOML or text file has no `with`. The writer derives `with` from `record.source_index`, which the linker clears for a split import. So the report has no `with type:` line for that edge. - The writer marks an unused import as `"external": true`, for example a type-only `import { Foo } from "./types"` in a TypeScript file. The report still counts that edge under "External imports". - #34531 (open) changes the key matching inside the reverse dependency loop of the same function. The two changes touch adjacent lines. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/metafile.test.ts <!-- robobun:evidence:end -->
…oven-sh#42596) ### Problem - With `--splitting`, `--metafile-md` reports an `import()` of a bundled file as external. It prints `[EXTERNAL: entry.js imports ./lazy-6ngx7bj8.js]`, counts the edge under "External imports", and lists the imported module as "(entry point or orphan)". A split `require()` (`--target=bun`) has the same problem. - `generate_markdown` (`src/bundler/linker_context/MetafileBuilder.rs:801`) reads only `path` and `external` of each `inputs[file].imports[]` edge. For a split import these hold the output chunk and `true`. oven-sh#42423 added `entryPoint` (the bundled input) to the edge but did not change `--metafile-md`. ### Fix - The new helper `import_target` reads one edge. An edge that has `entryPoint` is an import of that input and is not external. All other edges keep `path` and `external`. - All three readers of an edge in `generate_markdown` call the helper. The metafile JSON does not change. - Correct because `entryPoint` is a key of `inputs`, and the bundler writes it only for a record that the linker pointed at a chunk. The chunk stays visible under "Loads these chunks". - Verified: `test/bundler/metafile.test.ts` (the new test fails on main, 50/50 pass with the change). Also `test/bundler/esbuild/metafile.test.ts`. Self-reviewed: 8 concerns raised, all about code outside this diff (see Notes). ### Background - The metafile is the esbuild-format build report. `inputs` maps each source file to its import edges. - `--metafile-md` renders that JSON as markdown. `generate_markdown` sees only the JSON, not the linker state. - With `--splitting`, the target of an `import()` becomes an entry point with its own chunk. The linker points the import record at that chunk. So the JSON edge has the chunk in `path` and `"external": true`. <details><summary>Notes</summary> - Follow-up to oven-sh#42423, whose body says `--metafile-md` does not use the field yet. oven-sh#38465 (closed in favor of oven-sh#42423) had the same markdown test idea. - The three readers are the reverse dependency map with the external count, the "Imports" list of each module, and the raw `[IMPORT:` / `[EXTERNAL:` lines. - Test fixture output before the change: `| External imports | 3 |`, `[EXTERNAL: entry.js imports ./lazy-6ngx7bj8.js]`, `[EXTERNAL: entry.js imports external-pkg]`, `[EXTERNAL: entry.js imports ./required-0dd5m4wr.js]`, no `[IMPORTED_BY:` line. - Test fixture output after the change: `| External imports | 1 |`, `[IMPORT: entry.js -> lazy.js]`, `[EXTERNAL: entry.js imports external-pkg]`, `[IMPORT: entry.js -> required.js]`, `[IMPORTED_BY: lazy.js <- entry.js]`, `[IMPORTED_BY: required.js <- entry.js]`. - The "Imports" list of `entry.js` now reads `lazy.js (dynamic-import, contributes 15 bytes, specifier: ./lazy.js)`. Before, it read `./lazy-6ngx7bj8.js (dynamic-import, **external**, specifier: ./lazy.js)`. - The chunk also stays visible in `[ENTRY: lazy.js -> ./lazy-6ngx7bj8.js (54 bytes)]`. Those sections read `outputs`, which this diff does not touch. - `cargo clippy -p bun_bundler --no-deps` and `cargo fmt --check` are clean. `prettier --check` is clean on the test file. Not changed here. These are in the JSON writer (`generate`) or in code next to this diff: - The JSON edge of a split `import()` of a JSON, TOML or text file has no `with`. The writer derives `with` from `record.source_index`, which the linker clears for a split import. So the report has no `with type:` line for that edge. - The writer marks an unused import as `"external": true`, for example a type-only `import { Foo } from "./types"` in a TypeScript file. The report still counts that edge under "External imports". - oven-sh#34531 (open) changes the key matching inside the reverse dependency loop of the same function. The two changes touch adjacent lines. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/metafile.test.ts <!-- robobun:evidence:end -->
What
generate_markdown(the--metafile-mdreport) had two quadratic passes over the metafile inputs:JsonObject::contains(target)(a linear scan of theinputsobject) and, on miss, iterated every input key again to try a suffix match. Import paths are written asrecord.path.text(absolute) while input keys aresource.path.pretty(relative), so the exact match essentially always misses and the fallback runs for every edge.Vec, sorted them, then re-fetched each value withinputs_obj.get(path), another linear scan per input.Both are O(N²) in the input count. This PR builds a
StringHashMapexact-match set and abasename -> [input keys]index once before the reverse-dependency pass, and carries the value reference through the sorted-inputs iteration instead of re-looking it up. The two suffix-match criteria in the fallback already requiredbasename(target) == basename(input_key), so restricting the scan to the basename bucket preserves exactly which key is picked (first match in insertion order). Output is byte-identical.Separately,
match_imports_with_exports_for_fileallocated a freshVec<usize>sort-index per reachable file; this now reuses a scratch buffer onLinkerContextthe same way the adjacentcycle_detectordoes.Why
--metafile-mdis the "help me understand my bundle" report. Large projects are exactly where you want it, and exactly where it was slow.Flat graph (one entry re-exporting N leaf modules),
bun build --metafile-mdvsbun build --metafileonly:Release overhead goes 336 ms -> 950 ms -> 1864 ms when doubling N; the remaining debug overhead is JSON parsing of the metafile plus the other linear passes.
The 127 KB markdown for a 301-input graph is byte-identical between the released bun and this branch across repeated runs.
Testing
The two new tests in
test/bundler/metafile.test.tsexercise the rewritten lookup: a 41-input graph asserting every leaf is attributed to the entry, and a pair of inputs sharing a basename across directories asserting each is attributed to its own importer. These are correctness tests for the refactored path; the markdown output is unchanged, so they pass against the previous implementation as well. The speedup is demonstrated by the measurements above rather than by a timing assertion.All 41 pre-existing
metafile.test.tscases andbundler_edgecase/bundler_cjs2esmpass.Known limitation
The basename bucket degenerates when many inputs share a basename, most obviously
node_modules/*/index.js: with P packages the reverse-dependency pass is O(P^2) over theindex.jsbucket. That is still strictly better than the previous O(N^2) over every input, and for the projects I measured theindex.jsbucket is in the hundreds, not thousands. The clean fix is upstream of this function: emitsources[record.source_index].path.prettyfor resolved imports in the metafile JSON (matching esbuild) so the exact-match hash lookup fires for every internal edge and the suffix fallback becomes unreachable. That is a user-visible change to theinputs[*].imports[].pathformat (and also resolves the pre-existing Windows separator mismatch and run-to-run nondeterminism in that field), so it is tracked separately rather than folded into this output-preserving change.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/metafile.test.ts