Skip to content

strings: route all byte search through highway, deny the paths around it - #37052

Merged
Jarred-Sumner merged 7 commits into
mainfrom
claude/slice-methods-simd-comparison-1c2c37
Aug 6, 2026
Merged

strings: route all byte search through highway, deny the paths around it#37052
Jarred-Sumner merged 7 commits into
mainfrom
claude/slice-methods-simd-comparison-1c2c37

Conversation

@dylan-conway

@dylan-conway dylan-conway commented Aug 6, 2026

Copy link
Copy Markdown
Member

What does this PR do?

Byte and substring search over &[u8] now goes through bun_core::strings (Google Highway kernels, runtime CPU dispatch) everywhere, and the routes around it are denied so it stays that way. libcore's slice/str searchers never reach highway (iter().position(|&b| b == x) is a byte-at-a-time loop, <[u8]>::contains is scalar SWAR, windows(n).position() is O(n·m); we also build Rust with -Ctarget-cpu=nehalem on x64), and Rust can't override inherent slice methods, so most sites ported from bun.strings.indexOfChar / std.mem.indexOfScalar had quietly gone scalar.

The slow methods being rooted out (what each compiles to on &[u8]; throughput measured on aarch64, 1 MiB haystack, needle absent — the SIMD memchr baseline on the same machine is ~126 GB/s):

method what it actually does measured
s.iter().position(|&b| b == x) / .rposition() / .any() / .find() / .filter(..).count() (and str::bytes() versions) one ldrb; cmp; b.eq per byte — the early exit stops LLVM from vectorizing or even unrolling 4.5 GB/s
s.split(|b| *b == x) / rsplit same byte-at-a-time predicate loop per field 4.5 GB/s
<[u8]>::contains(&x), str::contains(char), str::find(char) libcore's internal memchr: a 2×usize-per-iteration SWAR loop in scalar registers, no NEON/SSE 48 GB/s
slice::windows(n).position(|w| w == needle) O(n·m) memcmp at every offset (std has no substring search for [u8] at all) 0.75 GB/s
str::find(&str) / str::contains(&str) Two-Way searcher; the simd_contains fast path is compile-time gated on SSE2/NEON, so with our -Ctarget-cpu=nehalem x64 build it never uses AVX2 ~40 GB/s (needle ≤ 32 B)
str::split*/lines/split_once/rsplit_once/matches per-field CharSearcher over the same internal SWAR memchr
memchr::*, bstr::ByteSlice::{find,rfind,find_byte,…} a second, separately-dispatched SIMD implementation (the memchr crate) instead of the one highway path
libc::memrchr (old last_index_of_char on Linux) fine on glibc, a plain byte loop on musl; scalar rposition() on macOS/Windows
WTF::reverseFind (old Buffer.lastIndexOf(byte)) one 16-byte vector per mask→scalar transfer 58 GB/s (now 72)

==, starts_with, ends_with, strip_prefix/suffix lower to memcmp and are left alone.

  • clippy.toml denies str::{find,contains,split*,…}, slice::windows, memchr::*, bstr::ByteSlice search; the memchr crate is dropped. test/internal/source-lints/byte-search.test.ts catches the element-generic forms clippy can't type-filter (byte-literal .contains(&b'x'), iter().position/rposition/any(..), .split(|b| ..)).
  • New kernels: LastIndexOfChar, LastIndexOfAnyChar, IndexOfNotChar, CountChar; memrmem's 1-byte path uses LastIndexOfChar. Buffer.indexOf/lastIndexOf/includes(byte) use them instead of WTF::find/reverseFind.
  • strings::last_index_of_char/last_index_of/count_char/index_of_not_char are highway on every platform (were glibc memrchr / bstr / scalar); new last_index_of_any, contains_any, count, split_any, rsplit, tokenize{,_any}, split_once*, SplitIterator: Iterator.
  • ~250 call sites converted mechanically; no behaviour change intended, with one exception taken from review: a JSX tag name containing - or : is now a string tag regardless of case (<Foo-Bar/>jsx("Foo-Bar"), matching esbuild — previously the check looked for the substring "-:" and emitted jsx(Foo-Bar, …)). Hand-written while s[i] != b'x' loops are out of scope.

Performance

CI bun-darwin-aarch64 artifact of this branch (1cdd47f9e) vs the canary release (d18ddfc1a = this branch's merge-base plus one unrelated commit; byte-identical to main's CI artifact), Apple Silicon, best of 3 interleaved rounds.

User-visible winsBuffer byte search (needle absent unless noted, so the whole buffer is scanned):

