XML parser: SIMD structural index + tape rows, one-call rows→JS - #37146
Conversation
…rser WIP checkpoint: Highway kernel + scalar indexer, stage 2 hops index entries for content/attribute values/comments/CDATA/PIs, ObjectJSON/ArrayJSON rows on a JsonTape, criterion bench vs pugixml/expat/libxml2/quick-xml/roxmltree.
…tribute-value paths, nonchar detection in the kernel
…TF-8 validation for already-decoded input
… presize XML staging stacks; bench additions
…nded reservations, reused fold map, layout asserts, robust WTF-8 decode, shared ToJSError mapping, arena TLS without a destructor; tests
No-Verification-Needed: lint-only change
|
Updated 6:54 PM PT - Aug 7th, 2026
✅ @Jarred-Sumner, your commit ee1d72390a314884209b0a34b5cbff5c4319539e passed in 🧪 To try this PR locally: bunx bun-pr 37146That installs a local version of the PR into your bun-37146 --bun |
…rgets No-Verification-Needed: CI allowlist only
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Fix your slop
| return {}; | ||
| } | ||
| const RowValue* rows = m_items + a.first; | ||
| JSArray* array = constructEmptyArray(m_globalObject, nullptr, a.count); |
There was a problem hiding this comment.
Use a MarkedArgumentsBuffer with a JSC::constructArray, or tryCreateUninitializeRestricted but MarkedArgumentsBuffer is safer. Do that for both.
There was a problem hiding this comment.
Done in 19dc05c: array() collects into a MarkedArgumentBuffer (ensureCapacity + hasOverflowed check) and calls JSC::constructArray; objects still constructEmptyObject + putDirect (now with the exception check right after construction).
|
|
||
| // The parsers hand over WTF-8: UTF-8 that may also encode lone surrogates (from JSON | ||
| // `\uD800`-style escapes). Anything malformed becomes U+FFFD, one per offending byte. | ||
| static String decodeWTF8(std::span<const Latin1Character> bytes) |
There was a problem hiding this comment.
Terrible slop. Not only are there at least 10 implementations of this function in the codebase, this precise one should be completely unnecessary.
There was a problem hiding this comment.
Removed. The converter is now templated on the tape's encoding with three paths: Latin-1 and UTF-16 spans go straight into jsonAtomStringCache.makeIdentifier / tryMakeJSString, UTF-8 takes the same Latin-1 path when ASCII and String::fromUTF8 otherwise; the only thing that can fail strict decoding is WTF-8 from a JSON \uD800-style escape, and that falls back to the existing Rust wtf8_to_utf16_alloc path (Bun__JSONRows__wtf8ToJS) instead of a new decoder. Bun.XML.parse now parses an 8-bit JS string byte-per-character with no re-encoding and produces a Latin-1 tape (a UTF-8 reparse happens only if a character reference needs something above U+00FF); UTF-16 strings still go through one simdutf transcode to UTF-8 for now — making the scanner generic over the code unit so 16-bit strings are parsed natively (and produce a UTF-16 tape, which the converter already handles) is what I'm doing next.
|
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:
WalkthroughChangesThe PR adds SIMD and scalar XML structural indexing with resumable chunk support. It routes XML rows through JavaScriptCore conversion, adds source-encoding handling, and expands XML benchmarks, parser tests, build generation, and documentation. ChangesXML parsing and conversion pipeline
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/build/bun.ts (1)
309-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the PCH opt-out comment to name both files.
scripts/build/flags.tsapplies-O2in debug profiles to bothhighway_json.cppandhighway_xml.cpp.🤖 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 `@scripts/build/bun.ts` around lines 309 - 315, Update the comment above the cfg.debug PCH opt-out in the build script to explicitly name both highway_json.cpp and highway_xml.cpp, accurately reflecting that flags.ts applies -O2 to each file in debug profiles.
🤖 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/xml/xml.mjs`:
- Around line 82-85: Define one strict BUN_XML_BENCH_FIXTURES contract across
both runners: in bench/xml/xml.mjs lines 82-85, distinguish unset from an empty
value, reject empty configuration, and resolve the configured directory to an
absolute path before file operations; in src/parsers/benches/xml_parse.rs lines
23-43, apply the same policy and propagate directory enumeration, entry, and
file-read errors instead of silently ignoring failures or unwrapping results.
In `@scripts/bench-json-rust.sh`:
- Line 55: Update the pugixml download and extraction command in the benchmark
setup to retain the optional fallback while explicitly warning when it fails;
include “pugixml” in the warning and advise retrying the benchmark or dependency
download, without silently treating the failure as success.
In `@src/js_parser_jsc/expr_jsc.rs`:
- Around line 28-35: Update to_js_error to replace the wildcard fallback with
explicit matching for every remaining ToJSError variant, including
CannotConvertIdentifierToJS and CannotConvertArgumentTypeToJS, and include each
variant’s name in the thrown error message. Keep the existing mappings for
OutOfMemory, JSError, and JSTerminated, and make the match exhaustive so future
variants require an explicit error message.
In `@src/jsc/bindings/JSONRowsToJS.cpp`:
- Around line 68-71: Add direct compile-time assertions next to the Rust
JsonValue definition in E, asserting its size is 16 bytes and its payload starts
at offset 4, with a brief note documenting the repr(C, u32) layout and matching
C++ assertion. Leave the existing PropertyJSON layout assertions unchanged.
- Around line 110-114: In the JSON row conversion flow, update the
constructEmptyObject call in JSONRowsToJS so it immediately checks for an
exception with RETURN_IF_EXCEPTION(scope, {}) before object is used by the
following loop.
In `@src/parsers/benches/support/xml_c_shim.cpp`:
- Around line 35-55: Update bench_expat_parse and bench_libxml2_parse to return
0 when len exceeds INT_MAX before narrowing it for the native parser calls. In
bench_expat_parse, also check XML_ParserCreate(NULL) immediately and return 0
before configuring or parsing with a null parser.
In `@src/parsers/benches/xml_parse.rs`:
- Around line 107-115: Harden the BUN_XML_BENCH_LOOP parsing flow so an unset
variable keeps the existing skip behavior, while any set value must contain
exactly three non-empty fields in impl:fixture:iterations format. Validate known
implementations and fixtures, reject invalid or zero iterations, and guard
contents.len() * n against overflow. Replace unwrap/expect/panic paths with one
recoverable contextual error naming the variable, rejected value, and accepted
format.
In `@src/parsers/xml_index.rs`:
- Around line 218-246: Update the test helpers and agreement tests around
build_both, simd_and_scalar_indexers_agree, and
streaming_and_scalar_indexers_agree_on_large_documents to detect when the SIMD
kernel is unavailable via bun_core::env::IS_NATIVE and skip with an explicit
reason instead of comparing scalar output against itself. Ensure all relevant
build_both call sites use this guard so the tests only run when SIMD execution
is actually available.
- Around line 51-107: Document and enforce the sequential-access invariant in at
and fill_to: add a debug assertion rejecting logical positions older than the
retained lookbehind/base before any wrapping subtraction, and ensure seek
handles the same backward-access violation rather than restarting from the
window end. In fill_to, clamp keep_from to win.len() before copy_within/truncate
so an initial forward jump with an empty window cannot panic.
- Around line 35-48: Add a recoverable size validation in Scanner::build_index
before constructing StructuralIndex, rejecting inputs larger than u32::MAX
rather than relying on with_producer’s debug_assert!. Apply the same validation
after UTF-16 or Latin-1 transcoding, and return the parser’s established error
type so finish, scalar_chunk, and the Highway kernel never receive oversized
documents.
In `@test/js/bun/xml/xml.test.ts`:
- Around line 376-389: Add a numeric-key ordering test to
test/js/bun/jsonc/jsonc.test.ts using Bun.JSONC.parse on {"a":1,"0":2,"b":3},
and assert Object.keys(result) equals ["0", "a", "b"]. Keep this coverage in the
JSONC tests rather than extending the XML test.
---
Outside diff comments:
In `@scripts/build/bun.ts`:
- Around line 309-315: Update the comment above the cfg.debug PCH opt-out in the
build script to explicitly name both highway_json.cpp and highway_xml.cpp,
accurately reflecting that flags.ts applies -O2 to each file in debug profiles.
🪄 Autofix
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: da6d6749-5b76-4b68-a482-cd58f88a434a
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockbench/xml/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
bench/xml/package.jsonbench/xml/xml.mjsscripts/bench-json-rust.shscripts/build/bun.tsscripts/build/codegen.tsscripts/build/flags.tsscripts/build/unified.tsscripts/build/xmlByteClass.tsscripts/verify-baseline-static/allowlist-aarch64.txtscripts/verify-baseline-static/allowlist-x64-windows.txtscripts/verify-baseline-static/allowlist-x64.txtsrc/ast/e.rssrc/ast/lib.rssrc/bundler/ParseTask.rssrc/highway/lib.rssrc/js_parser_jsc/expr_jsc.rssrc/js_parser_jsc/lib.rssrc/jsc/bindings/JSONRowsToJS.cppsrc/jsc/bindings/highway_xml.cppsrc/parsers/Cargo.tomlsrc/parsers/benches/support/xml_c_shim.cppsrc/parsers/benches/xml_parse.rssrc/parsers/build.rssrc/parsers/json.rssrc/parsers/lib.rssrc/parsers/xml.rssrc/parsers/xml_index.rssrc/runtime/api.rssrc/runtime/api/JSONCObject.rssrc/runtime/api/XMLObject.rstest/js/bun/xml/xml.test.ts
| if (process.env.BUN_XML_BENCH_FIXTURES) { | ||
| const dir = process.env.BUN_XML_BENCH_FIXTURES; | ||
| for (const f of readdirSync(dir).sort()) { | ||
| if (/\.(xml|svg)$/.test(f)) docs.push([f, readFileSync(join(dir, f), "utf8")]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Define one strict BUN_XML_BENCH_FIXTURES contract.
The JavaScript runner treats an empty value as unset and accepts relative paths. The Rust runner treats an empty value as an unreadable directory, ignores that error, and can panic while reading a selected fixture. The same configuration can therefore benchmark different corpora or silently fall back to synthetic inputs.
bench/xml/xml.mjs#L82-L85: Distinguish unset from empty, reject an empty configured value, and resolve the configured directory to an absolute path before file operations.src/parsers/benches/xml_parse.rs#L23-L43: Apply the same empty and absolute-path policy. Propagate directory enumeration, entry, and file-read errors instead of usingif let Ok,filter_map(|e| e.ok()), andunwrap().
As per coding guidelines, use absolute file paths, distinguish empty and unset input, and propagate I/O failures explicitly.
📍 Affects 2 files
bench/xml/xml.mjs#L82-L85(this comment)src/parsers/benches/xml_parse.rs#L23-L43
🤖 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 `@bench/xml/xml.mjs` around lines 82 - 85, Define one strict
BUN_XML_BENCH_FIXTURES contract across both runners: in bench/xml/xml.mjs lines
82-85, distinguish unset from an empty value, reject empty configuration, and
resolve the configured directory to an absolute path before file operations; in
src/parsers/benches/xml_parse.rs lines 23-43, apply the same policy and
propagate directory enumeration, entry, and file-read errors instead of silently
ignoring failures or unwrapping results.
Source: Coding guidelines
| XML_C_LIBS=() | ||
| PUGI_VERSION=1.14 | ||
| if [ ! -f "$SUP/pugixml-$PUGI_VERSION/src/pugixml.cpp" ]; then | ||
| curl -fsSL "https://github.com/zeux/pugixml/releases/download/v$PUGI_VERSION/pugixml-$PUGI_VERSION.tar.gz" | tar -xz -C "$SUP" || true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report the skipped pugixml comparison.
|| true converts a failed download or extraction into a successful script result. The benchmark then runs without identifying that it omitted pugixml. Keep the optional fallback, but print a warning that names the failed dependency and gives a retry action.
As per coding guidelines, never swallow failures or signal success after failure.
🤖 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 `@scripts/bench-json-rust.sh` at line 55, Update the pugixml download and
extraction command in the benchmark setup to retain the optional fallback while
explicitly warning when it fails; include “pugixml” in the warning and advise
retrying the benchmark or dependency download, without silently treating the
failure as success.
Source: Coding guidelines
| pub fn to_js_error(e: ToJSError, global: &JSGlobalObject) -> JsError { | ||
| match e { | ||
| ToJSError::OutOfMemory => JsError::OutOfMemory, | ||
| ToJSError::JSError => JsError::Thrown, | ||
| ToJSError::JSTerminated => JsError::Terminated, | ||
| _ => global.throw(format_args!("Cannot convert value to JS")), | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Name the rejected variant in the fallback error message.
to_js_error is public and re-exported from the crate root. The _ arm therefore reaches callers that convert arbitrary Expr values, not only JSON and XML rows. Those callers receive "Cannot convert value to JS", which does not say whether the failure was CannotConvertIdentifierToJS or CannotConvertArgumentTypeToJS.
Include the variant in the message so the thrown error identifies the rejected value kind.
🐛 Proposed fix
pub fn to_js_error(e: ToJSError, global: &JSGlobalObject) -> JsError {
match e {
ToJSError::OutOfMemory => JsError::OutOfMemory,
ToJSError::JSError => JsError::Thrown,
ToJSError::JSTerminated => JsError::Terminated,
- _ => global.throw(format_args!("Cannot convert value to JS")),
+ ToJSError::CannotConvertIdentifierToJS => {
+ global.throw(format_args!("Cannot convert an identifier to a JS value"))
+ }
+ ToJSError::CannotConvertArgumentTypeToJS => {
+ global.throw(format_args!("Cannot convert this expression type to a JS value"))
+ }
}
}An exhaustive match also makes a new ToJSError variant a compile error instead of a silent generic message.
Based on the coding guideline "Error messages must identify the failed resource, violated constraint, rejected value, cause, and concrete remedy while preserving rich underlying errors."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn to_js_error(e: ToJSError, global: &JSGlobalObject) -> JsError { | |
| match e { | |
| ToJSError::OutOfMemory => JsError::OutOfMemory, | |
| ToJSError::JSError => JsError::Thrown, | |
| ToJSError::JSTerminated => JsError::Terminated, | |
| _ => global.throw(format_args!("Cannot convert value to JS")), | |
| } | |
| } | |
| pub fn to_js_error(e: ToJSError, global: &JSGlobalObject) -> JsError { | |
| match e { | |
| ToJSError::OutOfMemory => JsError::OutOfMemory, | |
| ToJSError::JSError => JsError::Thrown, | |
| ToJSError::JSTerminated => JsError::Terminated, | |
| ToJSError::CannotConvertIdentifierToJS => { | |
| global.throw(format_args!("Cannot convert an identifier to a JS value")) | |
| } | |
| ToJSError::CannotConvertArgumentTypeToJS => { | |
| global.throw(format_args!("Cannot convert this expression type to a JS value")) | |
| } | |
| } | |
| } |
🤖 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/js_parser_jsc/expr_jsc.rs` around lines 28 - 35, Update to_js_error to
replace the wildcard fallback with explicit matching for every remaining
ToJSError variant, including CannotConvertIdentifierToJS and
CannotConvertArgumentTypeToJS, and include each variant’s name in the thrown
error message. Keep the existing mappings for OutOfMemory, JSError, and
JSTerminated, and make the match exhaustive so future variants require an
explicit error message.
Source: Coding guidelines
| // Kept in step with the `offset_of!` assertions next to the Rust definitions. | ||
| static_assert(offsetof(RowValue, tag) == 0 && offsetof(RowValue, string) == 4); | ||
| static_assert(offsetof(RowProperty, key) == 0 && offsetof(RowProperty, keyLoc) == 12 && offsetof(RowProperty, value) == 16); | ||
| static_assert(offsetof(RowSpan, tape) == 0 && offsetof(RowSpan, first) == 8 && offsetof(RowSpan, count) == 12); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Pin the JsonValue payload offset with a direct Rust assertion.
The C++ side asserts offsetof(RowValue, string) == 4 and sizeof(RowValue) == 16. The Rust side asserts neither for JsonValue. It only pins the size transitively, through size_of::<PropertyJSON>() == 32 and offset_of!(PropertyJSON, value) == 16.
If JsonValue gains a variant with 8-byte alignment, the Rust payload moves to offset 8 and the size becomes 24. PropertyJSON then becomes 40 bytes, so the existing assertion does fire. The failure message points at PropertyJSON, not at the enum that changed. Add the direct assertions next to JsonValue in src/ast/e.rs so the compile error names the real cause.
const _: () = assert!(core::mem::size_of::<E::JsonValue>() == 16);
// `#[repr(C, u32)]`: the payload follows the 4-byte discriminant.
// Read from C++ (JSONRowsToJS.cpp), which asserts the same offset.🤖 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/jsc/bindings/JSONRowsToJS.cpp` around lines 68 - 71, Add direct
compile-time assertions next to the Rust JsonValue definition in E, asserting
its size is 16 bytes and its payload starts at offset 4, with a brief note
documenting the repr(C, u32) layout and matching C++ assertion. Leave the
existing PropertyJSON layout assertions unchanged.
| let Ok(spec) = std::env::var("BUN_XML_BENCH_LOOP") else { | ||
| return; | ||
| }; | ||
| let mut parts = spec.split(':'); | ||
| let (imp, fx, n) = ( | ||
| parts.next().unwrap(), | ||
| parts.next().unwrap(), | ||
| parts.next().unwrap().parse::<usize>().unwrap(), | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Reject malformed loop specifications without panic.
Empty fields, a zero iteration count, invalid integers, unknown fixtures, and unknown implementations reach unwrap, expect, or panic!. A large valid usize can also overflow contents.len() * n on Line 157.
Parse exactly three non-empty fields. Reject zero and overflowing iteration counts. Return one contextual error that identifies BUN_XML_BENCH_LOOP, the rejected value, and the accepted impl:fixture:iterations format.
As per coding guidelines, distinguish empty, zero, and unset values, validate numeric boundaries, and expose user-reachable failures as recoverable errors.
Also applies to: 151-157
🤖 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/xml_parse.rs` around lines 107 - 115, Harden the
BUN_XML_BENCH_LOOP parsing flow so an unset variable keeps the existing skip
behavior, while any set value must contain exactly three non-empty fields in
impl:fixture:iterations format. Validate known implementations and fixtures,
reject invalid or zero iterations, and guard contents.len() * n against
overflow. Replace unwrap/expect/panic paths with one recoverable contextual
error naming the variable, rejected value, and accepted format.
Source: Coding guidelines
| fn with_producer(contents: &'c [u8], use_scalar: bool) -> Self { | ||
| debug_assert!(contents.len() <= i32::MAX as usize); | ||
| let win_cap = contents.len().min(REFILL_INPUT) + 64 + SENTINELS + LOOKBEHIND; | ||
| StructuralIndex { | ||
| contents, | ||
| win: Vec::with_capacity(win_cap), | ||
| base: 0, | ||
| done: false, | ||
| src_off: 0, | ||
| kernel_state: [0; 3], | ||
| use_scalar, | ||
| s_in_tag: false, | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the callers of StructuralIndex and any document-size gate on the XML parse path.
set -euo pipefail
rg -n -C 6 'StructuralIndex::new|StructuralIndex::with_producer' --type=rust
echo '--- XML parse entry points and any length gates ---'
fd -t f 'xml.rs|XMLObject.rs' src --exec rg -n -C 4 'i32::MAX|u32::MAX|MAX_LEN|too large|len\(\) *[<>]' {}Repository: oven-sh/bun
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- StructuralIndex definitions and uses ---'
rg -n -C 8 'StructuralIndex|with_producer|fn new' src/parsers --type rust || true
printf '%s\n' '--- XML-related source files ---'
fd -t f -i 'xml' src | head -80
printf '%s\n' '--- Relevant parser entry points and size checks ---'
rg -n -C 5 'parse|from_utf8|contents|i32::MAX|u32::MAX|MAX_LEN|too large|len\(\)' src/parsers --type rust | head -400Repository: oven-sh/bun
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- XML index implementation ---'
cat -n src/parsers/xml_index.rs | sed -n '1,170p'
printf '%s\n' '--- XML parse construction and scanner state ---'
cat -n src/parsers/xml.rs | sed -n '70,105p;540,610p;2635,2705p'
printf '%s\n' '--- All XML entry-point callers ---'
rg -n -C 5 'XML::parse|parse_document\(|build_index\(' src --type rust
printf '%s\n' '--- XML size-limit and error definitions ---'
rg -n -C 4 'DocumentTooLarge|AllocError|SyntaxError|pub enum Error|type Result' src/parsers/xml.rs src/parsers src --type rust | head -300Repository: oven-sh/bun
Length of output: 48166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Highway XML wrapper and native signature ---'
rg -n -C 12 'xml_structural_index_chunk|base_offset|uint32_t|int32_t|uint32' src bun* scripts --glob '*.{rs,cpp,cc,h,hpp,zig,ts}' | head -300
printf '%s\n' '--- Scanner source-buffer transitions ---'
rg -n -C 10 'self\.src\s*=|src:|transcod|init_document|validate_utf8|UTF16|Vec<u8>|document_len' src/parsers/xml.rs | head -500
printf '%s\n' '--- XML API caller input construction and limits ---'
cat -n src/runtime/api/XMLObject.rs | sed -n '1,130p'
rg -n -C 8 'XMLObject|XML\.parse|xml_parse|parse_xml' src/runtime src/bundler --type rust | head -400
printf '%s\n' '--- Focused static invariant probe ---'
python3 - <<'PY'
from pathlib import Path
idx = Path("src/parsers/xml_index.rs").read_text()
xml = Path("src/parsers/xml.rs").read_text()
checks = {
"release_retained_size_gate": "if contents.len()" in idx or "assert!(contents.len()" in idx,
"debug_only_size_gate": "debug_assert!(contents.len() <= i32::MAX as usize)" in idx,
"u32_position_storage": "win: Vec<u32>" in idx,
"lossy_finish_cast": "self.contents.len() as u32" in idx,
"lossy_scalar_cast": "(base + i) as u32" in idx,
"xml_parse_calls_index_without_gate": "StructuralIndex::new(self.src)" in xml,
}
for name, value in checks.items():
print(f"{name}={value}")
PYRepository: oven-sh/bun
Length of output: 45163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Highway XML files and exact call definitions ---'
git ls-files | rg '(^|/)(highway|bun_highway)|highway_xml|xml.*highway' | head -100
rg -n -C 20 'xml_structural_index_chunk|base_offset|uint32_t|int32_t' src/jsc/bindings/highway_xml.cpp src --glob '*.{rs,cpp,cc,h,hpp,zig}' | rg -C 8 'xml_structural|base_offset|Highway|uint32_t|int32_t' | head -500
printf '%s\n' '--- Generated/in-tree binding declarations ---'
rg -n -C 10 'xml_structural_index_chunk' . --glob '!node_modules/**' --glob '!build/**' --glob '!dist/**' --glob '!target/**' | head -200
printf '%s\n' '--- Exact size-gate probe (excluding debug assertions) ---'
python3 - <<'PY'
from pathlib import Path
idx = Path("src/parsers/xml_index.rs").read_text()
xml = Path("src/parsers/xml.rs").read_text()
lines = idx.splitlines()
for n, line in enumerate(lines, 1):
if "contents.len()" in line or " as u32" in line or "StructuralIndex::new" in line:
print(f"xml_index.rs:{n}:{line}")
print("runtime_if_before_constructor=" + str(any(
line.lstrip().startswith(("if ", "assert!", "return Err")) and "contents.len()" in line
for line in lines[:48]
)))
print("xml_parse_result_has_size_error=" + str("DocumentTooLarge" in xml or "InputTooLarge" in xml))
PYRepository: oven-sh/bun
Length of output: 39060
Reject oversized XML documents before building StructuralIndex.
Scanner::build_index has no size gate. finish, scalar_chunk, and the Highway kernel cast positions to u32; inputs larger than u32::MAX therefore produce wrapped offsets. Replace the debug-only check with recoverable parser error handling, including after UTF-16 or Latin-1 transcoding.
🤖 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/xml_index.rs` around lines 35 - 48, Add a recoverable size
validation in Scanner::build_index before constructing StructuralIndex,
rejecting inputs larger than u32::MAX rather than relying on with_producer’s
debug_assert!. Apply the same validation after UTF-16 or Latin-1 transcoding,
and return the parser’s established error type so finish, scalar_chunk, and the
Highway kernel never receive oversized documents.
Source: Coding guidelines
| #[test] | ||
| fn simd_and_scalar_indexers_agree() { | ||
| let mut state = 0x9E3779B97F4A7C15u64; | ||
| let mut rng = move || { | ||
| state ^= state << 13; | ||
| state ^= state >> 7; | ||
| state ^= state << 17; | ||
| state | ||
| }; | ||
| let alphabet: &[u8] = b"<>&=\"' \t\n\rabc/?!-[]x01\x01\x1f\x0b\x80\xc3\xa9\xef\xbf\xbe"; | ||
| for _ in 0..20_000 { | ||
| let len = (rng() % 300) as usize; | ||
| let mut buf = Vec::with_capacity(len); | ||
| for _ in 0..len { | ||
| buf.push(alphabet[(rng() as usize) % alphabet.len()]); | ||
| } | ||
| let (si, ci) = build_both(&buf); | ||
| assert_eq!(si, ci, "index mismatch for {:?}", bstr::BStr::new(&buf)); | ||
| } | ||
| for pad in 40..=200usize { | ||
| let doc = format!( | ||
| "<r a=\"{}\">{}<b\n\tc='d'/>&e;\r\n</r>", | ||
| "v ".repeat(pad / 3), | ||
| "t\u{e9}\t".repeat(pad), | ||
| ); | ||
| let (si, ci) = build_both(doc.as_bytes()); | ||
| assert_eq!(si, ci, "mismatch for {doc:?}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make the SIMD/scalar agreement test prove that the SIMD path ran.
build_both builds the first index with StructuralIndex::new, which selects the scalar producer when bun_core::env::IS_NATIVE is false. On those targets this test compares the scalar indexer against itself and cannot fail for the intended reason. The same applies to streaming_and_scalar_indexers_agree_on_large_documents at Lines 248-270 and to the build_both call at Line 214.
Skip the comparison with a stated reason when the SIMD kernel is unavailable, so the test never passes vacuously.
♻️ Proposed change to `build_both`
fn build_both(contents: &[u8]) -> (Vec<u32>, Vec<u32>) {
+ // Off native targets `StructuralIndex::new` also picks the scalar producer, so the
+ // comparison would be vacuous. Callers skip instead of asserting scalar == scalar.
+ assert!(
+ bun_core::env::IS_NATIVE,
+ "build_both requires the Highway kernel; guard callers with IS_NATIVE"
+ );
let mut simd = StructuralIndex::new(contents);🤖 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/xml_index.rs` around lines 218 - 246, Update the test helpers and
agreement tests around build_both, simd_and_scalar_indexers_agree, and
streaming_and_scalar_indexers_agree_on_large_documents to detect when the SIMD
kernel is unavailable via bun_core::env::IS_NATIVE and skip with an explicit
reason instead of comparing scalar output against itself. Ensure all relevant
build_both call sites use this guard so the tests only run when SIMD execution
is actually available.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… the tape's encoding, arrays via MarkedArgumentBuffer + constructArray, no hand-rolled decoder; XML parses Latin-1 strings as is (Latin-1 tape) with a UTF-8 reparse only when a reference needs > U+00FF
No-Verification-Needed: docs only
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ast/e.rs`:
- Line 995: The JsonTape encoding path must not expose StrEncoding::Utf16 unless
its storage invariant is enforced. Keep UTF-16 disabled in src/ast/e.rs at
JsonTape.encoding, and in src/jsc/bindings/JSONRowsToJS.cpp lines 195-198 make
no direct Utf16 cast unless the byte range is guaranteed aligned and has an even
length; otherwise preserve UTF-8-only handling.
In `@test/js/bun/jsonc/jsonc.test.ts`:
- Around line 395-403: Extend the test case around JSONC.parse to exercise WTF-8
fallback by adding a JSON string containing a lone \uD800 or \uDC00 escape and
comparing the parsed result with JSON.parse. Replace the repeated long key and
value construction with Buffer.alloc(count, fill).toString(), while preserving
the existing key-order, prototype, and ownership assertions.
🪄 Autofix
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: 84dedcff-c4b8-451f-938b-b9707c6958e8
📒 Files selected for processing (8)
src/ast/e.rssrc/js_parser_jsc/expr_jsc.rssrc/jsc/bindings/JSONRowsToJS.cppsrc/parsers/error.rssrc/parsers/xml.rssrc/runtime/api.rssrc/runtime/api/XMLObject.rstest/js/bun/jsonc/jsonc.test.ts
| item_locs: Vec<crate::Loc, TapeAlloc>, | ||
| str_chunks: Vec<Vec<u8, TapeAlloc>, TapeAlloc>, | ||
| str_used: usize, | ||
| pub encoding: StrEncoding, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/ast/e.rs --match JsonTape --view expanded
rg -n -C 4 \
'str_chunks|StrEncoding::Utf16|encoding\s*=\s*.*Utf16|utf16\(const RowStr' \
src testRepository: oven-sh/bun
Length of output: 10272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- JsonTape and Str definitions ---'
sed -n '930,1145p' src/ast/e.rs
rg -n -C 8 \
'enum StrEncoding|struct Str\b|type Str\b|pub encoding: StrEncoding|encoding\s*[:=].*StrEncoding|StrEncoding::Utf16|RowEncoding::Utf16' \
src
printf '%s\n' '--- JSON row construction and tape consumers ---'
rg -n -C 10 \
'JSONRowsToJS|RowStr|str_chunks|alloc_str_join|alloc_str\(|\.encoding|encoding\(\)' \
src/jsc src/ast src/runtime testRepository: oven-sh/bun
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '930,1145p' src/ast/e.rs
rg -n -C 8 \
'enum StrEncoding|struct Str\b|type Str\b|pub encoding: StrEncoding|encoding\s*[:=].*StrEncoding|StrEncoding::Utf16|RowEncoding::Utf16' \
src
rg -n -C 10 \
'JSONRowsToJS|RowStr|str_chunks|alloc_str_join|alloc_str\(|\.encoding|encoding\(\)' \
src/jsc src/ast src/runtime testRepository: oven-sh/bun
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/ast/e.rs definitions and string allocation ---'
rg -n 'enum StrEncoding|struct Str\b|pub struct JsonTape|pub encoding: StrEncoding|StrEncoding::Utf16|encoding\s*=' src/ast/e.rs
sed -n '1,180p' src/ast/e.rs
sed -n '980,1120p' src/ast/e.rs
printf '%s\n' '--- exact JsonTape encoding mutations and consumers ---'
rg -n -C 5 'JsonTape|\.encoding\s*=|encoding: StrEncoding::Utf16|StrEncoding::Utf16' src --glob '*.rs' --glob '*.cpp' --glob '*.h' --glob '*.hpp' |
rg 'ast/e.rs|JsonTape|encoding|Utf16|JSONRows|RowStr|row'
printf '%s\n' '--- JSON row bridge ---'
sed -n '1,240p' src/jsc/bindings/JSONRowsToJS.cpp
rg -n -C 8 'JSONRowsToJS|RowStr|RowEncoding|str_chunks|raw_rows|alloc_str_join|alloc_str\(' src/ast src/jsc --glob '*.rs' --glob '*.cpp' --glob '*.h' --glob '*.hpp'Repository: oven-sh/bun
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'enum StrEncoding|struct Str\b|pub struct JsonTape|pub encoding: StrEncoding|StrEncoding::Utf16|encoding\s*=' src/ast/e.rs
sed -n '1,180p' src/ast/e.rs
sed -n '980,1120p' src/ast/e.rs
rg -n -C 5 'JsonTape|\.encoding\s*=|encoding: StrEncoding::Utf16|StrEncoding::Utf16' src --glob '*.rs' --glob '*.cpp' --glob '*.h' --glob '*.hpp' |
rg 'ast/e.rs|JsonTape|encoding|Utf16|JSONRows|RowStr|row'
sed -n '1,240p' src/jsc/bindings/JSONRowsToJS.cpp
rg -n -C 8 'JSONRowsToJS|RowStr|RowEncoding|str_chunks|raw_rows|alloc_str_join|alloc_str\(' src/ast src/jsc --glob '*.rs' --glob '*.cpp' --glob '*.h' --glob '*.hpp'Repository: oven-sh/bun
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- XML input encoding and tape output ---'
sed -n '1,125p' src/parsers/xml.rs
sed -n '1040,1110p' src/parsers/xml.rs
sed -n '2090,2265p' src/parsers/xml.rs
sed -n '2700,2760p' src/parsers/xml.rs
printf '%s\n' '--- all JsonTape encoding writes, restricted to parser/bridge crates ---'
rg -n -C 8 \
'encoding\s*=|encoding\s*:' \
src/parsers/json.rs src/parsers/json_stage2.rs src/parsers/xml.rs src/js_parser_jsc/expr_jsc.rs
printf '%s\n' '--- all alloc_str callers ---'
rg -n -C 6 'alloc_str_join|alloc_str\(' src/parsers src/js_parser_jsc src/ast --glob '*.rs'Repository: oven-sh/bun
Length of output: 33302
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,125p' src/parsers/xml.rs
sed -n '1040,1110p' src/parsers/xml.rs
sed -n '2090,2265p' src/parsers/xml.rs
sed -n '2700,2760p' src/parsers/xml.rs
rg -n -C 8 'encoding\s*=|encoding\s*:' \
src/parsers/json.rs src/parsers/json_stage2.rs src/parsers/xml.rs src/js_parser_jsc/expr_jsc.rs
rg -n -C 6 'alloc_str_join|alloc_str\(' \
src/parsers src/js_parser_jsc src/ast --glob '*.rs'Repository: oven-sh/bun
Length of output: 33156
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
for pattern in (
r'\bStrEncoding::Utf16\b',
r'\.encoding\s*=\s*[^;]+',
r'\bencoding\s*:\s*StrEncoding::Utf16\b',
):
print(f'--- {pattern} ---')
rx = re.compile(pattern)
for path in Path('src').rglob('*'):
if path.suffix not in {'.rs', '.cpp', '.h', '.hpp'}:
continue
try:
text = path.read_text()
except UnicodeDecodeError:
continue
for number, line in enumerate(text.splitlines(), 1):
if rx.search(line):
print(f'{path}:{number}:{line.strip()}')
PY
printf '%s\n' '--- XML UTF-16 conversion ---'
sed -n '880,1050p' src/parsers/xml.rs
printf '%s\n' '--- JSON/XML parser string shapes ---'
sed -n '470,545p' src/parsers/json_stage2.rs
sed -n '2460,2530p' src/parsers/xml.rsRepository: oven-sh/bun
Length of output: 15412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
for pattern in (
r'\bStrEncoding::Utf16\b',
r'\.encoding\s*=\s*[^;]+',
r'\bencoding\s*:\s*StrEncoding::Utf16\b',
):
print(f'--- {pattern} ---')
rx = re.compile(pattern)
for path in Path('src').rglob('*'):
if path.suffix not in {'.rs', '.cpp', '.h', '.hpp'}:
continue
try:
text = path.read_text()
except UnicodeDecodeError:
continue
for number, line in enumerate(text.splitlines(), 1):
if rx.search(line):
print(f'{path}:{number}:{line.strip()}')
PY
sed -n '880,1050p' src/parsers/xml.rs
sed -n '470,545p' src/parsers/json_stage2.rs
sed -n '2460,2530p' src/parsers/xml.rsRepository: oven-sh/bun
Length of output: 15344
Do not expose StrEncoding::Utf16 without a storage invariant.
No current parser sets JsonTape.encoding to Utf16; UTF-16 XML input is transcoded to UTF-8. However, alloc_str_join stores bytes at arbitrary offsets in Vec<u8>. If Utf16 is enabled, JSONRowsToJS.cpp can read an unaligned pointer and silently truncate odd lengths with len / 2. Keep this path disabled, or guarantee alignment and even lengths before the cast.
📍 Affects 2 files
src/ast/e.rs#L995-L995(this comment)src/jsc/bindings/JSONRowsToJS.cpp#L195-L198
🤖 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/ast/e.rs` at line 995, The JsonTape encoding path must not expose
StrEncoding::Utf16 unless its storage invariant is enforced. Keep UTF-16
disabled in src/ast/e.rs at JsonTape.encoding, and in
src/jsc/bindings/JSONRowsToJS.cpp lines 195-198 make no direct Utf16 cast unless
the byte range is guaranteed aligned and has an even length; otherwise preserve
UTF-8-only handling.
| test("Bun.JSONC.parse builds objects the way JSON.parse does: index keys first, __proto__ own, keys of every kind", () => { | ||
| const doc = `{"b":1,"0":2,"a":3,"__proto__":{"x":1},"ünï":4,"${"k".repeat(40)}":5,"1":6,"":7,"s":"","t":"x","u":"${"y".repeat(40)}","v":"ünï"}`; | ||
| const parsed = Bun.JSONC.parse(doc) as any; | ||
| const reference = JSON.parse(doc); | ||
| expect(parsed).toEqual(reference); | ||
| expect(Object.keys(parsed)).toEqual(Object.keys(reference)); | ||
| expect(Object.getPrototypeOf(parsed)).toBe(Object.prototype); | ||
| expect(Object.hasOwn(parsed, "__proto__")).toBe(true); | ||
| expect(Bun.JSONC.parse(`[[],[1,"a",{}],[[["deep"]]]]`)).toEqual([[], [1, "a", {}], [[["deep"]]]]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test the WTF-8 fallback path.
The current Unicode values are valid UTF-8. They do not call the new lone-surrogate conversion path. Add a \\uD800 or \\uDC00 JSON escape and compare it with JSON.parse. Use Buffer.alloc(count, fill).toString() for the long key and value.
Proposed test update
test("Bun.JSONC.parse builds objects the way JSON.parse does: index keys first, __proto__ own, keys of every kind", () => {
- const doc = `{"b":1,"0":2,"a":3,"__proto__":{"x":1},"ünï":4,"${"k".repeat(40)}":5,"1":6,"":7,"s":"","t":"x","u":"${"y".repeat(40)}","v":"ünï"}`;
+ const longKey = Buffer.alloc(40, "k").toString();
+ const longValue = Buffer.alloc(40, "y").toString();
+ const doc = `{"b":1,"0":2,"a":3,"__proto__":{"x":1},"ünï":4,"${longKey}":5,"1":6,"":7,"s":"","t":"x","u":"${longValue}","v":"ünï"}`;
const parsed = Bun.JSONC.parse(doc) as any;
const reference = JSON.parse(doc);
expect(parsed).toEqual(reference);
expect(Object.keys(parsed)).toEqual(Object.keys(reference));
expect(Object.getPrototypeOf(parsed)).toBe(Object.prototype);
expect(Object.hasOwn(parsed, "__proto__")).toBe(true);
+ expect(Bun.JSONC.parse(`{"lone":"\\uD800"}`)).toEqual(JSON.parse(`{"lone":"\\uD800"}`));
expect(Bun.JSONC.parse(`[[],[1,"a",{}],[[["deep"]]]]`)).toEqual([[], [1, "a", {}], [[["deep"]]]]);
});As per coding guidelines, “Tests must cover the complete relevant variant matrix” and repetitive strings must use Buffer.alloc(count, fill).toString().
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("Bun.JSONC.parse builds objects the way JSON.parse does: index keys first, __proto__ own, keys of every kind", () => { | |
| const doc = `{"b":1,"0":2,"a":3,"__proto__":{"x":1},"ünï":4,"${"k".repeat(40)}":5,"1":6,"":7,"s":"","t":"x","u":"${"y".repeat(40)}","v":"ünï"}`; | |
| const parsed = Bun.JSONC.parse(doc) as any; | |
| const reference = JSON.parse(doc); | |
| expect(parsed).toEqual(reference); | |
| expect(Object.keys(parsed)).toEqual(Object.keys(reference)); | |
| expect(Object.getPrototypeOf(parsed)).toBe(Object.prototype); | |
| expect(Object.hasOwn(parsed, "__proto__")).toBe(true); | |
| expect(Bun.JSONC.parse(`[[],[1,"a",{}],[[["deep"]]]]`)).toEqual([[], [1, "a", {}], [[["deep"]]]]); | |
| test("Bun.JSONC.parse builds objects the way JSON.parse does: index keys first, __proto__ own, keys of every kind", () => { | |
| const longKey = Buffer.alloc(40, "k").toString(); | |
| const longValue = Buffer.alloc(40, "y").toString(); | |
| const doc = `{"b":1,"0":2,"a":3,"__proto__":{"x":1},"ünï":4,"${longKey}":5,"1":6,"":7,"s":"","t":"x","u":"${longValue}","v":"ünï"}`; | |
| const parsed = Bun.JSONC.parse(doc) as any; | |
| const reference = JSON.parse(doc); | |
| expect(parsed).toEqual(reference); | |
| expect(Object.keys(parsed)).toEqual(Object.keys(reference)); | |
| expect(Object.getPrototypeOf(parsed)).toBe(Object.prototype); | |
| expect(Object.hasOwn(parsed, "__proto__")).toBe(true); | |
| expect(Bun.JSONC.parse(`{"lone":"\uD800"}`)).toEqual(JSON.parse(`{"lone":"\uD800"}`)); | |
| expect(Bun.JSONC.parse(`[[],[1,"a",{}],[[["deep"]]]]`)).toEqual([[], [1, "a", {}], [[["deep"]]]]); | |
| }); |
🤖 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 `@test/js/bun/jsonc/jsonc.test.ts` around lines 395 - 403, Extend the test case
around JSONC.parse to exercise WTF-8 fallback by adding a JSON string containing
a lone \uD800 or \uDC00 escape and comparing the parsed result with JSON.parse.
Replace the repeated long key and value construction with Buffer.alloc(count,
fill).toString(), while preserving the existing key-order, prototype, and
ownership assertions.
Source: Coding guidelines
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/runtime/xml.mdx`:
- Line 51: Update the parser description near the well-formedness claims to
qualify UTF-8 validation as applying to byte input only. Also document that
JavaScript string input bypasses UTF-8 validation and preserves its Latin-1 or
UTF-16 representation.
🪄 Autofix
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: 9dbca099-861f-47b2-9c13-7e3f25b622f9
📒 Files selected for processing (1)
docs/runtime/xml.mdx
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/parsers/benches/support/xml_old.rs:1-3—src/parsers/benches/support/xml_old.rs(3096 lines) was accidentally re-added in 19dc05c after you already deleted it in 072d131 ("bench: drop the accidentally added copy of the old parser") — nothing references it and it isn't compiled.git rm src/parsers/benches/support/xml_old.rsagain.Extended reasoning...
What the issue is
src/parsers/benches/support/xml_old.rsis a 3096-line verbatim copy of the pre-rewritexml.rsparser sitting in the bench support directory. It is completely unreferenced:rg xml_oldreturns zero hits anywhere in the repository, nomod xml_old;declaration or#[path]attribute pulls it in, andCargo.tomlhas no[[bench]]entry for it — so cargo never even compiles it. The active benchmark,benches/xml_parse.rs, callsbun_parsers::xml::XML::parse(the real parser) and does not import this file.The specific history that makes this a mistake, not a design choice
git log -- src/parsers/benches/support/xml_old.rsshows three commits:- 228880f — file first added (presumably to compare old-vs-new in the bench, then never wired up).
- 072d131 — "bench: drop the accidentally added copy of the old parser" — the author explicitly deleted it, acknowledging it was accidental.
- 19dc05c (HEAD) — the rows→JS commit re-adds all 3096 lines.
git show --stat 19dc05c8 | grep xml_oldconfirms+3096.
So the author already agreed this file should not exist, deleted it, and then accidentally re-staged it in the very next commit (likely a stray
git add -Awhile working on the rows→JS changes). The PR diff now shows it as a new file.Why nothing prevents this
Because the file is never compiled (no
moddeclaration, no[[bench]]entry), rustc's dead-code lints never see it. Public items also escape dead-code lints per REVIEW.md, which is why that document requires manual grep-based verification.Impact
No runtime impact — the file is inert source that never reaches the compiler. But it adds ~120 KB / 3096 lines of unmaintained, superseded parser code to the tree. Anyone grepping for XML parser symbols will hit two implementations; anyone reading the bench directory will wonder why there's a second parser that nothing uses.
Why this is flagged at normal severity
REVIEW.md's "Code style & idioms reviewers enforce" section says: "Delete dead code in the same PR that makes it dead (required scope — name the deletions in the description): superseded implementations … Do not keep empty files around … Delete dead code." The document's preamble states everything in it "has blocked merges." This file is exactly a "superseded implementation" — the old
xml.rsthat this PR rewrites. The repo rule raises this above a pure nit, and the author's own commit 072d131 confirms the intent.Step-by-step proof
wc -l src/parsers/benches/support/xml_old.rs→3096.rg xml_old(repo-wide, file contents) → no matches.git log --oneline -- src/parsers/benches/support/xml_old.rs→228880f4(add),072d1319(delete: "drop the accidentally added copy"),19dc05c8(re-add, HEAD).grep xml_old src/parsers/Cargo.toml src/parsers/benches/xml_parse.rs→ nothing; the only[[bench]]entries arejson_parseandxml_parse.- Therefore the file is uncompiled, unreferenced dead code that the author already deleted once in this PR.
Fix
git rm src/parsers/benches/support/xml_old.rs
…ural index are generic over the code unit (u8 for UTF-8 / Latin-1, u16 for UTF-16), so a string is parsed in place in whichever representation it has and yields strings in the same one (UTF-16 tape)
…aths No-Verification-Needed: docs only
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/runtime/xml.mdx`:
- Line 29: Update the parser description near the SIMD-pass explanation to
qualify that character data and attribute values avoid byte-at-a-time scanning
only during structural scanning; explicitly note that DTD/entity replacement
text still undergoes byte-level scanning and may affect those regions.
🪄 Autofix
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: f77c4b3c-e266-475f-a6c1-be1e4824d78b
📒 Files selected for processing (2)
docs/runtime/xml.mdxsrc/parsers/xml.rs
|
|
||
| ## Performance | ||
|
|
||
| The parser works in two stages, like Bun's JSON parser: a SIMD pass (runtime-dispatched AVX2/AVX-512/NEON/SVE kernels) records the position of every byte that can change the parse — `<`, `>`, `&`, line ends, quotes and `=` inside tags — and the parser then hops from one of those positions to the next, so character data, attribute values, comments and CDATA sections are never scanned a byte at a time. The result is written as flat rows and turned into JavaScript objects in a single pass that reuses JavaScriptCore's atom-string cache for element and attribute names, the same way `JSON.parse` does. A JS string is parsed in place in whatever representation the engine holds it in (Latin-1 or UTF-16) and the strings in the result share that representation, so nothing is transcoded on the way in or out; `Buffer` and `Blob` input is parsed as UTF-8. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Qualify the byte-scan claim for entity processing.
Line 29 says that character data and attribute values are never scanned a byte at a time. DTD and entity replacement text still use byte-level scanning. Entity replacement can occur in character data and attribute values. Limit this statement to structural scanning and document the exception.
Proposed wording
-... so character data, attribute values, comments and CDATA sections are never scanned a byte at a time.
+... so structural scanning of character data, attribute values, comments and CDATA sections avoids byte-at-a-time scans; DTD and entity replacement text still use byte-level scanning.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The parser works in two stages, like Bun's JSON parser: a SIMD pass (runtime-dispatched AVX2/AVX-512/NEON/SVE kernels) records the position of every byte that can change the parse — `<`, `>`, `&`, line ends, quotes and `=` inside tags — and the parser then hops from one of those positions to the next, so character data, attribute values, comments and CDATA sections are never scanned a byte at a time. The result is written as flat rows and turned into JavaScript objects in a single pass that reuses JavaScriptCore's atom-string cache for element and attribute names, the same way `JSON.parse` does. A JS string is parsed in place in whatever representation the engine holds it in (Latin-1 or UTF-16) and the strings in the result share that representation, so nothing is transcoded on the way in or out; `Buffer` and `Blob` input is parsed as UTF-8. | |
| The parser works in two stages, like Bun's JSON parser: a SIMD pass (runtime-dispatched AVX2/AVX-512/NEON/SVE kernels) records the position of every byte that can change the parse — `<`, `>`, `&`, line ends, quotes and `=` inside tags — and the parser then hops from one of those positions to the next, so structural scanning of character data, attribute values, comments and CDATA sections avoids byte-at-a-time scans; DTD and entity replacement text still use byte-level scanning. The result is written as flat rows and turned into JavaScript objects in a single pass that reuses JavaScriptCore's atom-string cache for element and attribute names, the same way `JSON.parse` does. A JS string is parsed in place in whatever representation the engine holds it in (Latin-1 or UTF-16) and the strings in the result share that representation, so nothing is transcoded on the way in or out; `Buffer` and `Blob` input is parsed as UTF-8. |
🤖 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 `@docs/runtime/xml.mdx` at line 29, Update the parser description near the
SIMD-pass explanation to qualify that character data and attribute values avoid
byte-at-a-time scanning only during structural scanning; explicitly note that
DTD/entity replacement text still undergoes byte-level scanning and may affect
those regions.
… in the bench script No-Verification-Needed: comments and dev script only
…s classified through the same LUT gated on a zero high byte, 0xFFFE/0xFFFF as the non-characters), sharing the block core with the byte kernel; agreement tests; baseline allowlist
…VBMI/VBMI2 on the AVX-512 targets that have them) No-Verification-Needed: CI allowlist only
… kernels No-Verification-Needed: CI allowlist only
… index producers, incl. across blocks; entity values checked too); leaf-element fast path (text then own end tag finishes without the content loop); cheap all-distinct filter before folding repeated child names; tests
There was a problem hiding this comment.
I reviewed the changes since my last pass (5c36d2f addressed the lone-surrogate regression, the xmlByteClass.ts inventory row, and earlier doc/bash nits) and the bug hunt found nothing new. Given the scope — a full rewrite of the XML parser plus a new Rust↔C++ FFI layout contract for JsonTape rows, a new Highway SIMD kernel, and thread-local arena recycling that now backs every Bun.{JSONC,TOML,YAML,JSON5,XML}.parse call — a human sign-off is still warranted.
Checked and ruled out: the #[repr(C, u32)] JsonValue layout vs. the C++ RowValue mirror (offset asserts match on both sides); MarkedArgumentBuffer overflow handling in array(); the UTF-16 lone-surrogate carry across 64-unit blocks in both the SIMD and scalar producers; tag_degraded reset on every markup </>//> consumer.
Extended reasoning...
Overview
This PR rewrites Bun.XML's parser as a two-stage SIMD-indexed scanner producing immutable JsonTape rows, and adds a one-call C++ rows→JS converter (JSONRowsToJS.cpp) shared with Bun.JSONC. It touches ~2,000 lines in src/parsers/xml.rs, adds src/parsers/xml_index.rs (431 lines), src/jsc/bindings/highway_xml.cpp (222 lines), src/jsc/bindings/JSONRowsToJS.cpp (229 lines), reshapes E::JsonValue/PropertyJSON/ObjectJSON/ArrayJSON to #[repr(C)] for direct C++ reads, makes the scanner generic over u8/u16 code units, switches element parsing from recursive to iterative with an explicit depth cap, and introduces a #[thread_local] recycled arena in with_text_format_source_encoded that now backs all five text-format parse() host functions. Plus benches, docs, allowlists, and codegen wiring.
Security risks
The XML parser handles adversarial input by design. Entity-expansion limits (MAX_AMPLIFICATION, MAX_ENTITY_DEPTH, MAX_DEPTH) are preserved or added. The new FFI surface reads Rust-owned tape memory from C++ via raw pointers with layout mirrored by static_assert/const _: () = assert! on both sides — a mismatch would be memory corruption, not a compile error on one side alone. The SIMD kernel writes into a caller-sized MaybeUninit buffer with an assert!(out.len() >= chunk.len() + 64) guard. No auth/crypto/permissions surface.
Level of scrutiny
High. This is a large architectural rewrite of a user-facing API with new unsafe blocks (set_len after kernel writes, get_unchecked in the hot at/seek path, raw NonNull<JsonTape> writes, transmute_copy in unit_from_u16), a new cross-language struct-layout contract, GC integration via MarkedArgumentBuffer/putDirect, and a behavioural change to arena lifetime for four other parsers. The repo's review guidance singles out exactly these categories (FFI layout, SIMD kernels, GC rooting, per-thread state) as the most-blocked.
Other factors
Test coverage is substantial (1995 W3C conformance cases unchanged, new SIMD-vs-scalar agreement tests over random and block-straddling input for both unit widths, new lone-surrogate rejection tests across content/attributes/entity-values/after-root, new >-in-attribute-value tests for the tag_degraded path, a JSONC object-shape parity test). All prior inline findings from earlier passes (surrogate acceptance regression, bash 3.2 empty-array expansion, doc overstatements, stale Sink/end_attributes references, missing CLAUDE.md inventory row) have been addressed with confirming replies. Two CodeRabbit nits remain open (WTF-8 fallback test coverage, doc byte-scan qualifier) but neither is load-bearing. The scale and the FFI/SIMD/GC surface put this outside what an automated approval should sign off on.
…UnderlyingString has no Drop; hold it in StringOrBuffer as before) — LSAN caught it via resolve/jsonc.test.ts
There was a problem hiding this comment.
I reviewed this PR and the automated pass found no bugs. Given the scope — a full XML parser rewrite with a new SIMD stage-1 kernel, a new Rust↔C++ ABI surface (JSONRowsToJS.cpp reading #[repr(C)] tape rows), encoding-generic scanning over u8/u16, and the thread-local arena recycling in the text-format scaffold — a human sign-off is still warranted.
What was reviewed
- Rust/C++ layout parity:
offset_of!/static_assertpairs onRowValue/RowProperty/RowSpanline up;StrEncodingdiscriminants match. RowsToJS: exception scopes after every fallible call,MarkedArgumentBufferoverflow check,isSafeToRecurseguard on both object/array recursion.- UTF-16 lone-surrogate rejection: both index producers (SIMD and scalar) flag unpaired surrogates including block-straddling leads and end-of-input; covered by new tests.
tag_degradedbookkeeping around literal>and the Latin-1NeedsWiderEncodingretry path inXMLObject.rs.
Extended reasoning...
Overview
This PR rewrites Bun.XML's parser to mirror the JSON parser's two-stage design: a Highway SIMD structural indexer (highway_xml.cpp, xml_index.rs) plus an index-hopping stage-2 scanner, writing immutable rows onto an E::JsonTape instead of building E::Object/E::Array per node. Rows are converted to JS in one C++ call (JSONRowsToJS.cpp) that mirrors the #[repr(C)] layouts of JsonValue/PropertyJSON/ObjectJSON/ArrayJSON and routes keys through jsonAtomStringCache. The scanner is now generic over u8/u16 code units, enabling Latin-1 and UTF-16 JS strings to be parsed in place; the text-format scaffold in api.rs gains a SourceEncoding enum, a string_passthrough mode, and a per-thread recycled Arena. Supporting changes: codegen for the XML byte-class tables, verify-baseline allowlist updates, benchmark harnesses (Rust criterion + JS mitata), and docs.
Security risks
The parser handles untrusted input and enforces well-formedness limits (MAX_DEPTH, entity amplification, i32::MAX byte-length guard). The new UTF-16 path had a lone-surrogate acceptance gap that was caught and fixed during review (5c36d2f) with SIMD/scalar agreement tests. The rows→JS bridge reads Rust struct layouts by offset from C++; both sides carry matching static_assert/const _: () = assert! checks, so a silent ABI drift would fail to compile rather than corrupt memory. No auth/crypto/permissions surface is touched.
Level of scrutiny
High. This is a ~4K-line architectural rewrite of a hot-path parser with a new cross-language ABI, new SIMD kernels dispatched at runtime across five x64 targets plus SVE, three encoding paths, and a change to arena lifetime management shared by all text-format .parse host functions. It is well beyond the "simple/mechanical" bar for auto-approval.
Other factors
A maintainer has been actively reviewing (the MarkedArgumentBuffer and duplicate-decoder feedback were both addressed), and every earlier automated finding — including the lone-surrogate regression — has a fix commit and tests. The 1995-case W3C conformance suite, SIMD-vs-scalar index agreement tests, and jsonc/toml/yaml suites are cited as passing. Two minor CodeRabbit doc/test-coverage suggestions remain open but are non-blocking. Deferring rather than approving.
…-sh#37146) ### What does this PR do? Rewrites `Bun.XML`'s parser in the shape of the JSON parser: a Highway SIMD "stage 1" indexes every `<` `>` `&` `\r`, control character and encoded U+FFFE/FFFF (plus tab/newline/quotes/`=` inside tags, tracked with a MatchStar carry across 64-byte blocks), and stage 2 hops from index entry to entry — text runs, attribute values, comments, CDATA and PIs are never walked byte by byte — writing immutable `ObjectJSON`/`ArrayJSON` rows on a `JsonTape` instead of a `Vec` per object. Elements are parsed iteratively (explicit depth cap); the DTD and entity replacement text keep the byte-level scanner. Rows (JSON and XML alike) become JS values in one C++ call: objects via `putDirect`, arrays via `MarkedArgumentBuffer` + `constructArray`, keys and short values through the VM's `jsonAtomStringCache` as `JSON.parse` does, with three string paths keyed on the tape's encoding (UTF-8 / Latin-1 / UTF-16). An 8-bit JS string is parsed byte-per-character as is and yields Latin-1 strings without any re-encoding. `Bun.{XML,JSONC,TOML,YAML,JSON5}.parse` recycle a per-thread arena instead of a `mi_heap` per call. **JS level** — `bench/xml/xml.mjs` with the CI-built PR binary (`bunx bun-pr 37146`, linux-x64) vs main, ms/iter, lower is better (one loaded machine → read as ratios). Input is a JS string: ASCII files take the Latin-1 in-place path, the CJK-heavy one the native UTF-16 path. | document | PR (compact) | main | × | PR `{compact:false}` | main | txml | fast-xml-parser | @xmldom | |---|---|---|---|---|---|---|---|---| | small, 188 B | **0.0028** | 0.0149 | 5.4× | 0.0048 | 0.0168 | 0.0079 | 0.029 | 0.032 | | S3 ListObjects, 231 KB | **1.33** | 3.17 | 2.4× | 3.97 | 8.54 | 4.79 | 24.8 | 33.6 | | Atom feed, 193 KB | **1.10** | 3.25 | 3.0× | 2.56 | 5.93 | 4.57 | 20.8 | 26.2 | | SVG (LibreOffice art), 1.1 MB | **0.55** | 4.12 | 7.5× | 0.53 | 3.93 | – | 5.34 | 4.10 | | SVG (hixie perf 007), 388 KB | **0.34** | 1.62 | 4.8× | 0.38 | 1.77 | 0.29 | 2.89 | 2.33 | | Vulkan vk.xml, 3.1 MB | **23.1** | 61.9 | 2.7× | 47.6 | 109 | 65.5 | 304 | 311 | | RFC xml, 363 KB | **1.85** | 4.19 | 2.3× | 3.37 | 8.20 | 2.98 | 32.1 | 40.4 | | libphonenumber, 959 KB | **4.97** | 11.4 | 2.3× | 9.56 | 18.9 | 11.8 | 62.5 | 60.8 | | chromium enums.xml, 1.4 MB | **16.6** | 27.3 | 1.6× | 27.9 | 47.7 | 47.2 | 153 | 112 | | freedesktop mime db, 2.2 MB (UTF-16) | **26.7** | 60.2 | 2.3× | 43.4 | 94.4 | 60.0 | 317 | 303 | **Native level** — `scripts/bench-json-rust.sh --xml` (criterion; parse → compact tree), MiB/s, higher is better: | file | bun new | bun old | pugixml | quick-xml | expat | roxmltree | libxml2 | xml-rs | |---|---|---|---|---|---|---|---|---| | SVG (LibreOffice art) 1.2 MB | **3634** | 341 | 2177 | 964 | 174 | 294 | 630 | 54 | | SVG (hixie perf 007) 400 KB | **2348** | 300 | 2239 | 855 | 264 | 282 | 422 | 50 | | libphonenumber metadata 1 MB | 438 | 161 | 836 | 577 | 171 | 175 | 88 | 37 | | RFC xml (opus draft) 370 KB | 344 | 124 | 857 | 337 | 184 | – | 12 | – | | Vulkan vk.xml 3.2 MB | 339 | 98 | 627 | 239 | 132 | 108 | 41 | – | | chromium enums.xml 1.4 MB | 322 | 95 | 703 | 275 | 114 | 106 | 35 | 28 | | freedesktop mime db 2.4 MB | 207 | 60 | 585 | 237 | 119 | 91 | 30 | – | | synthetic dense records 1 MB | 162 | 37 | 583 | 132 | 86 | 82 | 22 | 15 | So: 3–10× the parser on main, ahead of every Rust parser and expat/libxml2 on all files, ahead of pugixml on attribute/text-heavy input, still behind pugixml (which skips UTF-8 validation and duplicate-attribute checks and parses in-situ) on element-dense input; stage 1 alone runs at 2–3 GiB/s everywhere. ### How did you verify your code works? `bun bd test test/js/bun/xml` (1995/1995 W3C conformance cases with identical messages, 56 unit tests incl. new ones for `>` inside attribute values and key kinds), `test/js/bun/jsonc` (incl. a new object-shape parity test against `JSON.parse`), `test/js/bun/toml`, yaml, `bundler_loader`, `bun-lock`; SIMD-vs-scalar index agreement unit tests; `bun run rust:check-all`. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… scraping (#37194) ### What does this PR do? The S3 client extracted `ListObjectsV2` results, `<Error>` bodies (simple requests and download streams) and the multipart `UploadId` with `index_of("<Tag>")` scans. That returned XML-escaped text verbatim — S3 escapes every text node, so a key `Tom & Jerry.mp4` came back with the entity still in it, and the same for prefixes, continuation tokens and error messages — could be confused by markup-looking text, and half-parsed ill-formed bodies. Now that #37146 landed a fast conforming parser, these paths read the response through `bun_parsers::xml` (node shape, so text is byte-exact: entities/CDATA decoded, whitespace in keys preserved) via a small `s3/xml_response.rs` helper. `list_objects.rs` shrinks from a ~350-line state machine to field mapping. Behavior changes worth calling out: - Escaped text is decoded everywhere (the actual bug). - A `200` ListObjectsV2 body that is not a well-formed `<ListBucketResult>` now **rejects** with `code: "InvalidResponse"` (message points at `encodingType: "url"` for keys with control characters) instead of resolving with a partial/empty listing. The existing tests that answered `<>` just to inspect the request URL now answer `<ListBucketResult/>`. - 404s whose `<Error>` has no `<Code>` still map to `NoSuchKey`. Also folded in: the per-thread recycled parse arena is factored out of the `Bun.{XML,JSONC,TOML,YAML,JSON5}.parse` scaffold as `RecycledArena` and shared with the S3 path (no `mi_heap` per response), and the `xml_parse` feature counter moves from the parser to its API entry points so internal use isn't counted. Not in scope: `uploadId` is still interpolated into the query string unencoded (pre-existing; AWS upload IDs are URL-safe and are validated as printable ASCII). ### How did you verify your code works? - New test in `test/js/bun/s3/s3-list-objects.test.ts` covering escaped keys/prefixes/owner, CDATA keys, `&#x…;` refs, `RestoreStatus` nesting, whitespace-exact keys, and decoded `<Error>` code/message on 404 — fails with `USE_SYSTEM_BUN=1`, passes on the build. - `bun bd test` green for `s3-list-objects` (36 pass / 3 skip), `s3-storage-class`, `s3-list-checksum-algorithm`, `s3-list-encode-overflow`, `s3-stream-error-gc`, `s3-requester-pays`, `s3-connection-close`, and the multipart upload-id / NetworkSink mock tests in `s3.test.ts`; XML suites unchanged. - Drove the debug binary against a local mock endpoint for all four paths (list, error body, 404 without `<Code>`, malformed 200, multipart create→complete). - The credentialed `s3.test.ts` suites are skipped locally (no secrets); CI runs them. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
… scraping (oven-sh#37194) ### What does this PR do? The S3 client extracted `ListObjectsV2` results, `<Error>` bodies (simple requests and download streams) and the multipart `UploadId` with `index_of("<Tag>")` scans. That returned XML-escaped text verbatim — S3 escapes every text node, so a key `Tom & Jerry.mp4` came back with the entity still in it, and the same for prefixes, continuation tokens and error messages — could be confused by markup-looking text, and half-parsed ill-formed bodies. Now that oven-sh#37146 landed a fast conforming parser, these paths read the response through `bun_parsers::xml` (node shape, so text is byte-exact: entities/CDATA decoded, whitespace in keys preserved) via a small `s3/xml_response.rs` helper. `list_objects.rs` shrinks from a ~350-line state machine to field mapping. Behavior changes worth calling out: - Escaped text is decoded everywhere (the actual bug). - A `200` ListObjectsV2 body that is not a well-formed `<ListBucketResult>` now **rejects** with `code: "InvalidResponse"` (message points at `encodingType: "url"` for keys with control characters) instead of resolving with a partial/empty listing. The existing tests that answered `<>` just to inspect the request URL now answer `<ListBucketResult/>`. - 404s whose `<Error>` has no `<Code>` still map to `NoSuchKey`. Also folded in: the per-thread recycled parse arena is factored out of the `Bun.{XML,JSONC,TOML,YAML,JSON5}.parse` scaffold as `RecycledArena` and shared with the S3 path (no `mi_heap` per response), and the `xml_parse` feature counter moves from the parser to its API entry points so internal use isn't counted. Not in scope: `uploadId` is still interpolated into the query string unencoded (pre-existing; AWS upload IDs are URL-safe and are validated as printable ASCII). ### How did you verify your code works? - New test in `test/js/bun/s3/s3-list-objects.test.ts` covering escaped keys/prefixes/owner, CDATA keys, `&#x…;` refs, `RestoreStatus` nesting, whitespace-exact keys, and decoded `<Error>` code/message on 404 — fails with `USE_SYSTEM_BUN=1`, passes on the build. - `bun bd test` green for `s3-list-objects` (36 pass / 3 skip), `s3-storage-class`, `s3-list-checksum-algorithm`, `s3-list-encode-overflow`, `s3-stream-error-gc`, `s3-requester-pays`, `s3-connection-close`, and the multipart upload-id / NetworkSink mock tests in `s3.test.ts`; XML suites unchanged. - Drove the debug binary against a local mock endpoint for all four paths (list, error body, 404 without `<Code>`, malformed 200, multipart create→complete). - The credentialed `s3.test.ts` suites are skipped locally (no secrets); CI runs them. --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
2 participants
What does this PR do?
Rewrites
Bun.XML's parser in the shape of the JSON parser: a Highway SIMD "stage 1" indexes every<>&\r, control character and encoded U+FFFE/FFFF (plus tab/newline/quotes/=inside tags, tracked with a MatchStar carry across 64-byte blocks), and stage 2 hops from index entry to entry — text runs, attribute values, comments, CDATA and PIs are never walked byte by byte — writing immutableObjectJSON/ArrayJSONrows on aJsonTapeinstead of aVecper object. Elements are parsed iteratively (explicit depth cap); the DTD and entity replacement text keep the byte-level scanner.Rows (JSON and XML alike) become JS values in one C++ call: objects via
putDirect, arrays viaMarkedArgumentBuffer+constructArray, keys and short values through the VM'sjsonAtomStringCacheasJSON.parsedoes, with three string paths keyed on the tape's encoding (UTF-8 / Latin-1 / UTF-16). An 8-bit JS string is parsed byte-per-character as is and yields Latin-1 strings without any re-encoding.Bun.{XML,JSONC,TOML,YAML,JSON5}.parserecycle a per-thread arena instead of ami_heapper call.JS level —
bench/xml/xml.mjswith the CI-built PR binary (bunx bun-pr 37146, linux-x64) vs main, ms/iter, lower is better (one loaded machine → read as ratios). Input is a JS string: ASCII files take the Latin-1 in-place path, the CJK-heavy one the native UTF-16 path.{compact:false}Native level —
scripts/bench-json-rust.sh --xml(criterion; parse → compact tree), MiB/s, higher is better:So: 3–10× the parser on main, ahead of every Rust parser and expat/libxml2 on all files, ahead of pugixml on attribute/text-heavy input, still behind pugixml (which skips UTF-8 validation and duplicate-attribute checks and parses in-situ) on element-dense input; stage 1 alone runs at 2–3 GiB/s everywhere.
How did you verify your code works?
bun bd test test/js/bun/xml(1995/1995 W3C conformance cases with identical messages, 56 unit tests incl. new ones for>inside attribute values and key kinds),test/js/bun/jsonc(incl. a new object-shape parity test againstJSON.parse),test/js/bun/toml, yaml,bundler_loader,bun-lock; SIMD-vs-scalar index agreement unit tests;bun run rust:check-all.