Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback - #36420
Conversation
Buffer#indexOf/includes went through highway_memmem, which SIMD-scanned for the needle's first byte and memcmp'd every candidate; lastIndexOf and the utf16le paths used std::find_end / std::search. All four are O(haystack * needle) on adversarial input: a 4 MiB 'a' haystack with a 4000-byte 'a…ab' needle took ~250 ms forward and ~7 s backward, a 1500x gap vs Node. highway_memmem, and new highway_memrmem / highway_memmem16 / highway_memrmem16, now use a two-anchor SIMD filter on the needle's two least-frequent bytes (full-needle histogram) so any distinguishing byte anywhere in the needle prunes to zero candidates. A false-positive budget caps total memcmp work at ~2*|haystack| and hands the remainder to a Two-Way (Crochemore-Perrin) search, which is O(n+m) worst case with O(1) space and handles reverse search via index-reversed views. The utf16le paths share the same kernel over uint16_t lanes. The libc memmem alias on Linux/macOS is the same symbol, so every in-process memmem call inherits the linear bound.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 5:36 PM PT - Jul 29th, 2026
⏳ @robobun, your commit b01343b is still building in |
|
The remaining comment-cop flags are on algorithm documentation (Two-Way attribution, anchor-selection rationale, the SIMD kernel section header) and a two-line contract. They explain non-obvious design, not workarounds; I trimmed what could go in 847e7ea and am keeping the rest. |
WalkthroughAdds a Two-Way fallback search, SIMD forward and reverse searches for byte and UTF-16 buffers, Buffer integration, and adversarial performance and correctness tests. ChangesBuffer substring search
🚥 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: 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 `@src/jsc/bindings/highway_strings.cpp`:
- Around line 928-934: Update the comments above kNotFound and kFallback to
identify kFallback as the sentinel requesting Two-Way fallback, and remove the
reference to the nonexistent BM template. Ensure the wording accurately
describes how MemMemImpl and MemRMemImpl use these sentinels.
- Around line 169-203: Avoid the unconditional 256-entry histogram setup in
MemMemPickAnchors for short needles by using the existing two-end anchor
selection below the intended length threshold, while preserving histogram-based
ranking for longer needles. Add a brief comment near bucket() documenting that
UTF-16 values are intentionally ranked by their low byte while matching remains
full-lane safe.
In `@src/jsc/bindings/JSBuffer.cpp`:
- Around line 1572-1614: Export the shared not-found sentinel used by
highway_memmem16 and highway_memrmem16 from a header alongside their
declarations, rather than keeping kNotFound private to highway_strings.cpp.
Update indexOf16, lastIndexOf16, and lastIndexOf to use that shared symbol at
all three sentinel checks, preserving the existing -1 return behavior.
In `@test/js/node/buffer-indexof-worstcase.test.ts`:
- Around line 156-158: Update the coverage comment near the deterministic
pseudo-random cross-check to remove references to the Boyer-Moore fallback and
kBMMinPatternLength. Describe only the durable input cases exercised by the
test—SIMD anchor filtering, short needles, utf16le, and byteOffset
handling—without asserting a specific search algorithm path.
- Around line 212-219: Update the test “utf16le indexOf / lastIndexOf still
correct” to pass UTF-16 strings as the needle arguments instead of Buffers,
including the missing-needle cases, so it exercises the indexOfString path
through indexOf16 and lastIndexOf16 while preserving the existing expected
offsets and -1 results.
- Around line 13-20: Update refLastIndexOf’s reverse-search start calculation to
clamp the result at zero by applying Math.max(0, ...) around the existing
upper-bound calculation. Preserve the empty-needle handling and reverse
iteration behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b44dabad-ea51-4890-bfa9-e2c78ef3bbd4
📒 Files selected for processing (4)
src/jsc/bindings/BufferStringSearch.hsrc/jsc/bindings/JSBuffer.cppsrc/jsc/bindings/highway_strings.cpptest/js/node/buffer-indexof-worstcase.test.ts
…ing-needle utf16 coverage
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/jsc/bindings/highway_strings.cpp (3)
2180-2183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAttribute the Two-Way fallback implementation.
This wrapper delegates to
bun::SearchString, but the fallback’s role is not documented at the handoff site. Add a short attribution identifying it as the guaranteed-linear Two-Way fallback used after the SIMD verification budget is exhausted.Based on the supplied search-layer contract, this is the correctness and complexity backstop for the SIMD filter.
🤖 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/highway_strings.cpp` around lines 2180 - 2183, Add a short attribution comment immediately above MemMemTwoWayFallback identifying bun::SearchString as the guaranteed-linear Two-Way fallback used after the SIMD verification budget is exhausted. Leave the wrapper’s delegation and behavior unchanged.
154-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the SIMD-to-Two-Way resume contract.
start_indexandis_forwardare the cross-boundary handoff contract between the SIMD kernels andbun::SearchString, but their direction-specific resume semantics and miss sentinel are undocumented. Add a concise comment here so future changes cannot accidentally recheck or skip candidates.🤖 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/highway_strings.cpp` around lines 154 - 157, Document the MemMemTwoWayFallback declaration’s SIMD-to-Two-Way handoff contract, specifying how start_index resumes searches for forward and reverse is_forward modes and identifying the miss sentinel returned by the fallback. Keep the comment concise and colocated with the declaration.
937-1018: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a concise SIMD-kernel section header.
This block contains the two-anchor filter, verification budget, and Two-Way handoff. A durable header immediately before
MemMemForwardwould make the algorithm boundary clear without requiring implementation-history comments.As per coding guidelines, comments should contain durable algorithm rationale rather than workaround narration.
🤖 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/highway_strings.cpp` around lines 937 - 1018, Add a concise durable algorithm-section header immediately before MemMemForward, identifying this block as the SIMD two-anchor filtering and verification-budget path, including its Two-Way fallback handoff. Keep the comment focused on algorithm boundaries and rationale; do not add implementation-history or workaround details.Source: Coding guidelines
🤖 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/jsc/bindings/highway_strings.cpp`:
- Around line 159-173: Update MemMemPickAnchors so its comments explicitly
distinguish the short-needle first/last-anchor policy from the least-frequency
histogram policy. Introduce a named constant for the short-needle threshold and
use it in the needle_len check, replacing the literal 16 while preserving the
existing behavior.
---
Outside diff comments:
In `@src/jsc/bindings/highway_strings.cpp`:
- Around line 2180-2183: Add a short attribution comment immediately above
MemMemTwoWayFallback identifying bun::SearchString as the guaranteed-linear
Two-Way fallback used after the SIMD verification budget is exhausted. Leave the
wrapper’s delegation and behavior unchanged.
- Around line 154-157: Document the MemMemTwoWayFallback declaration’s
SIMD-to-Two-Way handoff contract, specifying how start_index resumes searches
for forward and reverse is_forward modes and identifying the miss sentinel
returned by the fallback. Keep the comment concise and colocated with the
declaration.
- Around line 937-1018: Add a concise durable algorithm-section header
immediately before MemMemForward, identifying this block as the SIMD two-anchor
filtering and verification-budget path, including its Two-Way fallback handoff.
Keep the comment focused on algorithm boundaries and rationale; do not add
implementation-history or workaround details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53f9385e-1a51-45f6-b41b-f818e8e37c20
📒 Files selected for processing (3)
src/jsc/bindings/JSBuffer.cppsrc/jsc/bindings/highway_strings.cpptest/js/node/buffer-indexof-worstcase.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… kernels Same HWY_DYNAMIC_DISPATCH gate as the existing MemMemImpl entries.
…_t> N_AVX3 instantiation
|
CI on build #85551: all verify-baseline lanes pass, |
There was a problem hiding this comment.
I didn't find any bugs, but this rewrites the process-wide memmem (aliased as libc memmem on Linux/macOS) plus three new SIMD kernels and a from-scratch Two-Way search — worth a human look.
What was reviewed:
- SIMD load bounds in
MemMemForward/MemMemReverse— both anchor loads stay within[haystack, haystack+len)for every lane. kFallback→SearchStringresume-index handoff for the reverse path —resume ≤ diff, sorelative_start_indexdoesn't wrap.WTF::reverseFind'snotFoundsentinel round-trips to-1via theint64_treturn.- All prior CodeRabbit / comment-cop threads are resolved at HEAD.
Extended reasoning...
Overview
Replaces Buffer#indexOf/lastIndexOf/includes's search kernel with a two-anchor rare-byte SIMD filter plus a Two-Way (Crochemore-Perrin) O(n+m) fallback. Touches: highway_strings.cpp (~320 lines: rewritten MemMemImpl, new MemMemForward/MemMemReverse templates, MemMemPickAnchors, three new MemRMem/MemMem16/MemRMem16 impls with HWY_EXPORT + C wrappers), new BufferStringSearch.h (153-line Two-Way implementation), JSBuffer.cpp (rewires indexOf16/lastIndexOf16/lastIndexOf to the new kernels), three verify-baseline allowlists, and a 238-line test file.
Security risks
The change operates on caller-supplied buffer bounds only — no parsing of untrusted headers, no allocation sized from external input. The main risk class is out-of-bounds reads in the SIMD anchor loads and memcmp verification. I traced the loop invariants for both directions and both remainder loops: with anchor_b ≤ needle_len-1 and the loop conditions i+N ≤ last_start+1 (forward) / i ≥ N before decrement (reverse), the furthest byte read is haystack_len-1. MemMemVerify is only called with pos ≤ last_start. No injection/auth surface.
Level of scrutiny
High. highway_memmem is aliased as libc memmem on Linux and macOS, so a regression here affects every in-process substring search, not just Buffer#indexOf. The Two-Way algorithm is subtle (critical factorization, periodic vs non-periodic branches, index-reversing Vector for reverse search) and is a fresh implementation rather than a vendored copy. The verify-baseline allowlist edits declare which ISA features each new per-target symbol may use — these need to match what the compiler actually emits across six x64 target tiers plus four aarch64 SVE tiers.
Other factors
Test coverage is strong (adversarial timing budgets branched on isASAN/isDebug, 1500-trial randomized cross-check, boundary sweep, utf16le for both string- and Buffer-needle paths, Two-Way periodic/non-periodic forcing) and the PR reports the existing buffer.test.js / test-buffer-indexof.js / test-buffer-includes.js suites still pass. The timing-based tests use best-of-3 and generous debug budgets, but ratio assertions on wall-clock time are inherently a flake risk on noisy CI. All CodeRabbit and comment-cop threads are resolved; my earlier stale-comment nit was fixed in 847e7ea/b9c9414. Given the blast radius (libc override, per-target SIMD codegen across all platforms) and algorithm complexity, this should get human eyes before merge.
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/jsc/bindings/BunDebugger.cpp
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ... # Conflicts: # src/js/internal/debugger.ts
* upstream/main: (422 commits) install: drop packages held only by optional-peer resolution slots from bun.lock (oven-sh#35681) Update mimalloc to the upstream dev3 (v3.4.3) sync (oven-sh#36431) compile(pe): ftruncate the Windows --compile output after writing (oven-sh#36430) Strong: back bun_jsc::Strong with StrongRootBlock; free AbortSignal.timeout at wrapper GC (oven-sh#35849) test(harness): replace toRun matcher with async bunRun + toSpawn (oven-sh#36424) test: measure memory via harness rss() instead of process.memoryUsage.rss() (oven-sh#36429) Deflake a few tests no-orphans(windows): allow CREATE_BREAKAWAY_FROM_JOB and set DIE_ON_UNHANDLED_EXCEPTION on the Job (oven-sh#36414) GarbageCollectionController: replace per-tick heap sampler with idle timer only (oven-sh#35356) exe_format(pe): write a valid OptionalHeader.CheckSum for --compile output (oven-sh#36383) FileSink: flush buffered bytes when process.exit() runs in the same tick as write() (oven-sh#36250) test(http): speed up and de-flake serve-async-stream-client-abort.test.ts (oven-sh#35919) test(20144): stop racing child startup against the 1s SIGKILL guard (oven-sh#34166) test(no-orphans): skip fast-exit perl daemon test on macOS (oven-sh#36413) fs: return negative BigIntStats *Ns for pre-epoch timestamps (oven-sh#36187) event_loop: make DeferredTaskQueue::run tolerate re-entrant map mutation (oven-sh#32703) dotenv: stop panicking on nested `${...}` inside `${VAR:-default}` (oven-sh#36199) fetch: make the idle timer an absolute deadline for the response header block (oven-sh#36145) bundler: don't panic on unterminated naming template placeholders (oven-sh#36325) Buffer#indexOf/lastIndexOf: rare-byte SIMD filter with a Two-Way O(n+m) fallback (oven-sh#36420) ...
2 participants
What
Buffer#indexOf/includes/lastIndexOfhad an O(haystack × needle) worst case on adversarial input. This replaces the search kernel with a rare-byte two-anchor SIMD filter backed by a guaranteed-linear Two-Way fallback, for all four paths (uint8_t/uint16_t× forward/reverse).Repro
With the 'b' in the middle of the needle instead of at an end, Node's Boyer-Moore (capped at a 250-byte shift table) is also quadratic:
indexOftakes 7.2 s at m=4000 and times out at m=16000. The rare-byte anchor picker here anchors on that 'b' directly, so the same case is under a millisecond.Cause
highway_memmem(MemMemImpl) SIMD-scanned for the needle's first byte only, thenmemcmp'd every candidate. When the first byte is the haystack's dominant byte, that's N candidates × up-to-m compare each. The same symbol is aliased as libcmemmemon Linux and macOS, so every in-processmemmeminherited this.lastIndexOfwasstd::find_endand theutf16lepaths werestd::search/std::find_end, all naive.Fix
highway_strings.cpp
MemMemImpland newMemRMemImpl/MemMem16Impl/MemRMem16Implshare a two-anchor SIMD kernel (MemMemForward/MemMemReverse, templated on lane type): load a vector athaystack + i + anchor_aand athaystack + i + anchor_b, AND the equality masks, and onlymemcmplanes where both match. Reverse usesFindKnownLastTrue+FirstNto iterate candidates from the top.MemMemPickAnchorschoosesanchor_a/anchor_bas the needle's two least-frequent bytes via a full-needle histogram (low byte foruint16_t), so any distinguishing byte anywhere in the needle prunes the candidate set.2*|haystack|/|needle| + 32caps totalmemcmpwork at ~2·|haystack|; when it trips, the remaining range goes toMemMemTwoWayFallback.highway_memrmem/highway_memmem16/highway_memrmem16dispatch the per-target kernels.BufferStringSearch.h (new)
Two-Way string matching (Crochemore & Perrin, the algorithm glibc and musl
memmemuse): O(n + m) worst case, O(1) extra space. Templated onCharand driven through an index-reversingVectorso the same code serves forward and reverse search without copying.JSBuffer.cpp
lastIndexOfnow callshighway_memrmem;indexOf16/lastIndexOf16callhighway_memmem16/highway_memrmem16.std::find_end/std::searchare gone.Tests
test/js/node/buffer-indexof-worstcase.test.ts:utf16leand presence checks.Existing coverage still green:
test/js/node/buffer.test.js(617),test-buffer-indexof.js,test-buffer-includes.js,buffer-indexOf-detach.test.ts,stringWidth.test.ts.[review] gate passed · iteration 2 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file