256 B 4 KiB 64 KiB 1 MiB
buf.lastIndexOf(0x0a) 14.6 → 8.8 ns (−39%) 86 → 52 ns (−39%) 1139 → 630 ns (−45%) 17.6 → 10.0 µs (−43%)
buf.lastIndexOf("\n") 65 → 62 ns (−5%) 136 → 105 ns (−23%) 1030 → 674 ns (−35%) 15.9 → 10.1 µs (−36%)
buf.lastIndexOf(0x0a), hit at [0] 14.8 → 8.7 ns (−41%) 87 → 56 ns (−36%) 1139 → 629 ns (−45%) 17.6 → 10.0 µs (−43%)
buf.indexOf(0x0a) 14.0 → 9.9 ns (−29%) 82 → 77 ns (−6%) 939 → 934 ns 14.8 → 14.6 µs
buf.includes(0x0a) 23.5 → 19.6 ns (−17%) 83 → 79 ns (−5%) 940 → 934 ns 14.7 → 14.6 µs

Neutral (within noise): path.posix/win32.{normalize,join,resolve,relative,dirname,basename,extname} on 700-byte paths with and without .. segments; bun build of react-dom.development.js / typescript.js (9 MB) / a 2.3 MB comment-heavy CSS file with external sourcemaps; new Bun.Glob("**/*.{rs,ts,zig,cpp}").scanSync("src"); in-process Bun.serve + fetch round trip; startup with a 2000-line .env. (Bare bun -e 0 is ~0.35 ms slower for any PR artifact than for canary because PR lanes link without the startup order file — orderFileEligible() excludes pull requests; a same-toolchain local A/B against the merge-base shows no startup delta.)

Regression found and fixed along the way: the first cut made path.*.normalize/join/resolve/relative ~0.4 ns per input byte slower (e.g. normalize of a 700-byte path 470 → 690 ns) even with no .. in the input — inlining the FFI-reaching last_index_of_char into normalize_string_t's cold .. branch perturbed codegen of its per-byte loop. Fixed by handling sub-16-byte haystacks inline in the bun_highway wrappers (below one vector every kernel is scalar anyway) and moving the .. pop out of line; numbers above are after that fix.

How did you verify your code works?

  • New test/js/bun/util/highway-strings.test.ts sweeps each kernel over lane-boundary lengths × misaligned bases against a scalar reference (through a bun:internal-for-testing hook, since Buffer.indexOf is itself served by these kernels); Buffer.indexOf/lastIndexOf/includes asserted against planted positions incl. byteOffset variants.
  • Existing suites on the debug build: node path (ours + the vendored test-path-*.js), Buffer, which, password (plus new m=+… / unparseable-cost cases for the argon2 ceiling check), cron, ini, css, env, glob, workspaces, websocket, proxy, fetch, udp, transpiler (plus a new dashed/namespaced JSX tag test), bun pm pkg.
  • cargo check for windows / linux-gnu / linux-musl / freebsd / android; clippy + the new source lint clean; verify-baseline-static run locally against the CI linux-x64 profile artifact after allowlisting the new dispatch variants (0 violations).
  • Benchmarks as above; x64 not measured locally.

…paths that bypass it

Byte and substring search over `&[u8]` now goes through `bun_core::strings`
(Google Highway kernels with runtime CPU dispatch) everywhere, and the
libcore / third-party routes around it are denied so it stays that way.

- clippy.toml: deny `str::{find,rfind,contains,split*,lines,matches*,replace*}`,
  `slice::windows`, `memchr::*` and `bstr::ByteSlice` search methods, each
  pointing at the `strings::` replacement. Drop the `memchr` crate.
