Optimize StringView substring search - #3786
Conversation
There was a problem hiding this comment.
Pull request overview
Improves MoonBit standard library substring search performance by optimizing StringView::find / StringView::rev_find in dense false-positive scenarios while retaining Boyer–Moore–Horspool behavior for typical cases.
Changes:
- Add a hybrid fallback strategy for long patterns: BMH → first/last UTF-16 code unit scanner → verified Rabin–Karp.
- Refactor BMH verification to a shared
string_find_match_rangehelper and remove the previous long-pattern TODO. - Add regression tests for dense fallback cases and add a dedicated benchmark suite for
find/rev_find.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| builtin/string_methods.mbt | Implements the new fallback strategy and shared matching/hash helpers for find/rev_find. |
| builtin/string_test.mbt | Adds targeted tests covering dense-miss and view-offset cases for the new fallbacks. |
| builtin/string_find_bench_test.mbt | Introduces benchmarks to measure dense and non-dense find/rev_find performance across targets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e6b8d8ac2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if failures > string_find_max_bruteforce || | ||
| failures > string_find_cutover(i) { | ||
| return string_find_by_code_unit_scanner_from(haystack, needle, i + 1) |
There was a problem hiding this comment.
When the needle is long and the bad-character skip is large, this counts every ordinary BMH miss toward the fallback and trips after 65 probes because of string_find_max_bruteforce, even if there were no dense false positives. For example, a 4 KiB needle whose terminal code unit never appears in a 1 MiB haystack used to finish with roughly n / needle_len BMH windows, but now switches to the code-unit scanner after about 266 KiB and checks the remaining hundreds of thousands of positions one by one. Consider only cutting over on small skips or partial first/last collisions so rare-miss long patterns keep the BMH fast path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged. I am keeping this PR on hold for now because Yu-zh plans to add String i16x8 read intrinsics, which should give us a better path to evaluate this search optimization without tuning the current fallback heuristic further.
|
I plan to add intrinsics for reading i16x8 from String. So let's put this on hold for now. |
e6b8d8a to
04844c4
Compare
|
Thanks, that makes sense. I rebased the branch, but will keep this PR on hold until the String i16x8 intrinsic path is available for comparison. |
04844c4 to
c7699f6
Compare
c7699f6 to
fea3438
Compare
fea3438 to
3bb219f
Compare
Superseded by the SIMD two-anchor search — with one idea worth carrying forwardThanks for this work — the diagnosis (BMH degrading on dense partial matches) and the first/last-code-unit scanner direction were right. Since this PR was written, main landed SIMD string search (5561cc4, 9b1a415 "two-anchor SIMD", e2afdce), which is the vectorized form of the same first/last-anchor idea. Measured on this PR's own headline workload (all-
Main is ~4x faster than this branch's optimized version on the case it targets (and near the single-code-unit SIMD scan floor of 262 ns), so rebasing this branch would replace faster SIMD code with a scalar scanner — closing as superseded. One piece of this PR's design is not covered by main, though: the guaranteed-linear fallback. The two-anchor scheme's true worst case — a needle whose anchors and a long prefix match at every position (e.g. Analysis and measurements by Claude Code on behalf of @bobzhang. |
|
The follow-up landed as promised: #4119 adds the guaranteed-linear (KMP) fallback behind the SIMD two-anchor search — the remaining piece of this PR's design, with credit. Adversarial worst case: native 48.6µs → 6.1µs at n=4096/m=64 and 7.2ms → 91µs at n=65536/m=512 (the O(n·m) → O(n+m) signature), fast paths untouched. |
The SIMD two-anchor find/rev_find already cut over to a fallback after
enough dense false-anchor candidates, but that fallback was a naive
scalar scan — still O(target * pattern) on adversarial inputs where
both anchors and long pattern prefixes recur throughout the target
(e.g. needle 'a'*32 + 'Z' + 'a'*31 over all-'a' text). This replaces
the naive fallback with KMP, making find/rev_find O(target + pattern)
in the worst case: the existing cutover budget trips on the 65th
failed verification (`failures > 64`), capping pre-fallback
verification at O(pattern) work, and the fallback itself
is linear. The failure-table allocation is paid only after cutover,
i.e. exclusively on pathological inputs. Resolves the standing TODO on
find/rev_find ("consider using Two-Way algorithm to ensure linear time
complexity") — KMP behind the existing budget gives the same guarantee
with less machinery.
The reverse fallback runs forward KMP over the prefix that can still
contain a hit and keeps the rightmost match, continuing through
overlaps via the failure table.
The idea comes from PR #3786 (mizchi), whose dense-case analysis and
verified-fallback design predate the SIMD two-anchor work that
superseded its scanner; this lands its remaining piece.
Benchmarks (moon bench --release, main -> this branch):
adversary find m=64 n=4096: native 48.6 -> 6.1us (8.0x),
js 453 -> 27.3us (16.6x), wasm-gc 112 -> 10.6us (10.6x)
adversary rev_find m=64 n=4096: native 48.8 -> 7.0us,
js 456 -> 27.4us, wasm-gc 112 -> 12.4us
scaling adversary m=512 n=65536: native 7.20ms -> 91us (79x),
js 56.2ms -> 433us (130x), wasm-gc 15.0ms -> 175us (86x) —
the O(n*m) -> O(n+m) signature
fast paths (dense miss, rare hit): par on all backends; the SIMD
two-anchor path is untouched.
Tests: whitebox KMP tests over periodic/overlapping patterns and
boundary starts; blackbox end-to-end tests built from decoy blocks that
each contribute exactly one false-anchor candidate, so both directions
provably exhaust the 65-failure budget before the interesting region —
dense-anchor misses, hits beyond the cutover point (including a first
occurrence that straddles the decoy/hits boundary), reverse cutover
resolving overlapping hits to the rightmost start, and forward/reverse
naive-reference cross-checks over fast-path and fallback
configurations. string_find_adversary_bench_test.mbt commits the
adversarial workloads so the numbers above are reproducible.
Mutation-verified: zeroing the failure table fails 2 tests; dropping
the overlap continuation in the reverse fallback fails 2.
Codex CLI review (ultra, round 1): production implementation verified
sound by exhaustive binary-alphabet modeling (267M helper-contract
cases, 89M full-route cases, no counterexample vs naive search),
including the handoff off-by-ones and the skipped-position reasoning;
three P2 findings all in tests/prose — a whitebox test argument that
violated the reverse helper's bound (out-of-bounds unsafe_get, fixed
to 7 and the precondition documented), an overlap test that never
reached the fallback (rebuilt with decoy blocks), and overstated
naive-reference coverage (rebuilt with both directions and both
paths).
Copilot review addressed: the cutover comments now describe BOTH
triggers — the early ratio check (failures > 4 + scanned/8, tripping
as soon as the 5th failure for candidates packed near the scan start)
and the hard cap (failures > 64) — and the fallback-test header notes
its spaced decoys deterministically force the hard-cap path.
QuickCheck added (string_find_quickcheck_test.mbt): a general
random-text/needle equivalence property against the naive references
(dense two-symbol texts randomly exercise the early ratio cutover),
and a fallback-forcing property that surrounds a randomized hit/decoy
mixture with 70 corrupted blocks per side (anchors intact, one
interior char flipped), fuzzing the KMP fallback itself with
randomized needle shapes. Zeroing the failure table is falsified by
the property after 2 cases.
Round 2 (ultra) verified the fixes: "All three P2 fixes are correct
and complete. Whitebox bound is safely 7, with the precondition
documented. Decoy arithmetic, forward result 5699, reverse result 41,
and 65th-failure cutovers check out. Naive references use correct
inclusive bounds and cover fast/fallback paths in both directions."
Round 3 (ultra) verified the Copilot-prompted prose and the QuickCheck
layer: "Ratio cutover can first occur on failure 5 when scanned < 8;
cutover is always by failure 65. Period-82 decoys never trigger the
ratio branch and deterministically hit the hard cap. All 2,835 valid
corrupted-block constructions preserve anchors, differ from the needle,
and cannot form accidental cross-block matches. Both directions force
fallback before the mixture."
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex CLI <codex@openai.com>
Signed-off-by: Codex CLI <codex@openai.com>
The SIMD two-anchor find/rev_find already cut over to a fallback after
enough dense false-anchor candidates, but that fallback was a naive
scalar scan — still O(target * pattern) on adversarial inputs where
both anchors and long pattern prefixes recur throughout the target
(e.g. needle 'a'*32 + 'Z' + 'a'*31 over all-'a' text). This replaces
the naive fallback with KMP, making find/rev_find O(target + pattern)
in the worst case: the existing cutover budget trips on the 65th
failed verification (`failures > 64`), capping pre-fallback
verification at O(pattern) work, and the fallback itself
is linear. The failure-table allocation is paid only after cutover,
i.e. exclusively on pathological inputs. Resolves the standing TODO on
find/rev_find ("consider using Two-Way algorithm to ensure linear time
complexity") — KMP behind the existing budget gives the same guarantee
with less machinery.
The reverse fallback runs forward KMP over the prefix that can still
contain a hit and keeps the rightmost match, continuing through
overlaps via the failure table.
The idea comes from PR #3786 (mizchi), whose dense-case analysis and
verified-fallback design predate the SIMD two-anchor work that
superseded its scanner; this lands its remaining piece.
Benchmarks (moon bench --release, main -> this branch):
adversary find m=64 n=4096: native 48.6 -> 6.1us (8.0x),
js 453 -> 27.3us (16.6x), wasm-gc 112 -> 10.6us (10.6x)
adversary rev_find m=64 n=4096: native 48.8 -> 7.0us,
js 456 -> 27.4us, wasm-gc 112 -> 12.4us
scaling adversary m=512 n=65536: native 7.20ms -> 91us (79x),
js 56.2ms -> 433us (130x), wasm-gc 15.0ms -> 175us (86x) —
the O(n*m) -> O(n+m) signature
fast paths (dense miss, rare hit): par on all backends; the SIMD
two-anchor path is untouched.
Tests: whitebox KMP tests over periodic/overlapping patterns and
boundary starts; blackbox end-to-end tests built from decoy blocks that
each contribute exactly one false-anchor candidate, so both directions
provably exhaust the 65-failure budget before the interesting region —
dense-anchor misses, hits beyond the cutover point (including a first
occurrence that straddles the decoy/hits boundary), reverse cutover
resolving overlapping hits to the rightmost start, and forward/reverse
naive-reference cross-checks over fast-path and fallback
configurations. string_find_adversary_bench_test.mbt commits the
adversarial workloads so the numbers above are reproducible.
Mutation-verified: zeroing the failure table fails 2 tests; dropping
the overlap continuation in the reverse fallback fails 2.
Codex CLI review (ultra, round 1): production implementation verified
sound by exhaustive binary-alphabet modeling (267M helper-contract
cases, 89M full-route cases, no counterexample vs naive search),
including the handoff off-by-ones and the skipped-position reasoning;
three P2 findings all in tests/prose — a whitebox test argument that
violated the reverse helper's bound (out-of-bounds unsafe_get, fixed
to 7 and the precondition documented), an overlap test that never
reached the fallback (rebuilt with decoy blocks), and overstated
naive-reference coverage (rebuilt with both directions and both
paths).
Copilot review addressed: the cutover comments now describe BOTH
triggers — the early ratio check (failures > 4 + scanned/8, tripping
as soon as the 5th failure for candidates packed near the scan start)
and the hard cap (failures > 64) — and the fallback-test header notes
its spaced decoys deterministically force the hard-cap path.
QuickCheck added (string_find_quickcheck_test.mbt): a general
random-text/needle equivalence property against the naive references
(dense two-symbol texts randomly exercise the early ratio cutover),
and a fallback-forcing property that surrounds a randomized hit/decoy
mixture with 70 corrupted blocks per side (anchors intact, one
interior char flipped), fuzzing the KMP fallback itself with
randomized needle shapes. Zeroing the failure table is falsified by
the property after 2 cases.
Round 2 (ultra) verified the fixes: "All three P2 fixes are correct
and complete. Whitebox bound is safely 7, with the precondition
documented. Decoy arithmetic, forward result 5699, reverse result 41,
and 65th-failure cutovers check out. Naive references use correct
inclusive bounds and cover fast/fallback paths in both directions."
Round 3 (ultra) verified the Copilot-prompted prose and the QuickCheck
layer: "Ratio cutover can first occur on failure 5 when scanned < 8;
cutover is always by failure 65. Period-82 decoys never trigger the
ratio branch and deterministically hit the hard cap. All 2,835 valid
corrupted-block constructions preserve anchors, differ from the needle,
and cannot form accidental cross-block matches. Both directions force
fallback before the mixture."
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Codex CLI <codex@openai.com>
Signed-off-by: Codex CLI <codex@openai.com>
Summary
Optimize
StringView::findandStringView::rev_findfor dense false-positive substring searches.The existing BMH path can degrade when the skip distance is 1 and every candidate partially matches. This keeps the BMH fast path for common cases, then switches to a first/last UTF-16 code unit scanner after enough failed verifications, with a verified Rabin-Karp fallback for repeated candidate collisions.
This also removes the old long-pattern TODO because the implementation now has a bounded fallback path for the problematic dense cases.
Benchmarks
Command:
moon bench -p moonbitlang/core/builtin -f string_find_bench_test.mbt --target <target> --releasefind substring dense miss n=4096rev_find substring dense miss n=4096find substring dense miss n=4096rev_find substring dense miss n=4096find substring dense miss n=4096rev_find substring dense miss n=4096find substring dense miss n=4096rev_find substring dense miss n=4096Representative non-dense native cases stayed around the same range:
find single code unit hit end n=4096find single code unit miss n=4096find substring rare hit end n=4096rev_find substring rare hit start n=4096Validation
moon fmtmoon test builtin string --target allmoon check builtin string --target all --no-rendermoon infomoon bench -p moonbitlang/core/builtin -f string_find_bench_test.mbt --target native --releasemoon bench -p moonbitlang/core/builtin -f string_find_bench_test.mbt --target wasm --releasemoon bench -p moonbitlang/core/builtin -f string_find_bench_test.mbt --target wasm-gc --releasemoon bench -p moonbitlang/core/builtin -f string_find_bench_test.mbt --target js --release