Rewrite the JSON parser around a SIMD structural index - #33032
Conversation
Replace the byte-at-a-time JSON lexer + recursive descent (src/parsers/json_lexer.rs) with a two-stage design: 1. Stage 1 (src/jsc/bindings/highway_json.cpp, runtime-dispatched via Highway like the other highway_* kernels): one pass over the document classifies every byte with two nibble LUTs, resolves escaped quotes with the odd-backslash-run trick, computes the in-string mask with a prefix-XOR, and emits the byte offset of every structural character, both quotes of every string, and the first byte of every other scalar into a u32 index array (branchless CompressStore). It also returns a per-64-byte-block "string needs a scan" bitmap and document-global flags. Comments and single-quoted strings cannot be folded into the quote-parity math, so the first `/` or `'` outside a string bails out to a scalar indexer (src/parsers/json_index.rs) that understands them and emits the identical structure; that path also serves wasm. 2. Stage 2 (src/parsers/json_stage2.rs): recursive descent over the index array. String bounds are consecutive indices, so strings in clean blocks are zero-copy slices of the source with no per-byte work; numbers and keywords are bounded by the next index. Duplicate keys are detected with a per-object hash window instead of a pooled hash map per object. Everything the old lexer accepted still parses (comments, trailing commas, single quotes, hex/octal/underscore numbers, BOM and exotic unicode whitespace, `\x`/`\v` escapes, the `.env` auto-quote path), the error messages are unchanged, and all entry points keep their signatures. `cargo test -p bun_parsers` covers JSONC, escapes, numbers, errors, duplicate-key warnings, `is_single_line`, the version checker, and 64-byte block boundary cases. bench/json-corpus + scripts/bench-json-rust.sh fetch a corpus of real package.json files and npm registry manifests and run a criterion benchmark of the parser (and its stages) against them.
…ilds
- An object or array that hits end-of-file before its closing bracket is
a hard parse error again (the recovery path made `{"a": 1` parse).
- Compile highway_json.cpp with -O2 in debug profiles (per-file override
+ PCH opt-out). At -O0 every highway op is an outlined call passing
64-byte vectors through the stack and clang's unoptimized codegen for
that raises #GP under the debug sanitizer set; it also made every
debug-build JSON parse pathologically slow.
- Duplicate-key detection keeps a hash stack per object instead of a
hash-map insert per key, and falls back to one document-wide map only
for objects past 32 keys.
- Container children are built on reusable scratch stacks and moved into
an exactly-sized AstAlloc vec when the container closes.
- test/js/bun/jsonc/jsonc.test.ts: differential tests against JSON.parse
(generated documents, block boundaries, escapes, BOM, control
characters), and the malformed-flood test now asserts the rejection
rather than the exception shape (the parser reports the first error
instead of cascading through 250 KB of recovery).
…sts skip duplicate-key warnings The index buffer is up to 4 bytes per input byte (a hundred MB for the largest npm manifests); zero-filling it before every parse cost more than the parse for big documents. The kernel reports exactly how much it initialized, so write into the Vec's spare capacity and adopt that length, and shrink oversized scratch buffers when they go back to the per-thread pool. npm registry manifests now parse with json_warn_duplicate_keys=false (new `parse_utf8_registry` entry): the warnings are never surfaced for registry responses and computing them costs a measurable fraction of every manifest parse. Adds bench/json-corpus/jsonc-parse.mjs: an end-to-end benchmark of `Bun.JSONC.parse` (parser + AST-to-JS conversion) over the corpus, with JSON.parse as the reference.
A backslash outside of a string now escapes the following byte for quote-pairing purposes in the scalar indexer too (the SIMD kernel's odd-backslash-run parity is global, simdjson-style). Such input is never valid JSON — both paths hand stage 2 a junk token either way — but the two indexers agreeing bit-for-bit is what makes them testable against each other: `json_index::tests::simd_and_scalar_indexers_agree` now checks 20k randomized documents plus block-boundary shapes.
A worst-case whole-document index buffer is 4 bytes per input byte; for the largest registry manifests that is a >100MB allocation that falls in the allocator's huge class, so every parse paid its page faults again (the 25MB next manifest indexed slower than it parsed). The kernel now exposes a resumable entry point that carries its escape/in-string/ scalar-run state across calls: documents over 1MB are indexed in 1MB chunks through a small pooled per-chunk buffer into an exactly-sized index vector, so the transient never exceeds ~4MB regardless of document size. The standalone differential harness drives the chunked entry point over every fuzz case and corpus file and requires bit-identical output to the one-shot call; `chunked_and_scalar_indexers_agree_on_large_documents` covers the Rust driver.
The spill map for objects past the 32-key linear window was shared across the whole document, so a registry manifest with thousands of large objects grew it to millions of entries — a quarter of the parse went to growing and probing it (and made the 25MB next manifest parse slower than the old parser). Clearing it when a new object first spills keeps it the size of one object, like the old per-object pooled map, without giving up the no-hashing fast path for small objects.
The single reused spill map was wrong as soon as large objects nested: the inner object cleared and took over the map, so duplicates in the outer object after that point went undetected and an outer key equal to an inner key could be misreported. Spilled objects now take a map from a per-nesting-level pool (cleared on close), which is also what keeps each map small. Covered by duplicate_key_detection_with_nested_large_objects.
|
Updated 5:35 AM PT - Jul 1st, 2026
⏳ @Jarred-Sumner, your commit 32440a9 is still building in |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces the lexer-based JSON parser with a staged structural-index pipeline, adds compact JSON-only AST nodes and consumers, and wires in benchmark/build support for the new parsing path. Public JSON parsing entry points and package-json callers are updated. ChangesTwo-stage JSON parser pipeline
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/json-corpus/fetch.sh`:
- Around line 21-33: The benchmark fixture generation in abbrev() and full() is
using live registry package names, which makes the saved manifest-abbrev-*.json
and packument-full-*.json files mutable over time. Update fetch.sh to pull
immutable snapshots instead of current packuments by package name, and adjust
the calls for the affected packages (including the later entries referenced in
the comment) so the corpus stays reproducible across runs.
In `@bench/json-corpus/jsonc-parse.mjs`:
- Around line 47-53: The MiB/s calculation in the benchmark output is using the
wrong time unit and underreports throughput by 1,000,000x. Update the throughput
formula in the jsonc-parse benchmark to convert `jsonc.median` from nanoseconds
to seconds before dividing, and keep the MiB conversion consistent when
computing `mbs` in the console output.
In `@scripts/bench-json-rust.sh`:
- Around line 30-34: The bench cache in bench-json-rust.sh is keyed only on file
existence, so simdutf.cpp/.h and the compiled .o can go stale when
SIMDUTF_VERSION changes or simdutf_shim.cpp is edited. Update the caching logic
in build() and the simdutf download block to also consider version/input mtimes,
or always rebuild the small native objects each run. Use the existing
SIMDUTF_VERSION, build(), and $SUP/simdutf.cpp/.h checks to locate the change.
- Around line 65-72: The bench script is hardcoding Linux-only linker flags in
the RUSTFLAGS setup, which breaks non-Linux hosts. Update
scripts/bench-json-rust.sh to gate the system/C++ link args by host platform,
using the same target-aware logic as the main build helpers to choose the
correct C++ runtime and system libraries. Keep the native archive link arg, but
only append -lstdc++ -lm -ldl -lpthread -lc on Linux-like targets and use the
appropriate equivalents on Darwin/other platforms.
In `@src/highway/lib.rs`:
- Around line 635-649: The Highway JSON index wrappers are missing a guard for
the `u32` offset contract before entering the FFI call. Update
`highway_json_index` and the chunked path in `highway_json_index_chunked` to
validate that `input.len()` plus any `base_offset`/running offset stays within
`u32::MAX` before calling `highway_json_index*`. If the computed offset or
emitted index range can exceed `u32`, reject the input early with an error
instead of passing it across the boundary.
In `@src/parsers/json_index.rs`:
- Around line 190-191: The `build` function in `json_index` currently enforces
the `u32` size limit only with `debug_assert!`, so oversized input can still
reach indexing and allocation in release builds. Replace that check with a real
runtime validation at the start of `build(contents: &[u8])` that returns an
`IndexError` before any further work, and update `report_index_error()` in
`json.rs` to handle the new index-limit failure case.
In `@src/parsers/json_stage2.rs`:
- Around line 773-807: In json_stage2.rs, the duplicate-key tracking in the
`dup` block inside the parser’s key handling misses the threshold-crossing key
when `n_prior == Self::DUP_LINEAR_MAX`; after seeding
`self.dup_maps[self.spill_depth]` with prior hashes, also insert the current
hash `h` into that spill map before continuing so later repeats of that key are
detected. Keep the existing `self.dup_hashes`/`scratch_props` confirmation path
unchanged and make sure the `spill_depth` bookkeeping still matches the map that
owns the current object.
In `@test/js/bun/jsonc/jsonc.test.ts`:
- Around line 325-340: The new Bun.JSONC.parse test in jsonc.test.ts only
exercises the small-document path and does not cover the >1 MB chunked-indexing
behavior. Increase the fixture built in the test around Bun.JSONC.parse so the
serialized payload clearly exceeds the chunking threshold, then add an assertion
on the generated JSON size or another explicit precondition before parsing. Keep
the existing round-trip checks against the big object so the test validates the
new chunked scanner path as well as the result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f90429cf-9dfd-4ef8-bad8-64d1305edfa6
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
bench/json-corpus/.gitignorebench/json-corpus/fetch.shbench/json-corpus/jsonc-parse.mjsscripts/bench-json-rust.shscripts/build/bun.tsscripts/build/flags.tsscripts/build/unified.tssrc/highway/lib.rssrc/install/PackageInstall.rssrc/install/npm.rssrc/jsc/bindings/highway_json.cppsrc/parsers/Cargo.tomlsrc/parsers/benches/json_parse.rssrc/parsers/benches/support/simdutf_shim.cppsrc/parsers/json.rssrc/parsers/json_index.rssrc/parsers/json_lexer.rssrc/parsers/json_stage2.rssrc/parsers/lib.rssrc/parsers/native_test_shims.rstest/js/bun/jsonc/jsonc.test.ts
💤 Files with no reviewable changes (1)
- src/parsers/json_lexer.rs
…imple)
JSON documents parsed for data (registry manifests, package.json,
Bun.JSONC.parse, tsconfig) paid for an AST designed for JavaScript:
every key and every string value was its own EString node appended to
the AST store inside a 112-byte G::Property carrying an initializer,
a kind, decorators and three Option discriminants JSON can never use.
That materialization — not lexing, and not the structural index — is
where almost all of the JSON parser's time goes on large documents.
E::ObjectSimple / E::ArraySimple are ordinary Expr nodes (so every
entry point keeps returning Expr and nothing else in the AST changes),
but their children are not: a property is a 32-byte
`PropertySimple { key: Str, key_loc, value: JsonValue }` row and a
value is an inline 16-byte tagged `JsonValue` (number/bool/null
inline, strings as a `Str` borrowing the source unless they contain
escapes, nested containers as StoreRefs). The only store nodes left in
such a document are the containers themselves.
The JSON parser produces them behind `JSONOptions::simple_objects`
(requires force_utf8), via the same container loops (const-generic over
the row type) so every error message, recovery path, duplicate-key
warning and `is_single_line` bit is shared with the full form —
`json::tests::simple_objects_match_the_full_ast` proves the two forms
canonicalize identically. `Expr::to_js` and the JS printer handle the
new variants; `Bun.JSONC.parse` is the first entry point switched over
(`json::parse_jsonc`), covered end-to-end by the JSON.parse
differential suite.
Parser benchmark (criterion median, registry options, vs the parser
before this branch):
package.json (react, 1 KB) 25.7 µs -> 4.9 µs (5.3x)
express manifest (339 KB) 2.72 ms -> 1.13 ms (2.4x)
drizzle-orm manifest (1.9 MB) 15.2 ms -> 6.9 ms (2.2x)
typescript manifest (8.6 MB) 58.6 ms -> 28.0 ms (2.1x)
next manifest (25 MB) 175 ms -> 79 ms (2.2x)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/parsers/benches/json_parse.rs (1)
31-33: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe default fixture path is rooted incorrectly for this crate.
When
BUN_JSON_BENCH_FIXTURESis unset,fixtures_dir()is based onCARGO_MANIFEST_DIR, but this benchmark crate lives undersrc/parsers/while the new corpus files are added at repo-rootbench/json-corpus/. A normal checkout will therefore panic here instead of finding the shipped fixtures.Suggested fix
- manifest_dir.join("bench/json-corpus") + manifest_dir.join("../../bench/json-corpus")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parsers/benches/json_parse.rs` around lines 31 - 33, The fallback fixture path in fixtures_dir() is rooted too shallow for the json_parse benchmark, so when BUN_JSON_BENCH_FIXTURES is unset it points under src/parsers/ instead of the repo-root bench/json-corpus/. Update the default path construction in fixtures_dir() to resolve from the workspace/repo root rather than CARGO_MANIFEST_DIR alone, and keep the existing read_dir() panic path intact. Make sure the benchmark in json_parse.rs finds the shipped corpus files in a normal checkout without requiring the env var.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_printer/lib.rs`:
- Around line 4723-4734: `print_json_value()` can recurse into `ObjectSimple`
and `ArraySimple` without the stack guard used by `print_expr()`, so add the
existing `self.stack_overflowed` check to this path before descending further.
Update the `print_json_value` handling in `src/js_printer/lib.rs` so nested JSON
printing goes through the same overflow-protected flow as other expression
printing, reusing the existing stack-check logic around `print_object_simple`
and `print_array_simple`.
In `@src/parsers/json.rs`:
- Around line 499-507: Add a test covering the empty-string case for
Bun.JSONC.parse so it verifies the same empty-input behavior as
parse_jsonc/parse_ts_config, namely that "" is mapped to an empty object. Update
test/js/bun/jsonc/jsonc.test.ts to include this case using the existing
Bun.JSONC.parse assertions so the coverage matches the implementation in
parse_jsonc.
---
Outside diff comments:
In `@src/parsers/benches/json_parse.rs`:
- Around line 31-33: The fallback fixture path in fixtures_dir() is rooted too
shallow for the json_parse benchmark, so when BUN_JSON_BENCH_FIXTURES is unset
it points under src/parsers/ instead of the repo-root bench/json-corpus/. Update
the default path construction in fixtures_dir() to resolve from the
workspace/repo root rather than CARGO_MANIFEST_DIR alone, and keep the existing
read_dir() panic path intact. Make sure the benchmark in json_parse.rs finds the
shipped corpus files in a normal checkout without requiring the env var.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80b0fa2f-077f-4176-b19e-5dc0b62f6a88
📒 Files selected for processing (11)
src/ast/e.rssrc/ast/expr.rssrc/js_parser_jsc/expr_jsc.rssrc/js_printer/lib.rssrc/parsers/benches/json_parse.rssrc/parsers/json.rssrc/parsers/json_index.rssrc/parsers/json_stage2.rssrc/react_compiler/lowering/build_hir/expr.rssrc/react_compiler/lowering/find_context_identifiers.rssrc/runtime/api/JSONCObject.rs
A simple object's rows now live in the parse arena (one allocation + copy per container) instead of a `Vec<_, AstAlloc>`, like every decoded string already does: no allocator round trip per container on the manifest path, and the container node shrinks. drizzle-orm 1.9MB manifest: 6.9ms -> 5.1ms; next 25MB: 79ms -> 59ms (the old parser: 15.2ms / 175ms).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/parsers/json.rs (1)
1271-1278: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the escaped identifier path in this test.
The test is named for escaped keyword identifiers, but it only parses plain
true,false, andnull.Proposed fix
- let p = run(br#"[true, false, null]"#, Which::Utf8); + let p = run(br#"[\u0074rue, \u0066alse, \u006eull]"#, Which::Utf8);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parsers/json.rs` around lines 1271 - 1278, The test in escaped_keyword_identifiers only covers plain keyword literals, so it never exercises the escaped-identifier parsing path. Update the input passed to run and the expected output in escaped_keyword_identifiers so it uses escaped keyword identifiers that force the lexer/parser through the escaped identifier branch, while still asserting the same successful parsing and JSON serialization behavior.Source: Coding guidelines
src/parsers/json_stage2.rs (1)
1330-1336: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle signed numbers before calling
parse_number_text.
parse_number_textexpects unsigned numeric text, but this tail path passestailunchanged forb'-'. Inputs where a negative number begins after exotic whitespace can fail incorrectly or underflow in debug builds.Proposed fix
- b'0'..=b'9' | b'.' | b'-' => { - let (value, used) = self.parse_number_text(tail, pos)?; - if !self.rest_is_ws_cold(&tail[used..]) { - return Err(self.number_trailing_junk(pos + used)); - } - self.cursor += 1; - Ok(Expr::init(E::Number::new(value), loc_tail)) - } + b'0'..=b'9' | b'.' => { + let (value, used) = self.parse_number_text(tail, pos)?; + if !self.rest_is_ws_cold(&tail[used..]) { + return Err(self.number_trailing_junk(pos + used)); + } + self.cursor += 1; + Ok(Expr::init(E::Number::new(value), loc_tail)) + } + b'-' => { + let number_start = 1 + tail[1..] + .iter() + .position(|b| !matches!(b, b' ' | b'\t' | b'\n' | b'\r')) + .unwrap_or(tail.len() - 1); + if number_start >= tail.len() + || !matches!(tail[number_start], b'0'..=b'9' | b'.') + { + self.expected(self.cursor, "number"); + return Err(self.unexpected(self.cursor)); + } + let (value, used) = self.parse_number_text(&tail[number_start..], pos + number_start)?; + if !self.rest_is_ws_cold(&tail[number_start + used..]) { + return Err(self.number_trailing_junk(pos + number_start + used)); + } + self.cursor += 1; + Ok(Expr::init(E::Number::new(-value), loc_tail)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parsers/json_stage2.rs` around lines 1330 - 1336, The numeric tail branch is passing a leading minus sign straight into parse_number_text, which only accepts unsigned text; update the handling in json_stage2::Parser so signed numbers are normalized before parsing, using the same path that recognizes b'-' and consumes the sign before delegating to parse_number_text. Make sure the cursor/consumed-length accounting still matches the full token and that negative numbers after exotic whitespace are parsed without underflow or rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/parsers/benches/json_parse.rs`:
- Around line 68-72: The benchmarks for parse_nowarn, parse_simple, and
parse_utf8 are no longer measuring the production parse path because Bump::new()
was hoisted outside b.iter(). Restore the per-iteration arena lifecycle by
creating a fresh Bump inside each benchmark iteration, or alternatively
rename/split these benchmarks into explicit reuse-bump microbenches so the
numbers remain comparable to real parsing. Apply the same fix pattern in the
benchmark blocks around the parse_nowarn, parse_simple, and parse_utf8 setup
code, keeping the existing js_ast::StoreResetGuard and js_ast::Log::init usage
intact.
---
Outside diff comments:
In `@src/parsers/json_stage2.rs`:
- Around line 1330-1336: The numeric tail branch is passing a leading minus sign
straight into parse_number_text, which only accepts unsigned text; update the
handling in json_stage2::Parser so signed numbers are normalized before parsing,
using the same path that recognizes b'-' and consumes the sign before delegating
to parse_number_text. Make sure the cursor/consumed-length accounting still
matches the full token and that negative numbers after exotic whitespace are
parsed without underflow or rejection.
In `@src/parsers/json.rs`:
- Around line 1271-1278: The test in escaped_keyword_identifiers only covers
plain keyword literals, so it never exercises the escaped-identifier parsing
path. Update the input passed to run and the expected output in
escaped_keyword_identifiers so it uses escaped keyword identifiers that force
the lexer/parser through the escaped identifier branch, while still asserting
the same successful parsing and JSON serialization behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0a21194-0cb8-4754-a490-4b495262cf34
📒 Files selected for processing (6)
src/ast/e.rssrc/js_parser_jsc/expr_jsc.rssrc/js_printer/lib.rssrc/parsers/benches/json_parse.rssrc/parsers/json.rssrc/parsers/json_stage2.rs
Each document parsed in simple mode now has a single property-row tape and a single item tape, grown by doubling inside the parse arena (the abandoned halves are arena memory; allocations per document are logarithmic). A container appends its direct children — contiguous on top of the existing scratch stacks when it closes — as one block and holds a (first, count) span resolved through the tape's shared base pointer, which moves when the tape grows. `properties()` / `items()` keep their signatures so to_js, the printer and the tests are unaffected; an empty container never touches the (possibly still unallocated) tape.
Stage 2 spent a large share of a simple-mode parse (Bun.JSONC.parse, registry manifests) on per-property bookkeeping rather than on the tokens themselves: - Strings: parse_string built and returned a 40-byte E::EString that the simple AST immediately reduced to its byte slice. Split the body location/validation into string_body() and add parse_string_utf8_at(), which returns just the slice (simple mode implies force_utf8) and takes the opening-quote position the caller already resolved, so the clean-string path does one index-window lookup and no struct round trip. parse_json_value and the simple-mode key path use it; the full-AST string path is unchanged. - Container loops: peek() returns the token byte together with its byte position, replacing the peek_byte() + pos_at() pairs in parse_object / parse_array, and the key reuses that position instead of re-resolving it. - parse_json_value is inlined into the container loops, removing a call and a 16-byte Result spill per element. - The object key's token_range (two window lookups + a byte scan) is computed only inside the duplicate-key-warning branch that consumes it; the key Loc only needs the already-known start position. - is_single_line: stop scanning backwards for newlines once the container is already known to be multi-line, and decide the empty-gap (minified) case from the single preceding byte without entering the scan loop. Tried and reverted: sizing the row tape from a projected total row count to avoid the doubling re-copies; it measured 2-3% slower on the same fixture. parse_simple/manifest-abbrev-drizzle-orm (criterion median, interleaved before/after runs on the same pinned core): 3.35 ms -> 2.11 ms (-37%). parse_nowarn (full AST) on the same fixture: 7.07 ms -> 6.66 ms. Pretty-printed pkgjson fixtures: ~-20% from the is_single_line change.
The streaming index pre-zeroes its (tiny) dirty bitmap, so the chunk wrapper takes `&mut [u64]` for it instead of MaybeUninit, and the unused whole-document Rust wrapper is gone (the C entry point stays for the standalone differential harness). Belongs with the streaming-index commit; it was left out of it by mistake.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/parsers/json_stage2.rs (2)
1061-1073: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the bug-history portion out of the code comment.
The invariant is useful, but Lines 1069-1073 describe prior implementation history and performance fallout. Keep the durable ownership invariant here and move the history to the PR/benchmark notes. As per coding guidelines, “Keep code comments to 3 lines max” and “no bug history — that belongs in the PR description.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parsers/json_stage2.rs` around lines 1061 - 1073, The comment on the duplicate-key logic in json_stage2::is_duplicate includes bug-history and performance narrative that should not live in code comments. Trim the doc comment to only the stable ownership/invariant description for the dup_maps spill behavior, and move the historical explanation about the earlier shared map and registry-manifest slowdown to the PR/benchmark notes.Source: Coding guidelines
1157-1163: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute
-after exotic whitespace through signed-number parsing.
parse_number_textexpects an unsigned slice, butparse_scalar_tailpasses tails starting with-for inputs like[\u00a0-1]; the short-integer fast path can underflow in debug or produce a bogus value in release. The standalone-path also rejects-\u00a01even though this parser otherwise preserves exotic-whitespace handling.Also applies to: 1489-1495
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/parsers/json_stage2.rs` around lines 1157 - 1163, `parse_scalar_tail` is sending tails that start with `-` into `parse_number_text`, which only handles unsigned input and can break the short-integer fast path on exotic-whitespace cases like `[\u00a0-1]`. Update the signed-number flow in `parse_scalar_tail` (and the related standalone `-` handling) so any `-` encountered after exotic whitespace is routed through the signed-number parsing path before calling `parse_number_text`, preserving the existing exotic-whitespace behavior for inputs like `-\u00a01`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/parsers/json_stage2.rs`:
- Around line 1061-1073: The comment on the duplicate-key logic in
json_stage2::is_duplicate includes bug-history and performance narrative that
should not live in code comments. Trim the doc comment to only the stable
ownership/invariant description for the dup_maps spill behavior, and move the
historical explanation about the earlier shared map and registry-manifest
slowdown to the PR/benchmark notes.
- Around line 1157-1163: `parse_scalar_tail` is sending tails that start with
`-` into `parse_number_text`, which only handles unsigned input and can break
the short-integer fast path on exotic-whitespace cases like `[\u00a0-1]`. Update
the signed-number flow in `parse_scalar_tail` (and the related standalone `-`
handling) so any `-` encountered after exotic whitespace is routed through the
signed-number parsing path before calling `parse_number_text`, preserving the
existing exotic-whitespace behavior for inputs like `-\u00a01`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e15d551-65fa-49c5-b960-af8de8726c53
📒 Files selected for processing (2)
src/highway/lib.rssrc/parsers/json_stage2.rs
The compact JSON containers (EObjectSimple/EArraySimple) previously only worked through their own properties()/items()/get() surface; every generic Expr helper (get, as_property, as_array, get_array, has_any_property_named, get_by_index, as_property_string_map, is_object/is_array) silently returned None/false for them. Give each helper an arm for the simple containers so a read-only consumer handed a simple root works unchanged. Found values are materialized as Expr nodes via the new Expr::from_json_value (leaves become EString/ENumber/EBoolean/ENull; nested containers re-wrap the existing StoreRef). This allocates one Store node per string and is documented as being for occasional field reads, not hot iteration. ArrayIterator becomes an enum with a Simple variant that materializes each item; the one field poke (package_json.rs) now uses a reset() method. Expr::set/set_string keep rejecting the read-only simple containers (their debug_asserts now check EObject directly), and get_rope documents why no simple arm applies. A new unit test parses the same document in full and simple mode, walks it through the generic accessors only, and asserts both renderings match byte-for-byte (plus fixed expectations so a shared bug cannot cancel out).
parse_utf8_registry now sets simple_objects, so registry manifests are parsed into the compact read-only tape (E::ObjectSimple/E::ArraySimple) instead of the full Expr AST. PackageManifest::parse's two manifest passes (counting and fill) walk E::ObjectSimple::properties() rows and match E::JsonValue directly for the hot version/dependency/bin/dist loops; top-level occasional reads (error, name, modified, dist-tags, time) go through the simple-aware Expr accessors. Adds negatable_from_json_value for cpu/os/libc values coming from the simple AST. On the abbreviated drizzle-orm manifest fixture, the simple parse is ~2.03ms vs ~6.72ms for the full AST it replaces on the registry path.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/parsers/json.rs:138-146— 🟡 Thecontents.len()shortcut forUnterminatedBlockCommentat line 138 fails when the unterminated/*shares a scalar value's index run — e.g.[1 /* never(11 bytes): stage 2'sunexpected(cursor)reports at{offset:1, len:10}, so1+10 <= 11→earlier_stage2_err = trueand the confusing "Unexpected 1 /* never" stands instead of "Expected "*/" to terminate multi-line comment" (which the old lexer reported). #79's original suggestion saidUnterminatedBlockComment"would need aposfield for the/*byte offset"; the shortcut fails whenunexpected()'s range covers the whole run to EOF. The pinned tests ([1] /* unterminated,[@] /* unterminated) don't cover a scalar with/*in its run tail. Fix: giveUnterminatedBlockCommentaposfield (the scalar indexer already knows the/*offset at json_index.rs:~245) and use it here; addexpect_error("[1 /* unterminated", "terminate multi-line comment").Extended reasoning...
What
Commit 72c9ae5 (the fix for resolved #79) added an
earlier_stage2_errprecedence check forsidx.index_errorat json.rs:136-153, so a genuine stage-2 error whose range ends before the offending/is preferred over the index error. ForIndexError::UnterminatedBlockComment— which unlikeUnexpectedSlash { pos }carries no position — line 138 usespos = contents.len()as the precedence anchor:let pos = match e { IndexError::UnterminatedBlockComment => contents.len(), IndexError::UnexpectedSlash { pos } => pos, IndexError::DocumentTooLarge => 0, }; let earlier_stage2_err = log.msgs[log_mark.2..] .iter() .filter(|m| m.kind == bun_ast::Kind::Err) .filter_map(|m| m.data.location.as_ref()) .any(|l| l.offset + l.length.max(1) <= pos);
That is: stage 2's error wins iff its
offset + length <= contents.len()— trivially satisfied by any in-bounds error range that reaches EOF. When an unterminated/*immediately follows a number or keyword (sharing its index run), stage 2'srest_is_ws_coldcannot find*/in the run tail,parse_numberreportsunexpected(cursor)whosetoken_rangecovers the whole run to EOF, and that error wrongly beats the clearer "Expected "*/" to terminate" message.Step-by-step proof:
[1 /* never(11 bytes)- Scalar indexer (SIMD bails on the
/): emits[@0,1@1 (scalar-run start). At/@3 →Some(b'*')→ block-comment loop scans to EOF, no*/→fail(UnterminatedBlockComment)→finish()pushes sentinels[11, 11]. Indices =[0, 1, 11, 11]. parse_numberat cursor=1:full_run = contents[1..11] = "1 /* never".parse_number_textconsumes1(used=1).rest_is_ws_cold(" /* never")(json_stage2.rs:606-608): ati=1b'/',rest.get(2) = Some(b'*'),rest[3..].windows(2).position(b"*/")on" never"→ None → returnsfalse.- →
number_trailing_junk(2).contents[2] = ' ', notis_identifier_start, not\→junk_byte_error(cursor=1, 2, ' ')→js_punct_message(' ')→ None →unexpected(1).token_range(1):_arm, run ="1 /* never"(10 bytes), no trailing whitespace to trim →Range{loc:1, len:10}. Error: "Unexpected 1 / never"* at{offset:1, length:10}. - Back in
parse_impl_in(line 136-153):index_error = Some(UnterminatedBlockComment),pos = contents.len() = 11.earlier_stage2_err:1 + 10.max(1) = 11 <= 11→ true. ReturnErr(SyntaxError)at line 153, keeping stage 2's message instead ofreport_index_error's "Expected "*/" to terminate multi-line comment".
Old lexer: tokenized
1, thennext()skipped/*at byte 3 → block-comment loop → EOF →add_range_error("Expected \"*/\" to terminate multi-line comment")atloc = len. The user saw the clear message.Same for
{"a": true /* forgot,[1, 2, 3 /* end, top-level1 /* x— any scalar value (number/true/false/null) followed by an unterminated/*in the same index run.Why the pinned tests pass
expect_error("[1] /* unterminated", "terminate multi-line comment")(json.rs:2059): the]is its own structural index between1and/. Stage 2 parses[1]successfully (no error), soearlier_stage2_erris vacuouslyfalseand the index error wins. ✓expect_error("[@] /* unterminated", "Decorators are not allowed in JSON")(json.rs:2060):@errors viajs_punct_message→add_error(pos+1, ...)which setslen=0;2 + 0.max(1) = 3 <= 19→ true → the decorator error correctly wins (it IS a genuine earlier error). ✓
Neither test has a scalar VALUE with the
/*in its run tail. The pattern that trips this —unexpected(cursor)reporting the whole run — only fires when the value itself parses fine and only the run-tail check fails.Impact and root cause
Nit — diagnostic-quality only, same 🟡 error-message-quality family as resolved #15/#65/#79 (all filed and fixed by the author). Both old and new correctly return
Err. Affects both strict-JSON and JSONC modes (theindex_errorcheck is dialect-agnostic). The trigger — an unterminated block comment right after a number/keyword — is more plausible in JSONC (e.g. a tsconfig with"foo": 1 /* TODOwhere someone forgot to close it) than the exotic cases in this family, but still an error path.#79's original suggestion said
UnterminatedBlockComment"would need aposfield for the/*byte offset"; commit 72c9ae5 usedcontents.len()as a shortcut. That works when stage 2's error is reported with a small/zero length (js_punct_message, EOF), but not whenunexpected(cursor)reports the whole run. Same residual-incompleteness-in-just-applied-fix pattern as #44→#52, #63→#66, #79→(this).Fix
Give
IndexError::UnterminatedBlockCommentaposfield for the/*byte offset — the scalar indexer already knows it (startat json_index.rs:~245, where the block-comment loop is entered) — and use that at line 138 instead ofcontents.len(). Then for[1 /* never,pos = 3and1 + 10 = 11 > 3→earlier_stage2_err = false→ the index error wins → "Expected "*/" to terminate multi-line comment". Addexpect_error("[1 /* unterminated", "terminate multi-line comment")toerror_messagesalongside the two existing cases at json.rs:2059-2060. - Scalar indexer (SIMD bails on the
-
🟡
src/parsers/json.rs:157-161— 🟡 The #80 fix (if log.errors > log_mark.0at line 159) preserves pre-comment warnings when stage 2 has NO errors, but when stage 2 has BOTH a pre-comment warning AND a post-comment error — e.g. strict-JSON{"a":1,"a":2 // x\n,"b":@}(dup-key WARNING at ~7,@ERROR at ~23, comment at 13) —min_stage2_err=23 >= 13enters the block,log.errors > log_mark.0is true, anddrop_stage2_msgstruncateslog.msgswholesale (line 127), dropping the pre-comment warning along with the post-comment error. The old lexer emitted the warning at 7, then errored at//at 13 (never reaching@), keeping both. Same nit severity as #80; fix by havingdrop_stage2_msgsretainKind::Warnentries for thefirst_commentpath (log.msgs.retain(|m| m.kind != Kind::Err)and only resetlog.errors).Extended reasoning...
What
Commit 4db2504 (HEAD, the fix for #80) gated
drop_stage2_msgs(log)onif log.errors > log_mark.0(line 159) so that when stage 2 has ONLY warnings and no errors, the warnings survive alongside the "JSON does not support comments" error. Butdrop_stage2_msgs(lines 124-128) truncateslog.msgswholesale vialog.msgs.truncate(log_mark.2)and resets bothlog.errorsANDlog.warnings— so when stage 2 emitted BOTH a pre-comment warning AND a post-comment error, the gate is true (there IS a post-comment error to drop),drop_stage2_msgsfires, and the pre-comment warning is discarded along with it.Step-by-step proof: strict-JSON
{"a":1,"a":2 // x\n,"b":@}JSON_OPTS:allow_comments = false,json_warn_duplicate_keys = true. Bytes:{@0"@1a@2"@3:@41@5,@6"@7a@8"@9:@102@11/@13/@14x@16\n@17,@18"@19b@20"@21:@22@@23}@24.- Stage 1: SIMD kernel bails on
/(FLAG_ODDITY); scalar indexer restarts, recordsfirst_comment = Some({loc: 13, len: 4}), emits indices for the whole document (comments skipped, not truncated). - Stage 2 (since 9093ea8 treats comments as whitespace unconditionally):
parse_objectparses key"a"twice →warn_duplicate_keyemitsDuplicate key "a"WARNING at byte ~7. Continues past the comment to"b":@;contents[23] = 0x40→junk_byte_error→ "Decorators are not allowed in JSON" ERROR at byte ~23.log.warnings = 1,log.errors = 1,log.msgs.len() = log_mark.2 + 2. - Line 136:
sidx.index_error = None— skipped. - Line 155-157:
!opts.allow_commentstrue;sidx.first_comment = Some(range@13).min_stage2_err(log)filtersKind::Erronly →Some(~23)..is_none_or(|23| 23 >= 13)→ true. Block entered. - Line 159:
log.errors > log_mark.0→1 > 0→ true →drop_stage2_msgs(log)fires. - Line 127:
log.msgs.truncate(log_mark.2)— the WARNING at byte 7 is discarded along with the error at byte 23.log.warningsreset to 0. - Lines 162-171: log "JSON does not support comments" at byte 13. Return
Err.
Old lexer:
next()walked left-to-right, emitted the dup-key WARNING at ~7 inline, then hit//at byte 13 and (since!allow_comments) calledadd_range_error("JSON does not support comments")there, returningErr— never reaching@. The log kept BOTH the warning at ~7 AND the error at 13.Why the #80 fix is incomplete
#80 filed the case where stage 2 has ONLY a warning (no errors):
{"a":1,"a":2} // x. The fix implemented exactly that — skipdrop_stage2_msgswhenlog.errors == log_mark.0. Butdrop_stage2_msgstruncateslog.msgswholesale, so when there ARE stage-2 errors to drop (which is correct — the@error at 23 is a symptom of parsing past a comment the old lexer would have stopped at), any warnings interleaved before them go too. #80's rationale — "first_commentdoes not truncate the index, so stage 2's output on the pre-comment prefix is genuine" — applies to this warning exactly as it applied in #80: the duplicate key at byte 7 is before the comment at byte 13, so it is a genuine observation the old lexer would have kept.The
duplicate_key_warningstest at json.rs:1472 covers["{\"a\":1,\"a\":2} // x", "{\"a\":1,\"a\":2} /* x"]— neither has a stage-2-detectable error AFTER the comment, so both hit thelog.errors == log_mark.0case #80 fixed and neither exercises this residual.Impact
Nit — same diagnostic-quality-only severity as resolved #15/#65/#79/#80. The trigger requires (a) strict-JSON mode (
allow_comments=false), (b) a duplicate key BEFORE a comment, AND (c) a stage-2-detectable syntax error AFTER the comment, all in the same document — exotic. The parse correctly fails either way; only a warning is lost, costing the user one extra fix-and-re-parse cycle. Same residual-incompleteness-in-just-applied-fix pattern as #44→#52, #63→#66, #74→#75→#76, #79→#80 on this PR.Fix
Have
drop_stage2_msgsretainKind::Warnentries for thefirst_commentpath — warnings from the successfully-parsed pre-comment prefix are always genuine (per #15's own rationale). E.g.:let drop_stage2_errs = |log: &mut bun_ast::Log| { log.errors = log_mark.0; log.msgs.retain(|m| m.kind != bun_ast::Kind::Err); // do not reset log.warnings };
used at line 160 (the
first_commentblock); theindex_errorblock at line 149 can keep the wholesale drop since a truncated index makes even pre-truncation warnings suspect. Add e.g."{\"a\":1,\"a\":2 // x\n,\"b\":@}"to theduplicate_key_warningsloop at json.rs:1472 assertingp.warnings == 1. - Stage 1: SIMD kernel bails on
-
🟡
src/parsers/json.rs:249-255— 🟡 The block-comment skip added toguess_indentation()in 4db2504 (the fix for #81) is a raw byte scan with no string awareness, so a/*inside a JSON string on line 1 is treated as a block-comment opener — e.g.{"workspaces": ["packages/*"],\n "name": ...}now returnsIndentation::default()(2-space) instead of{Space, 4}, becauseindex_of(&s[27..], b"*/")finds nothing and bails. Before 4db2504 the function only looked for\n(which cannot appear raw in a JSON string), so this is a strictly-new regression from the HEAD commit; the old lexer-integratedindent_inforan inside the token loop and never saw string-body bytes. Same purely-cosmetic re-print-layout family/severity as #81; fix by adding ab'"'arm that scans to the closing quote (the same shape asskip_string_tokenat line 776), and addguess_indentation(b"{\"w\":[\"packages/*\"],\n \"a\":1}")→{Space, 4}toindentation_skips_block_comments.Extended reasoning...
What
The block-comment skip at
guess_indentation()(json.rs:249-255) — added by HEAD commit 4db2504 as the fix for PR comment #81 — is a raw byte walk with no string awareness:if s[i] == b'/' && s.get(i + 1) == Some(&b'*') { let Some(close) = bun_core::strings::index_of(&s[i + 2..], b"*/") else { return Indentation::default(); }; i += 2 + close + 2; continue; }
There is no
b'"'arm and no in-string state, so a/*inside a JSON string literal on line 1 (before the first\n+indent) is treated as a block-comment opener. If no*/appears later in the document, the function returnsIndentation::default()instead of the file's real indentation.Step-by-step proof:
{"workspaces": ["packages/*"],\n "name": "foo"}Bytes:
{@0"@1 …s@24/@25*@26"@27]@28,@29\n@30"@35 …After 4db2504 (HEAD):
i=0..24: no/, no\n→i += 1each.i=25:s[25]='/',s.get(26)=Some(&b'*')→ enters the block-comment arm.index_of(&s[27..], b"*/")over"],\n "name": "foo"}— no*/→ returnsIndentation::default()(2-space).
Before 4db2504 (no block-comment arm):
i=0..29: no\n→i += 1each (bytes 25/26 are just plain bytes).i=30:s[30]='\n'→i=31,s[31]=' 'matches → returns{Space, 4}. ✓
Same divergence for any glob-like string on line 1 (
"dist/*","src/**/*.ts") or a URL path segment containing/*. The old lexer-integratedindent_info(which #81 compared against) ran inside the token loop and never saw string-body bytes, so it did not have this problem either.Why this is a strictly-new regression from HEAD
Before 4db2504,
guess_indentation()only looked for\n— and a raw\ncannot appear inside a JSON string (a string containing one fails to parse beforeguess_indentationruns, since it's called insiderun_stage2'sOkbranch at json.rs:189). So/*inside a string was harmless: the scan walked past it and found the real first\n+indent. 4db2504 added the/*arm, which introduces the string-content confusion. This is the same residual-incompleteness-in-just-applied-fix pattern seen throughout this PR's review (#44→#52, #74→#75→#76, #79→#80).Why nothing catches it
The
indentation_skips_block_commentstest at json.rs:1481 (added by 4db2504) covers only/* c\n x */\n{\n\t"a": 1\n}— a real block comment before the object. No case has/*inside a string. The standard pretty-printed layout{\n ...is unaffected: the\nat byte 1 short-circuits before any string is scanned.Impact
Same purely-cosmetic re-print-layout impact as #81 (nit severity):
guess_indentationis called whenopts.guess_indentationis set —WorkspacePackageJSONCache(backingbun add/remove/update's package.json rewrite) and yarn.lock migration, all underPACKAGE_JSON_OPTS. The guessed indentation feeds the printer'sindentwhen the file is written back. The trigger requires/*inside a string on line 1 of a package.json — less common than the standard{\nlayout, but"workspaces": ["packages/*"]on line 1 of a compact/single-line-header package.json is plausible. Not a parse-correctness, value, or error-message issue.Fix
Add a
b'"'arm that scans to the closing quote before the/*check — the same shape asskip_string_tokenat json.rs:776:if s[i] == b'"' { i += 1; while i < s.len() { match s[i] { b'\\' => i += 2, b'"' => { i += 1; break; } _ => i += 1, } } continue; }
and add
guess_indentation(b"{\"w\":[\"packages/*\"],\n \"a\":1}")→{Space, 4}toindentation_skips_block_comments.
|
The review's finding (the |
A scalar whose index run contains the unterminated comment ([1 /* never) produced a stage-2 junk error whose range ends at EOF, which the precedence rule read as an earlier independent error.
… the drop; string-aware indentation scan Three follow-ups on the error-precedence rule: a strict-mode comment that precedes the index error is the error to report; dropping stage-2 messages now removes only errors, so a duplicate-key warning earlier in the document survives; and guess_indentation skips strings so a '/*' inside one is not taken for a comment opener.
The JSON parser rewrite in #33032 routes a define value whose first character is not JSON-like, a number, or a keyword through the auto-quote string path, which fixes the 1.4 regression in #32686 where values like ./src/worker.ts and /$bunfs/root/worker were rejected. Nothing covered those path-shaped cases, so add a regression test for them (plus the bare . / /= forms and the --define CLI entry point).
The JSON parser rewrite in #33032 routes a define value whose first character is not JSON-like, a number, or a keyword through the auto-quote string path, which fixes the 1.4 regression in #32686 where values like ./src/worker.ts and /$bunfs/root/worker were rejected. Nothing covered those path-shaped cases, so add a regression test for them (plus the bare . / /= forms and the --define CLI entry point).
… or / (#32688) Regression coverage for #32686. ### Background Bun 1.4 regressed on `Bun.build({ define })` (and `bun build --define`): a value beginning with a bare `.` or `/` (for example `"./src/worker.ts"` or a `/$bunfs/...` path produced when cross-compiling) was rejected at parse time instead of being auto-quoted and treated as a string literal, as Bun 1.3 did. ``` 1 | ./src/worker.ts ^ error: Syntax Error at defines.json:1:1 ``` ### What changed This PR originally fixed the issue in the old lexer-based JSON parser (`src/parsers/json_lexer.rs`). While it was open, #33032 rewrote the JSON parser around a SIMD structural index and removed `json_lexer.rs`. The new `parse_env_json` dispatches a define value by its first byte: JSON-like starts (`{ [ 0-9 " '`, or `-`/`.` that lead a number) go to the classic parser, `true`/`false`/`null`/`undefined` are keywords, and everything else, including values starting with `.` or `/`, goes through the auto-quote string path. That independently fixes the regression, so the original `json_lexer.rs` change is obsolete and has been dropped in the rebase. The existing auto-quote test block in `test/bundler/bun-build-api.test.ts` covers `*`/`?`/`(`/`)` starts but not the `.`/`/` path-shaped values from this issue, and #33032 added JSONC tests rather than define-path coverage. This PR keeps the regression test so those cases stay guarded. ### Test `test/regression/issue/32686.test.ts` asserts each value round-trips as a quoted string literal: `./src/worker.ts`, `../src/worker.ts`, `/abs/path`, `/$bunfs/root/worker`, `.`, `/`, `/=foo`, plus the `bun build --define` CLI entry point. - `bun bd test test/regression/issue/32686.test.ts` on current main (27009bf): 8 pass. Note: the fix already lives in main via #33032, so this is a test-only change. It does not carry its own `src/` diff.
2 participants
Fixes #8782
What
Bun's JSON/JSONC parser (
bun_parsers::json— every package.json read, npm registry manifests, tsconfig/Bun.JSONC.parse, bun.lock, the bundler's.json/.jsoncloaders,bun audit, source maps,--define/.env; notJSON.parse, which is JSC) is rewritten around a SIMD structural index, and the old byte-at-a-time lexer + mutable-AST builder is deleted. The parser now produces exactly one thing: an immutable row AST that owns its memory.Stage 1 —
src/jsc/bindings/highway_json.cppOne Highway kernel (runtime-dispatched like the other
highway_*TUs) classifies 64 input bytes at a time (two-nibble LUT + vector compares), resolves\"with simdjson's odd-backslash-run trick, derives the in-string mask with a prefix-XOR, and emits the byte offset of every structural character, both quotes of every string, and the first byte of every other scalar with a branchlessCompressStore, plus a per-64-byte-block "string needs a scan" bitmap. 3.3–3.9 GB/s on real manifests. The index is streamed, never materialized: the kernel is resumable, so stage 2 pulls indices through a ~32 KB sliding window refilled 8 KB of input at a time. Comments/single quotes (JSONC) poison SIMD quote parity, so the kernel reports the first/or'outside a string and indexing restarts with a scalar, comment-aware indexer that produces a bit-identical stream (differentially tested against the SIMD kernel).Stage 2 — the immutable row AST (
E::ObjectJSON/E::ArrayJSON)Recursive descent over the index stream. A clean string is a zero-copy slice of the source (the old parser walked every string byte); escaped strings decode directly to WTF-8 (no UTF-16 round-trip; lone
\uD800survives exactly likeJSON.parse).Profiling showed the parser was no longer the cost — materializing the JS-oriented AST was: an
EStringStore node for every key and string value inside a 112-byteG::Propertywith baggage JSON can never use. So containers are oneExprnode holding a(first, count)span of the document'sE::JsonTape:E::PropertyJSONrow, a value is an inline 16-byte taggedE::JsonValue;Vecs (property rows, item rows, stable chunks for escape-decoded bytes) owned by the document —ParsedJson { root, tape }— and freed byDrop. No arena, no per-parsemi_heap_new/destroy(which cost ~5µs and made the compact AST a net loss on small documents before);TapeAlloc::Arenaputs the tape (and every buffer in it) inside a caller's arena instead, for the bundler/module-loader world where the AST's lifetime is an arena and nothing runsDrop;Locinto a tape column (record_value_locs) — only when the entry point will materialize (below) — so exact locations cost the immutable hot path nothing.Who reads rows, who gets a classic tree
Read-only consumers iterate the rows directly (no
E::Objectis ever built): the resolver's package.json + tsconfig readers,Package::parseand the lockfile/override/catalog walkers, bun.lock, package-lock.json migration, npm registry manifests,Bun.JSONC.parse, audit/bunx/--filter/security-scanner/whoami, source maps, the standalone graph, the per-install name/version check, and runtimeimport data from "./x.json"(rows →to_js, nothing printed).Consumers that genuinely need the mutable
E::Objecttree get one materialized from the rows at their entry point — with every node's exact, parser-recorded source location — because they edit it, print it, or splice it into a JavaScript AST: the package.json editors (bun pm pkg/version/trust,init,add/remove,publish/packvia the workspace cache), the bundler's.jsonnamed-exports rewrite, macros,--define/.env, bunfig/npmrc (their walkers are shared with TOML/INI), and yarn.lock migration. A differential test proves the materialized tree is node-for-node identical (locations,is_single_line, canonical JSON) to what the deleted classic parser produced.Diagnostics that point at a property value or array item recover the location at the error site by re-scanning the source from the locations the rows do keep (
json::property_value_loc/array_item_loc); locations are never stored on the hot path.Behavior fixes found along the way
Bun.JSONC.parse('"\ud800"')returnedU+FFFDwhereJSON.parsereturns the lone surrogate (two independently lossy conversions). Both fixed; JSON escapes now round-trip as WTF-8 end to end (Bun.JSONC.parse, imported.json,bun build,--define), withJSON.parsedifferential tests.[-a]parsed as the number-49;[--]could panic a debug build.--define V=-1/V=.5became strings instead of numbers (first-byte dispatch missed-/.)..jsondocument deeper than the printer's stack or larger than 2 GiB now errors instead of overflowing/panicking.Locplumbing was write-only (dead) and is gone, along with the legacy framework loaders inpackage_json.rs.Behavior notes
to_jsare WTF-8-native, so output is unchanged (and lone surrogates are now more correct, see above).Tests
cargo test -p bun_parsers(scripts/bench-json-rust.sh --test): JSONC/lenient extensions, escapes, exact error messages, duplicate keys,is_single_line, the materialize-vs-classic and location-recovery differentials, a 20k-document SIMD-vs-scalar indexer differential, and the streaming/chunked paths.test/js/bun/jsonc/jsonc.test.ts: differential vsJSON.parseon generated documents, escape-heavy strings, lone/paired surrogates, BOM/exotic whitespace, control characters, huge documents, and a linear-time guarantee on pathological inputs.test/js/bun/resolve/jsonc.test.ts: runtime.json/.jsoncimport +Bun.JSONC.parsevsJSON.parse, code-unit-exact, including lone surrogates and non-ASCII.bun-install, workspaces,bun-lock/lockb, migrations (npm/yarn/pnpm),bun-pm-pkg/version,bun-audit, jsonc/tsconfig resolution,bundler_loader,env.Benchmarks
M4 Max, macOS. Three binaries: branch (4369851, release build), main (52a1ddf, release build, same machine/flags), and the official bun 1.3.14 release. Install runs are fully offline: a local static server on 127.0.0.1 serves the real npm manifest JSON (abbreviated packuments saved from registry.npmjs.org), tarball caches are pre-warmed per binary,
--ignore-scripts. hyperfine, ≥10 runs each.bun install
"fresh resolve" deletes
bun.lockand the binary manifest cache (*.npm) before every run, so the run re-fetches and re-parses every manifest from JSON;--dry-runis that resolution without linking.T3-stack Next.js app (
bench/install: 96 packages, 117 MB of manifest JSON —nextalone is a 25 MB packument):--dry-run)The bun repo's
test/package (1,380 packages, ~300 MB of manifest JSON across 1,616 manifests):--dry-run)bun.lock(rm node_modules)When one huge manifest sits on the resolution critical path (the T3 app), wall clock follows the parser. With 1,616 mostly-small manifests arriving over HTTP, wall clock is transfer-bound and the win shows up as CPU headroom instead. The lockfile and no-op paths are stat/clonefile-bound and were never parser-limited — the goal there is no regression.
bun build
--minify.jsonimportsJS-heavy bundles are bound by the JS parser/printer; the point of the first two rows is no regression. (Their 1.3.14 deltas are other main-branch improvements, not this PR.)
Bun.JSONC.parseend-to-end (bench/json-corpus/jsonc-parse.mjs, median of 30, parse → JSValue)These numbers include conversion to a JSValue (the script's
JSON.parsereference column is flat across all three binaries), which caps the end-to-end ratio well below the parser-only win —scripts/bench-json-rust.shisolates the parser. The small-document rows recover the regression main carried from the earlier arena-based compact AST (ami_heapcreate/destroy per parse) and land ahead of 1.3.14 as well.In-tree tooling:
bench/json-corpus/fetch.sh(corpus),scripts/bench-json-rust.sh(criterion:stage1,parse_immutable= rows,parse_nowarn= rows + materialize, vs the old parser at the merge base),bench/json-corpus/jsonc-parse.mjs(Bun.JSONC.parseend-to-end).Build note
highway_json.cppis compiled-O2in debug profiles (afileOverridesentry + PCH opt-out): at-O0every highway op is an outlined call passing 64-byte vectors by value, which is both broken under the debug sanitizer set and uselessly slow.