- test/internal/source-lints/byte-search.test.ts: reject the element-generic
  forms clippy can't type-filter (`.contains(&b'x')`, `iter().position/
  rposition/any/all/find/filter().count()` and `.split()` with a byte-literal
  comparand). Proc-macro crates and build scripts (read from Cargo.toml) are
  exempt.
- highway_strings.cpp: add LastIndexOfChar (two vectors per mask transfer),
  LastIndexOfAnyChar, IndexOfNotChar and CountChar kernels; use LastIndexOfChar
  for memrmem's single-byte needle; expose memrmem/memmem16/memrmem16 to Rust.
  Buffer.indexOf/lastIndexOf/includes(byte) use them instead of WTF::find /
  reverseFind (lastIndexOf on 64 KiB-1 MiB: ~58 -> ~72 GB/s on Apple Silicon;
  no case regresses).
- bun_core::strings: last_index_of_char/last_index_of/count_char/
  index_of_not_char are highway on every platform; the width-generic `_t`
  helpers dispatch u8/u16 to the kernels; add last_index_of_any, contains_any,
  count, split_any, rsplit, tokenize{,_any}, split_once{,_char},
  rsplit_once{,_char}, index_of_any_pos, and `Iterator` for SplitIterator.
- Convert ~250 call sites across the workspace mechanically; no behaviour
  change intended (split iterators yield the same sequence as `<[u8]>::split`).
- test/js/bun/util/highway-strings.test.ts drives each kernel through a
  bun:internal-for-testing hook across lane-boundary lengths and misaligned
  bases against a scalar reference.
@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Changes

SIMD string search migration

Layer / File(s) Summary
Search policy and string APIs
clippy.toml, src/CLAUDE.md, src/bun_core/string/immutable.rs, src/highway/lib.rs
Added Highway-backed search, splitting, tokenization, counting, and replacement APIs. Added lint rules and documentation for prohibited alternatives.
Highway kernels and bindings
src/jsc/bindings/highway_strings.cpp, src/jsc/bindings/JSBuffer.cpp, src/jsc/bindings/highway_strings_testing.*, src/js/internal-for-testing.ts
Added SIMD kernels, testing bindings, and Buffer integration.
Repository-wide migration
Cargo.toml, src/**
Replaced direct byte searches, iterator scans, slice splitting, memchr, and bstr searches with bun_core::strings helpers.
Validation and build metadata
test/internal/source-lints/byte-search.test.ts, test/js/bun/util/highway-strings.test.ts, test/bundler/transpiler/transpiler.test.js, scripts/verify-baseline-static/*
Added source-lint enforcement, SIMD and Buffer tests, JSX coverage, and updated static symbol allowlists.

Possibly related PRs

Suggested reviewers: jarred-sumner, robobun

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes routing byte searches through Highway and denying alternate search paths.
Description check ✅ Passed The description includes the required sections and explains the changes, verification, performance results, and known behavior exception.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 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 `@clippy.toml`:
- Line 42: Update the `str::lines` lint replacement reason in `clippy.toml` to
state that callers using `bun_core::strings::split(bytes, b"\n")` must trim a
trailing `\r` from each field, preserving `str::lines` behavior for CRLF input.

In `@src/clap/streaming.rs`:
- Around line 420-422: Replace the manual suffix loop in the assertion with
strings::index_of(expected, &quoted).is_some(), preserving the existing quoted
byte construction and assertion behavior.

In `@src/css/printer.rs`:
- Around line 431-435: Fix the final-column calculation in write_comment using
the newline index from strings::last_index_of_char: for comments without a
newline, preserve the comment length, and for comments with a newline, use the
number of bytes after that newline (subtract the newline itself). Add regression
tests covering no newline, a single newline, and multiple newline variants to
verify the resulting source-map column.

In `@src/jsc/bindings/highway_strings.cpp`:
- Around line 408-425: The LastIndexOfAnyChar path must safely handle character
sets outside the supported 2..=16 range in release builds. In
src/jsc/bindings/highway_strings.cpp lines 408-425, update LastIndexOfAnyChar’s
preload logic to clamp to the fixed array capacity or return text_len for
invalid lengths, matching IndexOfAnyCharImpl; in src/highway/lib.rs lines
487-508, add a non-debug release guard beside the debug_assert! so
last_index_of_any_char returns None without forwarding invalid lengths through
FFI.

In `@src/parsers/native_test_shims.rs`:
- Around line 61-64: Document src/parsers/native_test_shims.rs as an explicit
exception in the source-validation configuration or rule that scans src/**/*.rs.
Add only this file to the existing exemption list, preserving the current
exemptions for host-only paths and src/collections/linear_fifo.rs, and ensure
the variable-based search in the native test shim is covered by the documented
exception.

In `@test/internal/source-lints/byte-search.test.ts`:
- Around line 37-48: Normalize every path added to hostOnly to forward-slash
form, including proc-macro directories and build-script paths constructed in the
manifest loop. Reuse the same separator normalization applied to rel before the
hostOnly.some comparison, while preserving the existing directory-prefix and
exact-file matching behavior.
🪄 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: a22f3fa7-738c-4483-af47-082e0c88259d

📥 Commits

Reviewing files that changed from the base of the PR and between 0ffabf6 and 0f1d5e5.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (125)
  • Cargo.toml
  • clippy.toml
  • src/CLAUDE.md
  • src/base64/lib.rs
  • src/bun_core/fmt.rs
  • src/bun_core/ip_address.rs
  • src/bun_core/lib.rs
  • src/bun_core/string/immutable.rs
  • src/bun_core/util.rs
  • src/bundler/HTMLScanner.rs
  • src/bundler/defines.rs
  • src/cares_sys/c_ares.rs
  • src/clap/streaming.rs
  • src/css/printer.rs
  • src/css/properties/font.rs
  • src/css/targets.rs
  • src/dotenv/env_loader.rs
  • src/glob/lib.rs
  • src/highway/lib.rs
  • src/http/h2_client/dispatch.rs
  • src/http/h3_client/AltSvc.rs
  • src/http/lib.rs
  • src/http_jsc/websocket_client/WebSocketUpgradeClient.rs
  • src/http_types/MimeType.rs
  • src/http_types/URLPath.rs
  • src/ini/lib.rs
  • src/install/NetworkTask.rs
  • src/install/PackageInstaller.rs
  • src/install/TarballStream.rs
  • src/install/bin.rs
  • src/install/build.rs
  • src/install/dependency.rs
  • src/install/hosted_git_info.rs
  • src/install/lockfile.rs
  • src/io/ParentDeathWatchdog.rs
  • src/js/internal-for-testing.ts
  • src/js_parser/lib.rs
  • src/js_parser/parser.rs
  • src/jsc/bindings/JSBuffer.cpp
  • src/jsc/bindings/highway_strings.cpp
  • src/jsc/bindings/highway_strings_testing.cpp
  • src/jsc/bindings/highway_strings_testing.h
  • src/jsc/btjs.rs
  • src/jsc/resolver_jsc.rs
  • src/libarchive/lib.rs
  • src/lsquic_sys/Cargo.toml
  • src/lsquic_sys/lib.rs
  • src/md/ansi_renderer.rs
  • src/options_types/jsx.rs
  • src/parsers/json.rs
  • src/parsers/json_stage2.rs
  • src/parsers/native_test_shims.rs
  • src/patch/lib.rs
  • src/paths/lib.rs
  • src/paths/resolve_path.rs
  • src/ptr/ref_count.rs
  • src/react_compiler/lowering/build_hir/expr.rs
  • src/react_compiler/lowering/build_hir/jsx.rs
  • src/react_compiler/optimization/optimize_for_ssr.rs
  • src/react_compiler/program.rs
  • src/resolver/lib.rs
  • src/resolver/node_fallbacks.rs
  • src/resolver/package_json.rs
  • src/resolver/tsconfig_json.rs
  • src/router/lib.rs
  • src/runtime/api/Archive.rs
  • src/runtime/api/bun/h2_frame_parser.rs
  • src/runtime/api/bun/js_bun_spawn_bindings.rs
  • src/runtime/api/cron.rs
  • src/runtime/api/cron_parser.rs
  • src/runtime/bake/DevServer.rs
  • src/runtime/bake/FrameworkRouter.rs
  • src/runtime/bake/bake_body.rs
  • src/runtime/bake/dev_server/source_map_store.rs
  • src/runtime/cli/bunx_command.rs
  • src/runtime/cli/create_command.rs
  • src/runtime/cli/init_command.rs
  • src/runtime/cli/install_completions_command.rs
  • src/runtime/cli/mod.rs
  • src/runtime/cli/multi_run.rs
  • src/runtime/cli/pack_command.rs
  • src/runtime/cli/pm_pkg_command.rs
  • src/runtime/cli/pm_version_command.rs
  • src/runtime/cli/repl.rs
  • src/runtime/cli/run_command.rs
  • src/runtime/cli/test/ChangedFilesFilter.rs
  • src/runtime/cli/test/parallel/aggregate.rs
  • src/runtime/cli/update_interactive_command.rs
  • src/runtime/cli/why_command.rs
  • src/runtime/crypto/pwhash.rs
  • src/runtime/dns_jsc/dns.rs
  • src/runtime/ffi/ffi_body.rs
  • src/runtime/image/codecs.rs
  • src/runtime/node/dir_iterator.rs
  • src/runtime/node/memory_pressure.rs
  • src/runtime/node/node_os.rs
  • src/runtime/node/node_process.rs
  • src/runtime/node/path.rs
  • src/runtime/node/quic/endpoint.rs
  • src/runtime/node/quic/tls.rs
  • src/runtime/server/DirectoryRoute.rs
  • src/runtime/server/RequestContext.rs
  • src/runtime/server/server_body.rs
  • src/runtime/shell/builtin/export.rs
  • src/runtime/shell/builtin/rm.rs
  • src/runtime/socket/Handlers.rs
  • src/runtime/socket/SocketAddress.rs
  • src/runtime/socket/udp_socket.rs
  • src/runtime/test_runner/diff/printDiff.rs
  • src/runtime/test_runner/expect.rs
  • src/runtime/test_runner/expect/toIncludeRepeated.rs
  • src/runtime/test_runner/snapshot.rs
  • src/runtime/webcore/Blob.rs
  • src/runtime/webview/ChromeProcess.rs
  • src/s3_signing/credentials.rs
  • src/sourcemap/lib.rs
  • src/spawn/lib.rs
  • src/sql/postgres/protocol/NewReader.rs
  • src/sql_jsc/postgres/PostgresSQLConnection.rs
  • src/sys/Cargo.toml
  • src/sys/lib.rs
  • src/sys/windows/mod.rs
  • src/which/lib.rs
  • test/internal/source-lints/byte-search.test.ts
  • test/js/bun/util/highway-strings.test.ts
💤 Files with no reviewable changes (2)
  • Cargo.toml
  • src/sys/Cargo.toml

Comment thread clippy.toml Outdated
Comment thread src/clap/streaming.rs
Comment thread src/css/printer.rs
Comment thread src/jsc/bindings/highway_strings.cpp
Comment thread src/parsers/native_test_shims.rs
Comment thread test/internal/source-lints/byte-search.test.ts
Comment thread test/js/bun/util/highway-strings.test.ts Outdated
Comment thread src/js_parser/parser.rs Outdated
@dylan-conway dylan-conway self-assigned this Aug 6, 2026
… test oracle

- js_parser: a JSX tag name containing '-' or ':' is a string tag regardless
  of case (`<Foo-Bar/>` -> jsx("Foo-Bar")), matching esbuild's ContainsAny;
  the previous check looked for the substring "-:" and emitted
  `jsx(Foo-Bar, ...)`. Adds a transpiler test.
- strings::index_of_any / last_index_of_any accept sets larger than 16 bytes
  by scanning per 16-byte chunk instead of debug-asserting; the
  LastIndexOfAnyChar kernel clamps its preload count defensively.
- highway-strings.test.ts: assert Buffer.indexOf/lastIndexOf against the
  planted positions (Buffer methods are the code under test, not an oracle)
  and cover byteOffset variants.
- byte-search lint: normalize host-only manifest paths to forward slashes.
- clippy.toml: note that str::lines also strips a trailing '\r'.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/runtime/socket/udp_socket.rs (1)

354-367: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate the duplicated port-range validation.

The connect.port validation at lines 478-483 repeats the same coerce-and-range-check logic as the new parse_port helper at lines 1542-1553 (and the port field check at lines 354-367 follows the same pattern with a different lower bound). Extract a shared helper that takes the field name and minimum bound as parameters, and call it from all three sites.
[recommended]

♻️ Proposed shared helper
+    /// Coerce `value` to a port number in `[min, 65535]`, per Node's
+    /// `validatePort`. ToNumber on the value can run user JS.
+    fn coerce_port_range(
+        global_this: &JSGlobalObject,
+        value: JSValue,
+        field_name: &str,
+        min: f64,
+    ) -> JsResult<u16> {
+        // Range-check as f64: ToInt32 would wrap e.g. 2^32 + 9 to 9.
+        let number = value.coerce_f64(global_this)?;
+        if number.fract() != 0.0 || !(min..=65535.0).contains(&number) {
+            return Err(global_this.throw_invalid_arguments(format_args!(
+                "Expected \"{}\" to be an integer between {} and 65535",
+                field_name, min as u32
+            )));
+        }
+        Ok(number as u16)
+    }
+
     fn parse_port(global_this: &JSGlobalObject, port_val: JSValue) -> JsResult<u16> {
-        // Range-check as f64: ToInt32 would wrap e.g. 2^32 + 9 to 9.
-        let number = port_val.coerce_f64(global_this)?;
-        if number.fract() != 0.0 || !(1.0..=65535.0).contains(&number) {
-            return Err(global_this.throw_invalid_arguments(format_args!(
-                "Expected \"port\" to be an integer between 1 and 65535"
-            )));
-        }
-        Ok(number as u16)
+        Self::coerce_port_range(global_this, port_val, "port", 1.0)
     }

Also applies to: 478-483, 1542-1553

🤖 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/runtime/socket/udp_socket.rs` around lines 354 - 367, Extract the
repeated port parsing and range validation into a shared helper that accepts the
field name and minimum allowed value, preserving the existing error behavior and
bounds. Update the `port` field parsing block, `connect.port` validation, and
`parse_port` helper to call this shared function, using the appropriate lower
bound for each field.
🤖 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/runtime/socket/udp_socket.rs`:
- Around line 354-367: Extract the repeated port parsing and range validation
into a shared helper that accepts the field name and minimum allowed value,
preserving the existing error behavior and bounds. Update the `port` field
parsing block, `connect.port` validation, and `parse_port` helper to call this
shared function, using the appropriate lower bound for each field.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 27325780-b203-476c-8aff-8a4600e44d69

📥 Commits

Reviewing files that changed from the base of the PR and between 0f1d5e5 and 2d25c57.

📒 Files selected for processing (12)
  • clippy.toml
  • src/bun_core/string/immutable.rs
  • src/clap/streaming.rs
  • src/http/lib.rs
  • src/install/hosted_git_info.rs
  • src/js_parser/parser.rs
  • src/jsc/bindings/highway_strings.cpp
  • src/runtime/socket/udp_socket.rs
  • src/runtime/test_runner/expect.rs
  • test/bundler/transpiler/transpiler.test.js
  • test/internal/source-lints/byte-search.test.ts
  • test/js/bun/util/highway-strings.test.ts
💤 Files with no reviewable changes (1)
  • src/install/hosted_git_info.rs

Comment thread src/glob/lib.rs
CountCharImpl, IndexOfNotCharImpl, LastIndexOfCharImpl and
LastIndexOfAnyCharImpl are compiled per HWY_DYNAMIC_DISPATCH target
(SVE/SVE2, AVX2/AVX3 variants) and gated by hwy::SupportedTargets like the
neighbouring highway_strings.cpp kernels; add them to the aarch64, x64 and
x64-windows static-scan allowlists with the feature ceilings the scanner
reported. Verified locally against the CI linux-x64 profile artifact
(0 violations).

No-Verification-Needed: CI allowlist data only, no runtime surface

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@scripts/verify-baseline-static/allowlist-x64.txt`:
- Line 462: Synchronize the Highway block-count headers with their symbol lists:
update scripts/verify-baseline-static/allowlist-x64.txt at lines 462-462 and
scripts/verify-baseline-static/allowlist-x64-windows.txt at lines 440-440 from
227 to 243 symbols, and update
scripts/verify-baseline-static/allowlist-aarch64.txt at lines 8-8 from 149 to
161 symbols.
🪄 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: cdf837af-7fff-4655-ab14-3b735f325272

