perf: use bytes.Index for substring search
#104
Conversation
dkharms
added
benchmarks
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #104 +/- ##
==========================================
- Coverage 71.50% 71.23% -0.27%
==========================================
Files 201 200 -1
Lines 18170 18143 -27
==========================================
- Hits 12993 12925 -68
- Misses 4457 4500 +43
+ Partials 720 718 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughReplaces KMP-based substring matcher with a bytes.Index-based findSequence, removes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (4 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (4)
pattern/substring.go (1)
7-16: Tighten the loop and avoid double indexing.Minor cleanup and a tiny win: range with index+value and use
valforbytes.Index. No behavior change.-func findSequence(haystack []byte, needles [][]byte) int { - for cur := range needles { - val := needles[cur] - start := bytes.Index(haystack, needles[cur]) - if start == -1 { - return cur - } - haystack = haystack[start+len(val):] - } - return len(needles) -} +func findSequence(haystack []byte, needles [][]byte) int { + for i, val := range needles { + start := bytes.Index(haystack, val) + if start == -1 { + return i + } + haystack = haystack[start+len(val):] + } + return len(needles) +}Do we ever expect empty needles? If yes, current logic treats them as “found” without advancing. If that’s undesirable, we should skip/forbid empties and add a unit test.
pattern/pattern.go (3)
78-81: Unsafe aliasing needs lifetime pinning (keep original strings alive).
middlenow holds[][]byteproduced via an unsafe string→[]byte view. To avoid GC lifetime hazards and future accidental mutation, keep the original strings alongside the byte slices.type wildcardSearch struct { baseSearch prefix []byte suffix []byte - middle [][]byte + middle [][]byte + // keep original strings to pin GC lifetime when using unsafe conversions + middleKeepAlive []string middleLen int narrowed bool }
99-102: Pin the source strings when using StringToByteUnsafe.Store the source string to ensure it stays alive for the duration of the search. Also document the read-only invariant.
- val := util.StringToByteUnsafe(terms[i].Data) - s.middle = append(s.middle, val) - s.middleLen += len(val) + str := terms[i].Data + val := util.StringToByteUnsafe(str) // read-only view; do not mutate + s.middle = append(s.middle, val) + s.middleKeepAlive = append(s.middleKeepAlive, str) // pin lifetime + s.middleLen += len(val)If avoiding any unsafe GC nuance is preferred, switch to a safe copy:
- val := util.StringToByteUnsafe(terms[i].Data) + val := []byte(terms[i].Data) // safe copy
151-152: Guard slice bounds locally (defensive check).
checkMiddleguards length globally, but a local bound check keepsfindSequencecall obviously safe and future-proof if conditions change.- return findSequence(val[len(s.prefix):len(val)-len(s.suffix)], s.middle) == len(s.middle) + lo, hi := len(s.prefix), len(val)-len(s.suffix) + if hi < lo { + return false + } + return findSequence(val[lo:hi], s.middle) == len(s.middle)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pattern/pattern.go(2 hunks)pattern/substring.go(1 hunks)pattern/substring_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
pattern/substring_test.go (3)
24-40: Tests look good.Nice coverage on order, overlaps, and corner growth cases.
125-131: LGTM.Simple and fast random byte generator.
133-135: LGTM.Nice helper for readability in deterministic cases.
dkharms
force-pushed
the
98-bytes-index
branch
from
2b9cefd to
abb9bb5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
pattern/substring_test.go (3)
72-79: Use the canonical benchmark loop; drop redundant conversion.Replace
b.Loop()and[]byte(c.haystack).- b.Run(tc.name+"-"+strconv.Itoa(i), func(b *testing.B) { - for b.Loop() { - findSequence([]byte(c.haystack), c.needles) - } - }) + b.Run(tc.name+"-"+strconv.Itoa(i), func(b *testing.B) { + b.ReportAllocs() + for i := 0; i < b.N; i++ { + findSequence(c.haystack, c.needles) + } + })
97-108: Stabilize random bench; set bytes once.Move
SetBytesout of the loop and use the canonical loop.b.Run(size.name, func(b *testing.B) { haystack, needles := generateTestData( size.haystackSize, size.needleSize, size.needleCount, 256, ) - b.ResetTimer() - for b.Loop() { - findSequence(haystack, needles) - b.SetBytes(int64(len(haystack))) - } + b.ReportAllocs() + b.SetBytes(int64(len(haystack))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + findSequence(haystack, needles) + } })
111-123: Fix compile error and bound checks in data generator.Looping over
intwithrangedoesn’t compile;rand.Intn(len(haystack)-needleSize)can panic; copy must be bounded.func generateTestData(haystackSize, needleSize, needleCount, charset int) ([]byte, [][]byte) { haystack := generateRandomBytes(haystackSize, charset) needles := make([][]byte, needleCount) - for i := range needleCount { - pattern := generateRandomBytes(needleSize, charset) - pos := rand.Intn(len(haystack) - needleSize) - copy(haystack[pos:], pattern) - needles[i] = pattern - } + if needleSize > haystackSize { + needleSize = haystackSize + } + for i := 0; i < needleCount; i++ { + pattern := generateRandomBytes(needleSize, charset) + if needleSize > 0 { + pos := rand.Intn(len(haystack)-needleSize+1) + copy(haystack[pos:pos+needleSize], pattern) + } + needles[i] = pattern + } return haystack, needles }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
pattern/pattern.go(2 hunks)pattern/substring.go(1 hunks)pattern/substring_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pattern/substring.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
pattern/pattern.go (1)
342-349: Ignore empty-range wrap-around risk
baseSearch.first isint(tp.FirstTID())(≥1), andutil.BinSearchInRangenever returns below that, so on “not found”s.last = s.first - 1yields
s.last ≥ 0. Casting these non-negative ints touint32can’t wrap.Likely an incorrect or invalid review comment.
dkharms
force-pushed
the
98-bytes-index
branch
from
abb9bb5 to
c3b99d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pattern/pattern_test.go (3)
487-515: Good new coverage; consider a couple more edge tests.Add cases for empty haystack and/or empty needles to lock in boundary behavior (e.g., [], ["a"], [""]).
517-556: Nit: avoid redundant []byte conversion; optionally set bytes once.
c.haystackis already []byte. Also it helps to set bytes outside the loop.Apply this diff:
- b.Run(tc.name+"-"+strconv.Itoa(i), func(b *testing.B) { - for b.Loop() { - findSequence([]byte(c.haystack), c.needles) - } - }) + b.Run(tc.name+"-"+strconv.Itoa(i), func(b *testing.B) { + b.SetBytes(int64(len(c.haystack))) + for b.Loop() { + findSequence(c.haystack, c.needles) + } + })
558-584: SetBytes should be outside the loop.It’s constant per sub-benchmark; moving it reduces per-iter overhead.
Apply this diff:
- b.Run(size.name, func(b *testing.B) { + b.Run(size.name, func(b *testing.B) { haystack, needles := generateTestData( size.haystackSize, size.needleSize, size.needleCount, 256, ) - b.ResetTimer() + b.SetBytes(int64(len(haystack))) + b.ResetTimer() for b.Loop() { findSequence(haystack, needles) - b.SetBytes(int64(len(haystack))) } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pattern/pattern.go(3 hunks)pattern/pattern_test.go(2 hunks)pattern/substring.go(0 hunks)pattern/substring_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- pattern/substring_test.go
- pattern/substring.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pattern/pattern.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
pattern/pattern_test.go (4)
9-9: Import looks correct.Needed for strings.Repeat in tests/benchmarks.
600-607: LGTM.Simple, deterministic byte generator for the given charset.
608-610: LGTM.Handy helper to keep benchmarks tidy.
517-556: No action needed: go.mod and CI workflows already require Go 1.24+
dkharms
force-pushed
the
98-bytes-index
branch
from
c3b99d1 to
258a412
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pattern/pattern_test.go (1)
586-598: Fix compile error and off-by-one; guard oversized needles in generateTestData.Looping over an int won’t compile; current pos calc can panic and excludes the last valid start.
func generateTestData(haystackSize, needleSize, needleCount, charset int) ([]byte, [][]byte) { haystack := generateRandomBytes(haystackSize, charset) needles := make([][]byte, needleCount) - for i := range needleCount { - pattern := generateRandomBytes(needleSize, charset) - pos := rand.Intn(len(haystack) - needleSize) + for i := 0; i < needleCount; i++ { + plen := needleSize + if plen > len(haystack) { + plen = len(haystack) + } + pattern := generateRandomBytes(plen, charset) + maxStart := len(haystack) - plen + pos := 0 + if maxStart > 0 { + pos = rand.Intn(maxStart + 1) + } copy(haystack[pos:], pattern) needles[i] = pattern } return haystack, needles }
🧹 Nitpick comments (1)
pattern/pattern_test.go (1)
487-494: Preallocate needlesB to avoid slice growth (tiny test micro-opt).Pre-sizing eliminates reallocations in the helper.
-func testFindSequence(a *assert.Assertions, cnt int, needles []string, haystack string) { - var needlesB [][]byte - for _, needle := range needles { - needlesB = append(needlesB, []byte(needle)) - } +func testFindSequence(a *assert.Assertions, cnt int, needles []string, haystack string) { + needlesB := make([][]byte, len(needles)) + for i, needle := range needles { + needlesB[i] = []byte(needle) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pattern/pattern.go(3 hunks)pattern/pattern_test.go(2 hunks)pattern/substring.go(0 hunks)pattern/substring_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- pattern/substring_test.go
- pattern/substring.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pattern/pattern.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
dkharms
force-pushed
the
98-bytes-index
branch
from
258a412 to
098bcb7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
pattern/pattern_test.go (3)
517-556: Fix non-existent testing.B.Loop and drop redundant []byte cast; set bytes once.This won’t compile (b.Loop doesn’t exist). Also []byte(c.haystack) is redundant; c.haystack is already []byte.
for _, tc := range testCases { for i, c := range tc.cases { b.Run(tc.name+"-"+strconv.Itoa(i), func(b *testing.B) { - for b.Loop() { - findSequence([]byte(c.haystack), c.needles) - } + b.SetBytes(int64(len(c.haystack))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + findSequence(c.haystack, c.needles) + } }) } }
558-584: Fix benchmark loop and SetBytes placement.Use the standard b.N loop; move SetBytes outside the per-iteration body.
b.Run(size.name, func(b *testing.B) { haystack, needles := generateTestData( size.haystackSize, size.needleSize, size.needleCount, 256, ) - b.ResetTimer() - for b.Loop() { - findSequence(haystack, needles) - b.SetBytes(int64(len(haystack))) - } + b.SetBytes(int64(len(haystack))) + b.ResetTimer() + for i := 0; i < b.N; i++ { + findSequence(haystack, needles) + } })
586-598: Fix compile error, off-by-one, and guard oversize needles.
- for i := range needleCount won’t compile (cannot range over int).
- rand.Intn(len(haystack)-needleSize) can panic and excludes the last valid start.
- Allow needles longer than haystack by clamping length.
func generateTestData(haystackSize, needleSize, needleCount, charset int) ([]byte, [][]byte) { haystack := generateRandomBytes(haystackSize, charset) needles := make([][]byte, needleCount) - for i := range needleCount { - pattern := generateRandomBytes(needleSize, charset) - pos := rand.Intn(len(haystack) - needleSize) + for i := 0; i < needleCount; i++ { + plen := needleSize + if plen > len(haystack) { + plen = len(haystack) + } + pattern := generateRandomBytes(plen, charset) + maxStart := len(haystack) - plen + pos := 0 + if maxStart > 0 { + pos = rand.Intn(maxStart + 1) // include last valid index + } copy(haystack[pos:], pattern) needles[i] = pattern } return haystack, needles }
🧹 Nitpick comments (1)
pattern/pattern_test.go (1)
487-495: Helper looks good; preallocate slice capacity.Minor nit: avoid reallocations when building needlesB.
func testFindSequence(a *assert.Assertions, cnt int, needles []string, haystack string) { - var needlesB [][]byte + needlesB := make([][]byte, 0, len(needles)) for _, needle := range needles { needlesB = append(needlesB, []byte(needle)) } res := findSequence([]byte(haystack), needlesB) a.Equal(cnt, res, "wrong total number of matches") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pattern/pattern.go(3 hunks)pattern/pattern_test.go(2 hunks)pattern/substring.go(0 hunks)pattern/substring_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- pattern/substring_test.go
- pattern/substring.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pattern/pattern.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
pattern/pattern_test.go (4)
9-9: Import of strings is appropriate.Used by strings.Repeat in tests/benchmarks. LGTM.
496-515: Tests cover useful cases.Nice spread of scenarios; no issues spotted.
600-606: LGTM.Simple, efficient generator for benchmarks.
608-610: LGTM.Tiny helper keeps test cases readable.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
dkharms
force-pushed
the
98-bytes-index
branch
from
098bcb7 to
fe6da01
Compare
Description
Here's the benchmarks for KMP and
bytes.Index(I excluded comparison of heap allocation since both version had zero heap allocations).So we see that on regular cases we have around 53% of speedup by using
bytes.Index. And on large strings we see around of 90% speedup (I believe that's because we build LPS for needles, not the haystack).Here are features of my CPU (it supports both SSE4.2 and AVX2):
Closes #98
If you have used LLM/AI assistance please provide model name and full prompt:
Summary by CodeRabbit
Refactor
Tests