strings: call highway_memmem directly on all platforms - #34820
Conversation
The POSIX path called libc::memmem directly while Windows used bstr::ByteSlice::find (which wraps memchr::memmem::Finder, a SIMD implementation with guaranteed-linear worst case). Unify both into the bstr path so all platforms get the same algorithm instead of platform-variable libc memmem, matching what last_index_of already does. Also fixes the stale 'scalar fallback on windows' doc comment; the Windows path has been SIMD via memchr for some time.
WalkthroughChangesMemmem search path
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:08 PM PT - Jul 20th, 2026
❌ @robobun, your commit 09ffddc has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34820That installs a local version of the PR into your bun-34820 --bun |
|
This is a behavior-preserving refactor: CI on build 76505 (09ffddc): 261 passed, 25 canceled. Windows Ready for maintainer merge. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Per review: the previous libc::memmem call on POSIX resolved to highway_memmem via a link-time symbol override in highway_strings.cpp, not platform libc. Make that explicit by adding a bun_highway::memmem wrapper and calling it from strings::memmem on all platforms, matching how index_of_char already works. Windows now uses highway_memmem too instead of bstr/memchr, so all platforms run the same substring search.
robobun
changed the title
There was a problem hiding this comment.
LGTM — the follow-up commit addresses my earlier note: instead of swapping to memchr, this now calls highway_memmem directly, so POSIX hits the exact same MemMemImpl backend as before (just explicitly instead of via the memmem symbol alias) and Windows now matches POSIX and JSBuffer.cpp.
What was reviewed:
- FFI signature matches
highway_strings.cpp:2023(void*(const uint8_t*, size_t, const uint8_t*, size_t)↔*const u8/usize). - Edge cases: empty needle →
Some(0), needle > haystack →None, both guarded in the wrapper (and redundantly inMemMemImplitself);offset_fromis sound since the C++ side returnshaystack + index. - Wrapper follows the same shape as the neighboring
index_of_char;use bun_highway as highwayalready in scope inimmutable.rs.
Extended reasoning...
Overview
Two-file refactor: adds a bun_highway::memmem FFI wrapper around the existing extern "C" highway_memmem entrypoint (src/highway/lib.rs), and rewires bun_core::strings::memmem to call it on all platforms, deleting the #[cfg(windows)]/#[cfg(not(windows))] split (src/bun_core/string/immutable.rs).
This is the second revision. The first (f30210e) routed through memchr::memmem; my earlier review noted that the old libc::memmem call already resolved to highway_memmem via link-time symbol interposition, so that revision was silently swapping backends on a hot path. The author pivoted (09ffddc) to calling highway_memmem directly — which is strictly cleaner than the original code (explicit dependency instead of relying on the .set _memmem, _highway_memmem / alias("highway_memmem") trick) while keeping the same backend on POSIX and unifying Windows with it.
Security risks
None. Read-only substring search over caller-provided &[u8] slices; no allocation, no user-controlled lengths passed to FFI beyond the slice's own .len(). The unsafe block is bounded by the two &[u8] borrows and the empty/oversized-needle guards preceding it.
Level of scrutiny
Low-medium. Hot path (index_of, contains, SplitIterator), but on POSIX this is a no-op at the machine-code level — same HWY_DYNAMIC_DISPATCH(MemMemImpl) reached, just via a direct extern instead of the interposed memmem symbol. The only behavioral delta is Windows moving from bstr/memchr to highway_memmem, which aligns it with the in-tree helper the C++ side (JSBuffer.cpp:1564) already uses — the direction REVIEW.md's "grep for the in-tree helper" rule prefers.
Other factors
- FFI ABI verified against
highway_strings.cpp:2023: parameter types and return (void*↔*const u8) match; the wrapper mirrors the establishedindex_of_charpattern in the same file. p.offset_from(haystack.as_ptr())is well-defined:MemMemImplreturnsconst_cast<uint8_t*>(haystack + index)on success, so the pointer is within the same allocation and the offset is non-negative.- Edge cases (
needle_len == 0,haystack_len < needle_len) are guarded on both the Rust and C++ sides, so behavior matches the removedlibc::memmempath exactly, including on empty haystacks. - The prior inline thread is marked resolved; no outstanding reviewer comments. Bug hunting system found nothing on this revision.
2 participants
What
bun_core::strings::memmempreviously split on#[cfg]: POSIX calledlibc::memmem(which resolved tohighway_memmemvia the link-time symbol override inhighway_strings.cpp:2236-2252), Windows calledbstr::ByteSlice::find(memchr). The doc comment claimed "libc on posix, scalar fallback on windows", both halves of which were wrong.This adds a
bun_highway::memmemwrapper around the existinghighway_memmemC entrypoint and calls it fromstrings::memmemon all platforms, matching howstrings::index_of_char_usizealready callsbun_highway::index_of_char.Why
highway_memmem)#[cfg]split and the misleading doc commentSemantics
Behavior is identical to the previous POSIX path (same
MemMemImplbackend). Empty needle →Some(0), needle longer than haystack →None; both guarded in the Rust wrapper before the FFI call.index_of(the primary caller) additionally guards these itself and is unchanged.cargo check -p bun_coreandcargo clippypass on linux-x64 and x86_64-pc-windows-msvc. Fullbun bdlinks and smoke tests pass locally.This is a behavior-preserving refactor; no fail-before test is constructible.