Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 23 days. After that, they cost $0.25 per reviewed file. Or wait 1 minute for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
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 (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesThe transpiler now selects JSON and JSONC parsers by statement mode. JSON materialization requires recorded locations. A regression test measures deeply nested array performance. ChangesJSON parsing and transpiler flow
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change makes nested JSON and JSONC loading more efficient while preserving generated output and runtime behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, fix, technical rationale, verification steps, performance impact, and known limitations. It does not use the exact template headings, but it provides the required information in equivalent sections. Comment |
|
Updated 7:41 PM PT - Aug 28th, 2026
❌ @robobun, your commit 39e650b has 2 failures in
🧪 To try this PR locally: bunx bun-pr 40856That installs a local version of the PR into your bun-40856 --bun |
|
Status: fix and test pushed. The self-review changed the shape of the fix, see below. Reproduced on the release build with Shape: the first revision added a Verified on the debug build: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a bool threaded from the sole call site into a struct-update of the existing JSONOptions, gated on the exact condition that triggers materialize.
What was reviewed:
- Grepped for callers of
parse_json_into_arena/parse_jsonc_into_arena— onlyparse_data_loader, both arms updated. - Checked whether TOML/YAML/JSON5/XML arms share the bug class — they don't produce
EObjectJSON/EArrayJSON, so thematerializegate at transpiler.rs:1921 never fires for them. - Test uses a nested/flat ratio (20× bound vs. ~2× fixed / ~90×+ unfixed), so no
isASAN/isDebugbranching needed; harness conventions (Buffer.alloc,-espawn, concurrent pipe drain, combined-object assert) all followed.
Extended reasoning...
Overview
The PR fixes a quadratic-time path in Bun.Transpiler's json/jsonc data loaders. parse_data_loader in src/bundler/transpiler.rs was calling parse_json[c]_into_arena without requesting per-value source locations on the tape, then calling json::materialize, which — absent recorded locations — recovers each array item's Loc by re-scanning to its closing bracket. Nested arrays therefore cost depth × subtree size. The fix adds a record_value_locs: bool parameter to the two _into_arena entry points (folded into JSONOptions { record_value_locs, ..JSON_OPTS }) and passes !keep_json_and_toml_as_one_statement from the caller — the identical predicate that gates the materialize call a few lines below. A new test in test/js/bun/transpiler/ measures nested-vs-flat timing ratios in a subprocess and asserts each stays under 20×.
Security risks
None. No untrusted-input parsing logic changed; the tape parser already supported record_value_locs (used by parse_classic). This only flips an existing option on for one more entry point. No allocation sizing, no bounds arithmetic, no FFI, no user-controllable knob added.
Level of scrutiny
Low-to-medium. The Rust change is ~12 net lines: two signature extensions plus struct-update copies of const option blocks, and one local at the sole call site. I verified via grep that parse_data_loader is the only caller of both changed functions, so there are no stale call sites (including cfg-gated ones). The record_value_locs derivation exactly matches the materialize gate, so the flag is on iff it will be consumed. I checked the sibling loader arms (TOML/YAML/JSON5/XML) for the same bug class per REVIEW.md's "fix the whole class" rule: the materialize call is guarded on EObjectJSON | EArrayJSON, which only the JSON tape parser emits, so those arms are structurally unaffected. The new doc comment on parse_json_into_arena is load-bearing (tells future callers when to set the flag and why), not change narration.
Other factors
The test is well-constructed against the repo's review rules: Buffer.alloc(n, fill) instead of .repeat(), bunExe/bunEnv with -e, await using spawn, Promise.all over stdout/stderr/exited, and a combined {stdout, stderr, exitCode} toMatchObject assertion. It asserts a machine-independent ratio (best-of-3 nested ÷ best-of-3 flat) with a 20× bound against ~2× fixed and ~90×+ unfixed — wide enough margin that ASAN/debug jitter won't flake it, and the PR notes the unfixed debug build doesn't finish inside the timeout, so USE_SYSTEM_BUN=1 fails for the right reason. The subprocess also asserts scan and transformSync output correctness before timing, so a regression that made the loader wrong-but-fast would still be caught. File placement matches the directory's existing per-topic transpiler-*.test.ts convention. Exit reason was dry_streak and there are no prior reviews or outstanding objections on the timeline.
|
One correction to the review's loader check: the XML arm of |
robobun
changed the title
79c7a42 to
b1ffb86
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…irectly The Bun.Transpiler json and jsonc loaders parsed into the immutable row AST without recorded value locations and then materialized it. Without the locations the materializer re-scanned the source for every array item to recover its location, so a nested array cost depth times subtree size. When the data loader will edit the classic tree, parse with the classic entry points (parse_utf8 / parse_ts_config) the way the bundler's JSON loader does. They record every value's location and materialize in one step. The runtime module loader keeps the row entry points: it prints the rows and never materializes. Only the XML parser still hands rows to materialize, and it always records locations, so the materializer's re-scan fallback is dead. Remove it and require the location columns.
b1ffb86 to
39e650b
Compare
This branch has not been deployed
1 participant
Problem
Bun.Transpilerscan,transformandtransformSyncwith thejsonorjsoncloader take time quadratic in the nesting depth of an array. Release build,"[".repeat(d) + "]".repeat(d): 20 ms at d=4000, 300 ms at d=16000, 1.2 s at d=32000.bun build --no-bundleshares the path.parse_data_loader(src/bundler/transpiler.rs:1868) parsed into rows without value locations, then calledjson::materialize.Materializer::array(src/parsers/json.rs:1001) then recovered each item's location by a scan to the item's closing bracket, so each nesting level rescanned the rest of the document.Fix
parse_data_loaderwill edit the classic tree (keep_json_and_toml_as_one_statementis false), it parses withparse_utf8/parse_ts_config, the entry points the bundler's JSON loader uses. They record every value's location and materialize in one step. The runtime module loader keeps the row entry points and prints the rows.materializerequires the location columns.materialize_matches_the_classic_entry_pointsunit test compares recorded and recovered locations node for node, andtransformSync/bun build --no-bundleoutputs are byte-identical before and after.test/js/bun/transpiler/transpiler-json-nested-array.test.ts(new, fails on the unfixed build). Alsotest/js/bun/transpiler/,jsonc.test.ts,bundler_loader.test.ts,xml.test.ts.Background
E::ObjectJSON/E::ArrayJSONnodes with spans into anE::JsonTape. A row stores a property's key location, not its value's.record_value_locsadds a tape column with every value'sLoc.json::materializeconverts the rows into the classicE::Object/E::Arraytree that the named-exports rewrite edits.parse_classic(behindparse_utf8/parse_ts_config) is parse with locations plus materialize.Notes
scanon"[".repeat(d) + "]".repeat(d): d=1000 1.6 ms, 2000 5.1 ms, 4000 20 ms, 8000 80 ms, 16000 310 ms, 32000 1.2 s. Four times per doubling. Nested objects were linear:property_value_loconly skips the key string and the colon. The bundler (parse_classic) was linear on both shapes.transformSyncto about 640, and the asynctransform(thread pool stack) to about 310. The fixture nests 300 levels around a 4 MB string. Unfixed: 0.9 s in release and 6 s in debug, against 4 ms and 37 ms for the same bytes in a flat array. Fixed: nested costs 1.1 to 1.9 times flat (the extra array nodes). The assertion is on that ratio, so it does not depend on machine speed. On the unfixed debug build the fixture does not finish inside the test timeout.bun build --no-bundle nested.json(debug build): 6.1 s before, 0.27 s after, byte-identical output.import data from "./nested.jsonc"is unchanged: it still parses rows without locations.JSON document is too deeply nested(fromparse_classic) instead ofDocument is too deeply nested, the same as the bundler. json: use one depth-limit message for every parser path #40853 unifies these messages and touches adjacent lines in both files.record_value_locsflag toparse_json_into_arena/parse_jsonc_into_arenaand kept the fallback. The self-review pointed out that the classic entry points already encode the contract and that the fallback would be dead, so this revision uses them instead.new Bun.Transpiler().transformSync('{"default": 1}', "json")panics in the printer (print_decls,unreachable) on main and on the release build.cargo clippy -p bun_parsers -p bun_bundler(clean) andcargo check -p bun_parsers --tests. Thebun_parsersunit tests do not link standalone in this container.