📥 Commits

Reviewing files that changed from the base of the PR and between 2d25c57 and f71b416.

📒 Files selected for processing (3)
  • scripts/verify-baseline-static/allowlist-aarch64.txt
  • scripts/verify-baseline-static/allowlist-x64-windows.txt
  • scripts/verify-baseline-static/allowlist-x64.txt

Comment thread scripts/verify-baseline-static/allowlist-x64.txt Outdated
Comment thread src/runtime/crypto/pwhash.rs Outdated
…ze's byte loop call-free

Benchmarking the CI build against canary showed node:path normalize/join/
resolve/relative ~0.4 ns per input byte slower (e.g. path.posix.normalize of
a 700-byte path 470ns -> 690ns): inlining the FFI-reaching
`strings::last_index_of_char_t` into normalize_string_t's cold `..` branch
perturbed codegen of its per-byte loop, even for inputs with no `..` at all.

- bun_highway: index_of_char / last_index_of_char / index_of_not_char /
  count_char / index_of_any_char / last_index_of_any_char handle haystacks
  shorter than 16 bytes (below one vector on every dispatch target, where the
  kernels only do a scalar tail or a single masked op) inline instead of
  crossing FFI + the dispatch table.
- node/path.rs: move the `..` segment pop into an #[inline(never)]
  pop_last_segment_t so the loop body has no call in it.

