Repository navigation
Conversation
… point fits in Latin-1
UTF-8 decode (Buffer#toString("utf8"), Blob/Response #text/#json,
BunString::clone_utf8) previously had two output tiers: all-ASCII stayed
8-bit, anything else was widened to a 16-bit StringImpl. French, German,
Spanish, Portuguese text (every code point <= U+00FF) paid 2 bytes/char
where 1 would do and lost every JSC 8-bit fast path downstream:
compare/hash, indexOf, the YARR Latin-1 specialisations,
JSON.stringify's 8-bit writer, and re-encode on the way back out.
Add a third tier. A new Highway SIMD classifier
(highway_classify_utf8_width) scans the input once, packing the
first-non-ASCII index and one of {Ascii, Latin1Candidate, HasWide} into
a single scalar return, branching on whether any byte >= 0x80 and
whether any byte >= 0xC4 (in valid UTF-8 every code point <= U+00FF
encodes to bytes < 0xC4). to_latin1_or_utf16_alloc then dispatches:
Ascii caller keeps the bytes.
HasWide existing simdutf UTF-8 -> UTF-16 path, unchanged.
Latin1Candidate same UTF-16 decode, then highway_copy_u16_to_u8 to a
fresh Vec<u8>. A decode error (overlong, truncated,
stray continuation; all pass the <0xC4 gate) falls
through to the UTF-16 replacement path so U+FFFD
placement and fatal-mode behaviour are byte-identical.
simdutf's own convert_utf8_to_latin1* is not used: its icelake kernel
passes __m512i by value to a helper that does not inline in the
debug-ASAN WebKit build and faults on vmovdqa64 stack alignment.
Migrated the string-producing call sites (encoding.rs owned+borrowed
UTF-8 arms, Blob to_string/to_json/Internal::to_string_owned). The
existing to_utf16_alloc signature is kept as a thin wrapper for callers
that need a UTF-16 buffer regardless of representability.
BunString__fromUTF8 gains the same narrowing via WTF's
charactersAreAllLatin1 + copyCharacters. The streaming
to_utf16_alloc_maybe_buffered path (TextDecoder) is deliberately left
for a follow-up: it carries split-sequence state that a Latin-1 arm
would have to duplicate.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// Result of [`to_latin1_or_utf16_alloc`]: the narrowest representation the | ||
| /// decoded code points fit in. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Every decoded code point is `<= U+00FF`; buffer is one Latin-1 byte per | ||
| /// code point. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// At least one code point `> U+00FF` (or a U+FFFD replacement was | ||
| /// emitted); buffer is UTF-16LE. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Decode UTF-8 into the narrowest JSC string representation: [`Utf8Decoded::Ascii`] | ||
| /// when every byte is ASCII, [`Utf8Decoded::Latin1`] when every decoded code | ||
| /// point is `<= U+00FF`, else [`Utf8Decoded::Utf16`]. Error semantics match | ||
| /// [`to_utf16_alloc`]: with `fail_if_invalid` set invalid UTF-8 yields | ||
| /// `Err(InvalidByteSequence)`; otherwise invalid sequences are replaced with | ||
| /// U+FFFD and the result is always `Utf16` (U+FFFD is not Latin-1). | ||
| /// | ||
| /// Implementation: a single Highway SIMD pass over the input bytes | ||
| /// ([`bun_highway::classify_utf8_width`]) decides the output shape up front. | ||
| /// `HasWide` runs the existing simdutf UTF-16 decode unchanged. | ||
| /// `Latin1Candidate` runs the same decode and, on success, narrows the code | ||
| /// units to one byte each via `highway_copy_u16_to_u8` (valid UTF-8 with every | ||
| /// byte `< 0xC4` is exactly the set of code points `<= U+00FF`, so no | ||
| /// post-decode scan is needed); a decode error means invalid UTF-8, and the | ||
| /// replacement path yields UTF-16. simdutf's own `convert_utf8_to_latin1*` is | ||
| /// avoided because its icelake kernel passes `__m512i` by value to a helper | ||
| /// that does not inline in the debug-ASAN WebKit build and faults on stack | ||
| /// alignment. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// | ||
| /// Callers that build a JS string from the result should prefer | ||
| /// [`to_latin1_or_utf16_alloc`], which adds a Latin-1 tier so JSC's 8-bit fast | ||
| /// paths stay available for accented-Latin text. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// [`to_utf16_alloc`] for input already known to contain a non-ASCII byte at | ||
| /// `first`. Also reports whether simdutf accepted the input as valid UTF-8 | ||
| /// (callers use this to decide whether a Latin-1 narrowing is sound without | ||
| /// re-scanning the output). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Some byte `>= 0x80` but all `< 0xC4`: when the input is well-formed | ||
| /// UTF-8 every code point is `<= U+00FF`. Invalid sequences (overlongs, | ||
| /// strays, truncated tails) can still land here, so callers must validate. | ||
| /// Carries the index of the first byte `>= 0x80`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// One-pass classifier for [`Utf8Width`]. SIMD-wide; the ASCII prefix is | ||
| /// scanned at one compare per block, then the tail at one compare per block | ||
| /// with an early exit on the first byte `>= 0xC4`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Raw-pointer variant of [`copy_u16_to_u8`] for writing into uninitialised | ||
| /// storage. | ||
| /// | ||
| /// # Safety | ||
| /// `input` must be valid for `count` reads and `output` for `count` writes, | ||
| /// non-overlapping. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Narrow to an 8-bit StringImpl when every decoded code unit is | ||
| // <= U+00FF so downstream code keeps JSC's 8-bit fast paths. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // One-pass classifier for UTF-8 decode. The low two bits of the return value | ||
| // are the class: 0 = every byte < 0x80 (ASCII), 1 = some byte >= 0x80 but all | ||
| // < 0xC4 (every well-formed code point fits in Latin-1), 2 = some byte >= 0xC4. | ||
| // The remaining bits carry the index of the first byte >= 0x80 (or search_len | ||
| // when the class is 0). Packed into a single scalar return so the FFI boundary | ||
| // keeps everything in registers. Phase 1 scans ASCII-only blocks at one | ||
| // compare per block; phase 2 continues from the first non-ASCII block at one | ||
| // compare per block and early-exits on the first byte >= 0xC4. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // Ownership of the UTF-16 buffer transfers to JSC's external-string | ||
| // finalizer (which calls back into the default allocator's `free`). | ||
| // `into_raw` is the explicit ownership-transfer-to-FFI API; the | ||
| // matching free lives on the C++ side. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
Updated 11:57 PM PT - Jul 31st, 2026
❌ @autofix-ci[bot], your commit d2a2eda has 1 failures in
The baseline build contains instructions not available on Static scan violations |
| /// Build a Latin-1 `BunString` from an owned buffer, routing small outputs | ||
| /// through WTF's string allocator and large ones through an external | ||
| /// `StringImpl` (same size policy as [`encode_base64_to_bun_string`]). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
terrible code. do not allocate utf16 and then allocate u8. only allocate once. update all callers. do not be lazy
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // keeps everything in registers. Phase 1 scans ASCII-only blocks at one | ||
| // compare per block; phase 2 continues from the first non-ASCII block at one | ||
| // compare per block and early-exits on the first byte >= 0xC4. | ||
| size_t ClassifyUTF8WidthImpl(const uint8_t* HWY_RESTRICT start_ptr, size_t search_len) |
There was a problem hiding this comment.
🔴 The new Highway kernel ClassifyUTF8WidthImpl is compiled once per SIMD target (AVX3/SVE/etc.) via HWY_EXPORT, but it's not listed in scripts/verify-baseline-static/allowlist-{x64,x64-windows,aarch64}.txt — CI's static instruction scan is already failing on this PR (_ZN3bun10N_SVE2_12821ClassifyUTF8WidthImplEPKhm [SVE] on aarch64, _ZN3bun10N_AVX3_SPR21ClassifyUTF8WidthImplEPKhm [AVX, AVX512BW, AVX512F] on x64). The kernel is properly runtime-gated by HWY_DYNAMIC_DISPATCH, so the fix is to add the per-target mangled symbols to all three allowlist files alongside the existing highway_strings.cpp entries (e.g. next to IndexOfNewlineOrNonASCIIImpl).
Extended reasoning...
What the bug is
src/jsc/bindings/highway_strings.cpp gains a new SIMD kernel ClassifyUTF8WidthImpl that is exported via HWY_EXPORT(ClassifyUTF8WidthImpl) and dispatched via HWY_DYNAMIC_DISPATCH. Highway's foreach_target.h mechanism compiles this function once per enabled ISA target — on x64 that's N_AVX2, N_AVX3, N_AVX3_DL, N_AVX3_ZEN4, N_AVX3_SPR, N_AVX10_2; on aarch64 that's N_SVE, N_SVE2, N_SVE_256, N_SVE2_128. Each of those per-target compilations emits post-baseline instructions (AVX-512, SVE) into the binary.
Bun's CI runs a static instruction scanner (scripts/verify-baseline-static/) that objdumps the release binary and flags any function containing instructions above the baseline CPU (Nehalem on x64, Cortex-A53 on aarch64). Runtime-dispatched kernels are exempted by listing their mangled symbols in allowlist-{x64,x64-windows,aarch64}.txt. Every other kernel in highway_strings.cpp — IndexOfNewlineOrNonASCIIImpl, FirstNonAscii8Impl, CopyU16ToU8Impl, etc. — has entries in all three files. ClassifyUTF8WidthImpl does not (grep of scripts/verify-baseline-static/ returns zero hits), and the PR's changed-files list doesn't include any allowlist file.
How it manifests
The robobun CI comment on this PR already reports the failure:
- Linux aarch64:
_ZN3bun10N_SVE2_12821ClassifyUTF8WidthImplEPKhm [SVE] (11 insns) - Linux x64:
_ZN3bun10N_AVX3_SPR21ClassifyUTF8WidthImplEPKhm [AVX, AVX512BW, AVX512F] (12 insns)
with the scanner's own guidance: "If these are runtime-dispatched behind a CPUID gate: add each symbol to scripts/verify-baseline-static/allowlist-<arch>.txt with a comment pointing at the gate."
Step-by-step proof
highway_strings.cpp:2218addsHWY_EXPORT(ClassifyUTF8WidthImpl);in theHWY_ONCEblock.- Highway's
foreach_target.hre-includes the TU once per target; each pass compilesbun::N_<TARGET>::ClassifyUTF8WidthImplwith that target's-mavx512bw/+sveflags. - The scanner disassembles the release binary, finds e.g.
vpcmpub/whileloinside_ZN3bun10N_AVX3_SPR21ClassifyUTF8WidthImplEPKhm, looks up the symbol inallowlist-x64.txt, finds no match, and fails the job. - Sibling kernel
IndexOfNewlineOrNonASCIIImpl(nearly identical structure — oneGtcompare per block) has 6 entries inallowlist-x64.txt(lines 492, 529, 566, 604, 637, 674), 4 inallowlist-aarch64.txt, and 6 inallowlist-x64-windows.txt.ClassifyUTF8WidthImplhas 0 in any of them.
Why existing code doesn't prevent it
There is no wildcard or namespace-level allowlisting — each mangled symbol must be listed individually. The scanner's message explicitly says so.
Impact
This is a hard CI failure on linux-x64, linux-x64-musl, linux-aarch64, linux-aarch64-musl, and windows-x64. The PR cannot merge until the allowlists are updated. There is no runtime correctness issue — the kernel is properly gated behind HWY_DYNAMIC_DISPATCH → hwy::SupportedTargets(), so a baseline CPU would never execute the AVX-512/SVE variants — but the static scanner cannot know that without the allowlist entry.
How to fix
Add per-target entries for ClassifyUTF8WidthImpl to all three allowlist files, mirroring the existing IndexOfNewlineOrNonASCIIImpl entries (same targets, same feature-set brackets — the kernel uses the same Gt/AllFalse/FindFirstTrue primitives). For example in allowlist-x64.txt:
_ZN3bun10N_AVX3_SPR21ClassifyUTF8WidthImplEPKhm [AVX, AVX512BW, AVX512F]
_ZN3bun11N_AVX3_ZEN421ClassifyUTF8WidthImplEPKhm [AVX, AVX512BW, AVX512F]
_ZN3bun6N_AVX221ClassifyUTF8WidthImplEPKhm [AVX, AVX2]
_ZN3bun6N_AVX321ClassifyUTF8WidthImplEPKhm [AVX, AVX512BW, AVX512F]
_ZN3bun9N_AVX10_221ClassifyUTF8WidthImplEPKhm [AVX, AVX512BW, AVX512F]
_ZN3bun9N_AVX3_DL21ClassifyUTF8WidthImplEPKhm [AVX, AVX512BW, AVX512F]
in allowlist-aarch64.txt:
_ZN3bun10N_SVE2_12821ClassifyUTF8WidthImplEPKhm [SVE]
_ZN3bun5N_SVE21ClassifyUTF8WidthImplEPKhm [SVE]
_ZN3bun6N_SVE221ClassifyUTF8WidthImplEPKhm [SVE]
_ZN3bun9N_SVE_25621ClassifyUTF8WidthImplEPKhm [SVE]
and the demangled equivalents (bun::N_AVX3_SPR::ClassifyUTF8WidthImpl, etc.) in allowlist-x64-windows.txt. The exact feature-set brackets are whatever the scanner reports on the next CI run; the safest approach is to copy the brackets from IndexOfNewlineOrNonASCIIImpl (same primitives) and let CI confirm.
This branch has not been deployed
2 participants
What
UTF-8 decode (
Buffer#toString('utf8'),Blob/Response#text()/#json(),BunString::clone_utf8) previously had two output tiers: pure ASCII stayed 8-bit, anything else was widened to a 16-bitStringImpl. Accented-Latin text (café,résumé,naïve; every code point <= U+00FF) paid 2 bytes/char where 1 would do and lost every JSC 8-bit fast path downstream (compare/hash,indexOf, YARR Latin-1 matchers,JSON.stringify's 8-bit writer, re-encode on socket write).This adds a third tier so those inputs produce an 8-bit
StringImpl.How
A new Highway SIMD scanner
highway_classify_utf8_widthdoes one pass over the input and returns(first_non_ascii_index << 2) | classin a single register:< 0x80>= 0x80but none>= 0xC4(in valid UTF-8, the exact set of strings whose every code point is<= U+00FF, sinceU+00FF = C3 BFandU+0100 = C4 80)>= 0xC4strings::to_latin1_or_utf16_allocthen dispatches:AsciiHasWideLatin1Candidatehighway_copy_u16_to_u8into a freshVec<u8>. On a simdutf decode error (overlongC0/C1, truncated tail, stray continuation: all pass the<0xC4gate) fall through to the UTF-16 replacement path so U+FFFD placement andfail_if_invalidsemantics are byte-identicalsimdutf's own
convert_utf8_to_latin1*is avoided: its icelake kernel passes__m512iby value to a helper that does not inline in the debug-ASAN WebKit build and faults onvmovdqa64stack alignment (first caller would trip it).to_utf16_allockeeps itsOption<Vec<u16>>signature as a thin wrapper over the new core so non-string-building callers (WebSocket text frames,sys/windows,bun_string_jsc) are unchanged.Call sites migrated:
encoding.rsowned + borrowed UTF-8 arms,Blobto_string_with_bytes/to_json_with_bytes/Internal::to_string_owned.BunString__fromUTF8gains the same narrowing via WTF'scharactersAreAllLatin1+copyCharacters.Deferred:
to_utf16_alloc_maybe_buffered(feedsTextDecoder) carries[u8;3]split-sequence state that a Latin-1 arm would have to duplicate; per-chunk strings are small. Follow-up.Verification
test/js/web/encoding/utf8-decode-string-width.test.tsasserts the backing width (toBeLatin1String/toBeUTF16String) and decoded value acrossBuffer#toString,Buffer#utf8Slice,Blob#text,Response#textfor: accented Latin, theU+00FF/U+0100boundary, em dash, overlongC0 80, stray continuation, truncatedC3,C3+non-continuation, fullU+0080..U+00FFround-trip, and a >32KB buffer. 10/40 fail before, 40/40 pass after.Release best-of-5, 100KB input through
Buffer#toString('utf8'), same commit/toolchain:Run-to-run spread on this host (~30%) dominates the miss-case delta. The Latin-1-only case trades ~6 µs of decode for a string that is half the size and stays on JSC's LChar paths.
[review] gate passed · iteration 0 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file