Same-toolchain local A/B vs the merge base: path.* back to parity (-2..-6%),
bun build / CSS build / startup-with-.env / Glob.scanSync / HTTP round trip
unchanged.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@src/highway/lib.rs`:
- Around line 501-504: Update last_index_of_any_char to return None when chars
is empty before the debug_assert! and before either scalar processing or
highway_last_index_of_any_char FFI dispatch. Preserve the existing 2–16
candidate validation and behavior for non-empty character sets, matching
index_of_any_char.
🪄 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: 0c15496a-67dc-4939-9527-17301fb34204

📥 Commits

Reviewing files that changed from the base of the PR and between f71b416 and 1cdd47f.

📒 Files selected for processing (2)
  • src/highway/lib.rs
  • src/runtime/node/path.rs

Comment thread src/highway/lib.rs
… grammar

- pwhash: the m=/t=/p= pre-check parsed with parse_unsigned, which rejects a
  leading '+', and skipped the field on parse failure; rust-argon2's decoder
  is str::parse (accepts '+'), so `m=+4294967294` bypassed
  MAX_VERIFY_MEMORY_COST and the decoder tried to allocate it (SIGKILL).
  Parse with the same grammar and fail closed with InvalidEncoding when a
  known cost field doesn't parse. Test added next to the existing ceiling
  cases.
- glob::detect_glob_syntax: count preceding backslashes in `slice`, which is
  what `idx` indexes, not the original pattern (pre-existing; e.g. `\*a*`
  was reported as not-a-glob).
- bun_highway::last_index_of_any_char: return None for an empty set before
  the debug_assert, like index_of_any_char.
- verify-baseline allowlists: correct the Highway group symbol counts.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/glob/lib.rs`:
- Around line 39-41: Update the backslash parity tracking in the surrounding
glob-processing logic: replace the u16 backslash_count counter with a boolean
odd/even state, toggling it for each consecutive backslash. Use that boolean
wherever the count’s parity is checked, preserving behavior for arbitrary-length
user input without numeric overflow.

In `@test/js/bun/util/password.test.ts`:
- Around line 501-503: Strengthen the malformed-cost test around password
verification by asserting that both verifySync and the asynchronous
password.verify API reject the input with InvalidEncoding. Preserve the existing
junkMemory fixture and correct-password input while covering the synchronous and
asynchronous sibling APIs explicitly.
🪄 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: 8438e367-8d5d-4611-83e5-c5e0bd1b1c0d

📥 Commits

Reviewing files that changed from the base of the PR and between 1cdd47f and c90b881.

📒 Files selected for processing (7)
  • scripts/verify-baseline-static/allowlist-aarch64.txt
  • scripts/verify-baseline-static/allowlist-x64-windows.txt
  • scripts/verify-baseline-static/allowlist-x64.txt
  • src/glob/lib.rs
  • src/highway/lib.rs
  • src/runtime/crypto/pwhash.rs
  • test/js/bun/util/password.test.ts

Comment thread src/glob/lib.rs Outdated
Comment thread test/js/bun/util/password.test.ts Outdated
… argon2 junk-cost test

- detect_glob_syntax only needs the parity of the preceding backslash run,
  so toggle a bool instead of counting in a u16 (which could overflow-panic
  in debug on absurd input).
- password test: assert the specific InvalidEncoding error for an
  unparseable cost field, for both verifySync and verify.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No new issues found; all earlier threads (argon2 cost-cap bypass, glob backslash indexing, any-char set bounds, empty-set guard) are resolved. Deferring to a human given the scope — 131 files, new Highway SIMD kernels with FFI wrappers, ~250 mechanical call-site conversions across security-sensitive validation paths, a workspace-wide clippy denial of libcore search methods, and one intentional JSX behaviour change.

What was reviewed:

  • New C++ kernels (LastIndexOfCharImpl, CountCharImpl, IndexOfNotCharImpl, LastIndexOfAnyCharImpl) for lane-boundary/tail handling and the 255-vector flush in CountCharImpl.
  • strings::split/split_any/tokenize*/rsplit/split_once* semantics against the .split(|b| ..) forms they replace, incl. empty-delimiter and empty-input cases.
  • Security-gating conversions (bin_target_escapes_package_dir, alias_is_safe_install_target, is_safe_patch_path, has_unsafe_tarball_filename_part, S3 bucket check) — each preserves the original predicate.
  • The argon2 verify pre-check now fails closed with the decoder's own integer grammar; JSBuffer.cpp byteOffset plumbing into the new kernels matches the WTF path it replaces.
Extended reasoning...

Overview

This PR routes all &[u8] byte and substring search through bun_core::strings (Google Highway kernels with runtime CPU dispatch) and denies the libcore/memchr/bstr alternatives via clippy.toml plus a source-lint test. It adds four new C++ kernels (LastIndexOfChar, IndexOfNotChar, CountChar, LastIndexOfAnyChar), their Rust FFI wrappers with a sub-16-byte scalar prologue, and a family of new strings:: helpers (split_any, rsplit, tokenize{,_any}, split_once*, count, contains_any, last_index_of_any). Roughly 250 call sites across 131 files are converted mechanically, Buffer.indexOf/lastIndexOf/includes(byte) are rewired in JSBuffer.cpp, and one intentional behaviour change lands (JSX tags containing -/: are string tags regardless of case, matching esbuild).

Security risks

Several converted sites are security gates: package-install path traversal checks (bin.rs, PackageInstaller.rs, dependency.rs), tarball/archive/patch path validation, S3 bucket host injection, and the argon2 verify DoS cap in pwhash.rs. Each was traced to confirm the new form preserves the exact predicate; the argon2 case initially regressed (my earlier finding — a leading + bypassed the memory-cost cap) and is now fixed to fail closed with the decoder's grammar plus tests. The new C++ kernels take raw ptr/len and are called from safe Rust wrappers; bounds and the 2..=16 char-set precondition are guarded on the Rust side, and LastIndexOfAnyCharImpl additionally clamps chars_len defensively. No new attack surface identified beyond what was already addressed in-thread.

Level of scrutiny

High. This is a cross-cutting refactor touching hot paths (parser, path normalization, JSON stage-2, resolver), memory-safety-critical unsafe SIMD C++, FFI boundaries with debug-assert-only preconditions, and multiple security-relevant validation predicates. It also imposes a workspace-wide policy (48 clippy denials + a regex-based source lint) that every future contributor will interact with. The one intentional behaviour change (JSX tag classification) alters transpiler output. None of this fits the "simple, mechanical, obvious" bar for bot approval.

Other factors

The PR has already been through several review rounds: CodeRabbit and my earlier passes surfaced the argon2 bypass, a pre-existing glob indexing bug, any-char set bounds, and an empty-set debug-assert mismatch — all fixed across follow-up commits with tests added. A dedicated kernel test (highway-strings.test.ts) sweeps lengths × alignments against scalar references via bun:internal-for-testing, and the JSX change has a transpiler test. The bug-hunting pass on the current head found nothing new. All review threads are resolved. What remains is a maintainer sign-off on the architectural choice (denying libcore search wholesale), the new kernel implementations, and a spot-check of the higher-risk conversion sites — that's a human call.

@Jarred-Sumner
Jarred-Sumner merged commit 2c5c312 into main Aug 6, 2026
47 of 49 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the claude/slice-methods-simd-comparison-1c2c37 branch August 6, 2026 22:30
robobun added a commit that referenced this pull request Aug 7, 2026
…ernels

cargo miri test cannot call foreign functions, and #37052 routed all
byte search through the highway FFI, which broke the Miri workflow:
bun_ptr's type_base_name hits highway_memrmem via strings::last_index_of.

Under cfg(miri) the char and char-set scans take their existing scalar
prologue at every length, and the four mem*mem wrappers use a scalar
substring search. Kernels with no scalar form here (hashing, hex,
sourcemaps, lexer scans) stay FFI-only, so a Miri-tested crate reaching
one still fails loudly.
dylan-conway pushed a commit that referenced this pull request Aug 7, 2026
)

`cargo clippy` and `cargo fmt --check` are red on main, so every PR
touching Rust inherits failing Clippy and Format checks. Both workflows
only run on pull_request, which is how the drift landed unnoticed.

## Clippy (2 errors)

- `src/parsers/yaml.rs`: `bind_anchor` takes `PendingAnchor` by value on
purpose, so that binding consumes the `#[must_use]` anchor token (landed
via #37055, trips `-D clippy::needless-pass-by-value`). Added a targeted
`#[allow]` with a one-line reason, matching existing usage elsewhere in
the tree.
- `src/runtime/socket/uws_handlers.rs`: #37067 changed
`NewSocket::on_close` / `on_handshake` to return `()`, leaving two
`swallow(...)` wrappers passing a unit value (`-D clippy::unit-arg`).
Call them directly, matching the neighboring handlers in the same impl.

`cargo clippy --workspace --no-deps` now exits 0.

## rustfmt (4 files)

`src/bun_core/tty.rs`, `src/md/ansi_renderer.rs`,
`src/runtime/bake/bake_body.rs`, `src/runtime/server/server_body.rs` had
unformatted hunks. Ran `cargo fmt --all`; `cargo fmt --all --check` now
exits 0.

## tsconfig

- Root `tsconfig.json` still referenced `./src/bake`, which moved to
`./src/runtime/bake` in the Rust rewrite, so `tsc --noEmit` failed
immediately with TS6053. Updated the project reference.
- `test/tsconfig.json` now excludes the three
`test/regression/issue/14477/*-mismatch.tsx` fixtures: they contain
deliberately mismatched JSX closing tags (the test asserts the parse
error), which are unsuppressable TS17002 syntax errors.

Note: `cd test && tsc --noEmit` still reports several thousand
pre-existing semantic errors across the test suite; that is
long-standing drift (not CI-enforced) and out of scope here.

Also verified green on this branch: oxlint, clang-format check,
prettier.

## Miri

`cargo miri test` is also red on every PR: #37052 routed all byte search
through the highway C++ kernels, and Miri cannot call foreign functions.
The first caller to hit it is `bun_ptr::ref_count::type_base_name`
(`strings::last_index_of` -> `highway_memrmem`). Under `cfg(miri)` the
search wrappers in `src/highway/lib.rs` now take their scalar paths: the
char and char-set scans reuse their existing short-input scalar prologue
at every length, and the `mem*mem` wrappers get a scalar substring
search. Kernels with no scalar form (hashing, hex, sourcemaps, lexer
scans) stay FFI-only so a Miri-tested crate reaching one still fails
loudly. Verified locally: `bun run rust:miri` green on bun_ptr
(previously failing), bun_ast, bun_base64, bun_clap, bun_collections,
bun_dispatch, bun_errno, and bun_hash; this PR's Miri workflow runs the
full set.

## Verification

The changes have no runtime-observable behavior: the proof is this PR's
own Clippy and Format CI checks, which run `cargo clippy --workspace`
and `cargo fmt --all --check` (both red on main, both green here), plus
`tsc --noEmit` resolving again at the repo root. An earlier revision
added a source-lint test walking the tsconfig reference graph; it was
removed per maintainer feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants