⚡ perf: Add support for SWAR processing#230
Conversation
Convert the ASCII case-conversion hot paths from per-byte table lookups to SWAR (SIMD within a register) processing of eight bytes per uint64: - internal/caseconv: add UpperMask/LowerMask/ToUpperWord/ToLowerWord bit tricks plus first-index scans, copy converters, and in-place converters built on them. Scans and copy converters finish with one overlapping word; the in-place converters finish byte-wise because an overlapping word there would stall on store-to-load forwarding. - strings, bytes: route ToLower/ToUpper/UnsafeToLower/UnsafeToUpper through the SWAR helpers for inputs of 8+ bytes, keeping the byte-wise path for shorter inputs where call overhead dominates. - EqualFold: compare eight bytes per iteration by upper-casing both words, with a single overlapping word for the tail. - ToString: build []string and []int output with a single exactly- or tightly-sized []byte and AppendInt instead of strings.Builder plus per-element FormatInt strings. Benchmarks (linux/amd64, Xeon 2.10GHz): - EqualFold / EqualFoldBytes: -18% to -20% - ToLower/ToUpper (strings+bytes) 64B inputs: -21% to -66% - unchanged-input scans (e.g. already-lowercase ToLower): up to -66% - ToString []string -22%, []int -9%, [][]int/[]any -6% to -12% - short inputs (<8B) keep their previous byte-wise fast path Adds a 20k-trial randomized test comparing all SWAR paths against reference implementations across lengths 0-69, including non-ASCII bytes and case-variant pairs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
Follow-up optimizations from adversarial code review of the SWAR change: - byteseq.go: pin loadWord reads through an 8-byte reslice so the constant-index loads are bounds-check free; the generated code for EqualFold[string] drops 8 per-byte compare+branch pairs per word. Also skip both SWAR case-folds when the raw words already match, which speeds up byte-identical comparisons (the common outcome for canonical header/method checks) by ~34% at the cost of one predictable branch when case differs. - internal/caseconv: use constant-length b[i:i+8] windows in every word loop, removing the redundant per-iteration inner bounds check; export WordLen as the single source of truth for the word size and derive the strings/bytes swarMinLen constants and alignment masks from it; document the full ToLowerCopy/ToUpperCopy precondition on from (word-aligned, at or below the first convertible byte). - convert.go/format.go: size the ToString([]int) buffer exactly using a new loop-free uintDigits/intDigits (bits.Len64 log10 technique, inlinable), guaranteeing a single allocation for any element magnitude instead of reallocating on 4+ digit or negative values. - tests: extend the randomized differential suite to the strings in-place wrappers (over mutable backing arrays) and to the caseconv helpers' sub-word tails; add exhaustive digit-count tests covering 0..100k, every power-of-ten boundary, MinInt64/MaxInt64. Benchmarks vs pre-SWAR base (linux/amd64, median of 10): - EqualFold strings -36%, bytes -21%; byte-identical inputs -34% more - ToLower/ToUpper 64B: -21% to -67%; in-place variants -23% to -33% - ToString: []string -21%, []int -5% with exact single allocation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
Implements the SWAR utilities plan driven by gofiber/fiber's hot paths. P1 — new public package github.com/gofiber/utils/v2/swar exposing the word-at-a-time primitives so fiber and middleware can compose fused scans: Load8 (generic, no unsafe, verified to fuse into a single 8-byte load for both string and []byte shapes), ToLowerWord/ToUpperWord (fold only A-Z/a-z lanes, all other bytes incl >= 0x80 pass through), MatchByteMask (per-lane exact, safe for LastLane and lane masking), MatchRangeMask, FirstLane, LastLane, WordLen. internal/caseconv and EqualFold now delegate to these primitives instead of carrying their own copies of the bit tricks; generated code is unchanged. P2 — IndexAny2/IndexAny3: first index of any of 2/3 needle bytes. vs tight scalar loops: -46% to -56% (2 needles) and -8% to -42% (3 needles) at 8-512B; also 2-3x faster than strings.IndexAny. P3 — IndexFold/ContainsFold: ASCII case-insensitive substring search. Word path for needles <= 8B with lazily built folded-needle word and first-byte candidate masking; scalar path otherwise. -56% to -69% on Cache-Control-shaped workloads; the only non-win is a single-word miss (+3ns absolute). Includes the mandatory "no\rcache" vs "no-cache" negative test: only letter lanes fold, CR|0x20=='-' can never match. P4 — IsASCII (-27% to -69% at 8B+) and IndexNonQuotable (first byte needing RFC 9110 quoted-string escaping; -36% to -38% at 8B+ after folding controls and DEL into one biased range test). P5 — SWAR Trim family: DROPPED per the benchmark gate. On the shapes fiber actually trims (0-2 pad bytes: ", " separators, XFF segments, media ranges) the scalar loops win by 30-150%; SWAR only pays at 8+ pad bytes, which does not occur in practice. Scalar trims stay. P6 — IsIPv4/IsIPv6 SWAR assist: DROPPED per the benchmark gate. Word pre-validation lost on every shape (+10% to +49%) because the scalar octet math still has to run afterwards on <= 15-byte inputs. P7 — ParseUint/ParseInt now consume 8 digits per word (range-mask validate, then 3-multiply pairwise combine) for digit runs of 8+, dispatched from parseSigned/parseUnsigned so parseDigits stays under the inline budget: 8/12/19-digit parses -16%/-8%/-27%, 4-6 digit neutral to -12%, 1-digit +0.6ns (one extra length branch). Tests: exhaustive per-lane oracles over all 256 byte values (including the cross-lane borrow trap for MatchByteMask), randomized differential suites vs scalar references at lengths 0..40 for every new API, parse parity vs strconv over 200k random values plus error/overflow parity cases, and a load-fusion guard benchmark. All new paths allocate zero bytes across 78 benchmark rows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
Fixes from adversarial review of the swar/search/parse commit; no correctness findings survived three independent trace angles, so this round is efficiency and structure: - search: switch IndexAny2/IndexAny3/IndexFold candidate scans to the first-match zero-detect mask (shared zeroLanes helper, two ops cheaper per needle than the exact MatchByteMask, with the borrow direction documented); hoist needle broadcasts out of the word loops and behind the n >= 8 branch so sub-word inputs skip the multiplies. - IndexFold: match the first byte's two case variants directly instead of folding every haystack word, replace the lenMask sentinel with an explicit built flag, and extend the SWAR candidate scan to needles longer than 8 bytes (word-wise fold-compare verify) — real header names like proxy-authorization previously fell back to the fully scalar loop. Long-needle scans now beat scalar by 67-70%. - IsASCII: OR four words per iteration before testing (no index is needed on the reject path), -24% to -39% further at 64B+. - parse: replace the range-mask digit validation with the high-nibble isEightDigits check (fewer ops, and parse.go no longer reaches into ascii.go's constants); drop parseDigits' unreachable overflow guard (both callers dispatch 8+ byte runs to parseDigitsBig) and document the precondition instead. - swar: export Ones/HighBits/LowSeven so callers stop re-declaring the lane constants; make ToUpperWord's bit-clear explicit (&^) to mirror ToLowerWord's bit-set. - tests: slim the parse gate file into permanent parity tests against strconv (now also asserting NumError shape and Func), exhaustive isEightDigits lane coverage including the 0xFA+ carry shapes, borrow-stress IndexNonQuotable cases, and long-needle IndexFold fixtures and benchmarks. Same-process medians vs scalar references after this round: - IndexAny2 -47% to -63% (8-512B), IndexAny3 -26% to -60% - IndexFold -61% to -77%, long needles -67% to -70% (was scalar) - IsASCII -17% to -78%; IndexNonQuotable -31% to -38% - ParseUint digit runs improve slightly across 1-19 digits - Sub-word inputs: within ~1ns of the scalar baseline everywhere Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
Second review round: the correctness pass refuted every candidate (the approximate-mask first-match argument, foldEqualAt preconditions, isEightDigits carry exactness, IsASCII stride coverage, and parse test parity were each verified with concrete traces), so this round is polish only: - swar: export ZeroLanes as the documented first-match primitive so downstream fused scans don't re-derive it; cross-reference it from MatchByteMask's doc; add a property test that its lowest set lane is always a true zero across all fills and positions. - search/ascii: use swar.ZeroLanes directly (drop the private helper); express quoteLanes/backslashLanes as uint64(c)*swar.Ones constant expressions instead of hand-typed hex; fix the module comment that wrongly implied IndexFold's candidate scan uses the n-8 overlap tail. - IndexFold: a closure-based dedup of the two verify blocks was measured (+3-4ns on every call from capture setup, even with no candidates) and rejected; a comment now records that decision and pins the two copies together. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
gaby
requested review from
ReneWerner87,
efectn and
sixcolors
and removed request for
a team
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #230 +/- ##
==========================================
+ Coverage 83.94% 88.98% +5.04%
==========================================
Files 14 17 +3
Lines 1177 1380 +203
==========================================
+ Hits 988 1228 +240
+ Misses 156 132 -24
+ Partials 33 20 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds reusable SWAR primitives and applies them to ASCII detection, quoted-string scanning, search, case conversion, equality, integer parsing, and slice formatting. It also adds extensive tests, fuzz targets, benchmarks, contributor guidance, and refreshed benchmark documentation. ChangesSWAR utility expansion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant IndexFold
participant swar
Caller->>IndexFold: provide haystack and needle
IndexFold->>swar: load and fold 8-byte windows
swar-->>IndexFold: return lane match masks
IndexFold-->>Caller: return matching index or -1
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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 `@ascii.go`:
- Around line 46-52: Update IndexNonQuotable and both scanning paths to treat
HTAB (0x09) as quotable: classify control bytes as non-quotable only when below
0x20 and not equal to '\t', while preserving backslash, quote, and DEL handling.
Keep the documented RFC predicate and ensure both the word-wise and byte-wise
implementations behave consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23d680d0-b55e-4e3c-bedb-b3ed89a2fa85
📒 Files selected for processing (15)
ascii.gobytes/case.gobyteseq.goconvert.goformat.goformat_test.gointernal/caseconv/swar.goparse.goparse_swar_test.gosearch.gosearch_test.gostrings/case.goswar/swar.goswar/swar_test.goswar_verify_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: CodeRabbit
- GitHub Check: lint / lint
- GitHub Check: Compare
- GitHub Check: Build (1.25.x, windows-latest)
- GitHub Check: Build (1.26.x, windows-latest)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
swar/swar.go (1)
1-116: LGTM!internal/caseconv/swar.go (1)
1-150: LGTM!swar/swar_test.go (1)
1-196: LGTM!swar_verify_test.go (1)
1-118: LGTM!parse.go (1)
6-7: LGTM!SWAR digit parsing is correct:
isEightDigitsproperly rejects all non-digit bytes per lane (including 0xFA+ carry edge cases),parse8Digitsproduces the exact 8-digit value via pairwise multiply-combine, andparseDigitsBig's overflow check correctly fires only at the 20th+ digit since 19 digits always fit in uint64. TheParseIntfast path'slen(s)<=19guard ensures no uint64 overflow before the final int64 range check. Error wrapping inparseSigned/parseUnsignedis consistent with strconv's*NumErrorshape.Also applies to: 38-51, 162-222, 259-266, 295-303
parse_swar_test.go (1)
21-42: LGTM!Excellent test coverage: the exhaustive 256×8-lane sweep validates
isEightDigitsagainst every byte value per lane, the 200k-iteration randomized parity test covers all digit lengths, and the fixed edge-case list targets the SWAR/scalar handoff boundaries (8, 16, 19, 20 digits) and overflow/leading-zero/syntax-error shapes with full*NumErrorparity checks.Also applies to: 45-90, 92-111
format.go (1)
3-6: LGTM!
uintDigitscorrectly usesbits.Len64for a loop-free log10 approximation with a singlepow10correction, andintDigitssafely handlesmath.MinInt64viauint64(-n)two's complement conversion. Array access is always in bounds (t ∈ [1,19],pow10has 20 elements).Also applies to: 196-226
format_test.go (1)
342-370: LGTM!Thorough validation against
strconvas oracle, covering dense ranges, all power-of-ten boundaries up toMaxUint64, and explicitMinInt64/MaxInt64checks that exercise the two's complement edge case inintDigits.convert.go (1)
194-209: LGTM!Exact buffer sizing is correct:
len(v)+1accounts for brackets and inter-element spaces, and per-element byte/digit counts are exact.UnsafeStringis safe here sincebufis freshly allocated and never mutated after conversion. Theint64(n)conversion is consistent betweenintDigits(sizing) andAppendInt(formatting) across 32/64-bit platforms.Also applies to: 210-229
search_test.go (2)
197-197: Update this expectation with theascii.goHTAB fix.RFC 9110 allows this tab verbatim, so the expected result should be
-1. (rfc-editor.org)
1-196: LGTM!Also applies to: 198-375
ascii.go (1)
1-44: LGTM!search.go (1)
1-227: LGTM!bytes/case.go (1)
7-103: LGTM!strings/case.go (1)
8-94: LGTM!Also applies to: 101-110
byteseq.go (1)
5-5: LGTM!Also applies to: 14-44
Addresses the CodeRabbit review on PR #230 and the failing lint job: - IndexNonQuotable: RFC 9110 qdtext permits HTAB (HTAB / SP / %x21 / %x23-5B / %x5D-7E / obs-text), so stop classifying '\t' as non-quotable. The word path clears tab lanes from the control mask with the exact swar.MatchByteMask — an approximate mask could wrongly clear a genuine control lane above a tab — and both scalar paths gain the c != '\t' exclusion. The randomized suite's alphabet now includes HTAB, the scalar reference matches the new predicate, and fixed cases cover tabs in the word path, the scalar path, and directly before a real control byte. Measured cost is absorbed: IndexNonQuotable stays -36% to -43% vs scalar at 8B+. - foldNeedle now returns only the folded needle word, with the trivial length mask computed at the two call sites: CI revive (confusing-results) rejects two unnamed same-type results while nonamedreturns rejects naming them, so shrinking to a single result satisfies both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
Codecov measures each package only through its own test binary, so the cross-package randomized suite in the root package was not crediting bytes/case.go (46%) or strings/case.go (81%). Add in-package path-coverage tests: - bytes, strings: every dispatch path of ToLower/ToUpper and the Unsafe variants — sub-word inputs, word-path inputs with the first change at each interesting offset (start, past word 0, overlap tail only), non-ASCII passthrough, and the identity return for unchanged inputs (asserted to alias the input slice). Both packages now sit at 100% statement coverage. - search: cover IndexFold's lazy needle build when the first candidate only appears in the scalar remainder, and needles whose length is an exact word multiple in foldEqualAt. Coverage also exposed that the long-needle verify arm in the remainder loop was unreachable — the remainder is non-empty only when k <= n%8 <= 7, so a > 8-byte needle can never start there. The dead branch is removed and the bound documented. - parse: ParseInt fast-path word rejection, parseSigned's 8+ digit word dispatch (signed long inputs were untested anywhere), and the pre-existing ParseInt8/ParseUint8 sign, boundary, and fallback branches. parse.go is now fully covered. All additions are deterministic pure-computation tests, portable across the linux/macos/windows CI matrix. Patch files now measure 100% locally (root package 97.6% overall; the remainder is pre-existing code untouched by this PR). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
gaby
changed the title
gaby
changed the title
gaby
changed the title
…README Applies the review patch from the PR #230 second-pass report: no code blockers were found; this lands the accepted P1/P2 items and records the measured design decisions in code. swar package: - add Store8 (Load8's store counterpart, same lane order, fused write) and Broadcast (the documented needle form for ZeroLanes scans, used at all five search.go sites) - package docs now state the ZeroLanes approximation explicitly, the Load8/Store8 lane-order pairing, and why the FirstLane/LastLane empty-mask sentinels differ; godoc examples for the scan idiom, range classification, and lane order - Test_MatchRangeMask_Exhaustive now earns its name: all 8256 valid (lo, hi) ranges with boundary-value neighbor lanes plus 200k random words, pinning the cross-lane carry-freedom claim; Store8 round-trip and store-fusion benchmark leg added root package: - native fuzz targets (FuzzIndexFold, FuzzEqualFold, FuzzParse, FuzzIndexNonQuotable) against the scalar references with trap seeds; smoke runs 256k/507k execs clean - randomized lengths raised to 0..100 so IsASCII's 32-byte loop runs multiple iterations; direct from > 0 contract test for the caseconv copy helpers; 19-digit ParseInt fast-path boundary pinned (9223372036854775807/808 with ErrRange classification); 10k random words pin parse8Digits in isolation - IndexFold: foldPrep deduplicates the lazy build pair; the lazy structure itself stays, with the maintainer's benchstat numbers (eager build: +44% on 8B misses) recorded in the comment, as are the measured rejections of routing ParseInt through parseDigitsBig (+22%) and of sharing the caseconv loop pairs - private working-context references removed from comments docs: - README: new Search and ASCII sections, ParseUint digit-run benchmarks, all numbers regenerated in one run per README convention - AGENTS.md: review guardrails for this library (exported-without- in-module-callers is by design; benchstat methodology; conventions) Verified: full suite plus -race -shuffle=on green, fuzz smoke clean, IndexFold benchmarks unchanged after the foldPrep refactor, lint at the pre-existing baseline. Co-authored-by: ReneWerner87 <ReneWerner87@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
CI revive rejects two unnamed same-type results while nonamedreturns forbids naming them, so foldPrep now returns a small foldedNeedle struct (word + lane mask) instead of a (uint64, uint64) pair. The struct is register-passed; IndexFold benchmarks are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
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 (1)
search.go (1)
83-179: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd README coverage for
ContainsFold
README.mdalready includes the Search benchmarks forIndexAny2,IndexAny3, andIndexFold, butContainsFoldis missing. Add a matching entry so the exported search API set stays documented.🤖 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 `@search.go` around lines 83 - 179, Add a README benchmark entry for the exported ContainsFold API, matching the existing formatting and placement used for IndexAny2, IndexAny3, and IndexFold search benchmarks. Do not alter the IndexFold implementation.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (2)
README.md (1)
20-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBenchmark header uses
-count=1while AGENTS.md requires-count=10for performance claims.The README command at line 20 (
-count=1) generates single-run reference numbers for display. AGENTS.md line 17 requires benchstat with at least-count=10for verifying performance claims. These serve different purposes, but a contributor copying the README command to verify improvements would get insufficient data. Consider adding a note that the displayed numbers are single-run references and that performance claims require the benchstat workflow described in AGENTS.md.🤖 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 `@README.md` at line 20, Clarify the benchmark command in README.md by noting that its -count=1 results are single-run reference numbers. Point contributors to the AGENTS.md benchstat workflow, including -count=10, when validating performance claims.Source: Coding guidelines
search.go (1)
181-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd named return values to
foldPrepto clear revive lint failure.The
revivelinter flags unnamed results of the same type as confusing. Both returns areuint64, making call-site readability and godoc clarity worse.♻️ Proposed fix
-func foldPrep(needle string) (uint64, uint64) { +func foldPrep(needle string) (needleWord, lenMask uint64) { return foldNeedle(needle), ^uint64(0) >> ((8 - len(needle)) * 8) }🤖 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 `@search.go` around lines 181 - 186, Update the foldPrep function signature to use distinct named return values for its two uint64 results, clearly identifying the folded needle and length mask while preserving the existing returned values and call-site behavior.Source: Linters/SAST tools
🤖 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 `@README.md`:
- Around line 23-441: Update the README’s SWAR API catalog to include the
exported Store8 and Broadcast APIs, then regenerate or replace the adjacent SWAR
benchmark block with results from the same benchmark run. Keep the benchmark
formatting and existing section organization consistent.
---
Outside diff comments:
In `@search.go`:
- Around line 83-179: Add a README benchmark entry for the exported ContainsFold
API, matching the existing formatting and placement used for IndexAny2,
IndexAny3, and IndexFold search benchmarks. Do not alter the IndexFold
implementation.
---
Nitpick comments:
In `@README.md`:
- Line 20: Clarify the benchmark command in README.md by noting that its
-count=1 results are single-run reference numbers. Point contributors to the
AGENTS.md benchstat workflow, including -count=10, when validating performance
claims.
In `@search.go`:
- Around line 181-186: Update the foldPrep function signature to use distinct
named return values for its two uint64 results, clearly identifying the folded
needle and length mask while preserving the existing returned values and
call-site 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 248abb84-6179-4b5f-94fc-db26238b5f77
📒 Files selected for processing (12)
AGENTS.mdREADME.mdfuzz_test.gointernal/caseconv/swar.goparse.goparse_swar_test.gosearch.gosearch_test.goswar/example_test.goswar/swar.goswar/swar_test.goswar_verify_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- swar/swar.go
- parse_swar_test.go
- parse.go
- search_test.go
- internal/caseconv/swar.go
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: CodeRabbit
- GitHub Check: benchmark / benchmark
- GitHub Check: Build (1.25.x, windows-latest)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{go,md}
📄 CodeRabbit inference engine (AGENTS.md)
Code, comments, and PR text must always be written in English.
Files:
AGENTS.mdfuzz_test.goswar/example_test.goswar_verify_test.goREADME.mdswar/swar_test.gosearch.go
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Do not argue that exported functions or packages should move tointernal/solely because they have no non-test in-module callers; this repository exports helpers for downstream modules.
Performance claims for Go code must be verified withbenchstat, comparing base and head on the same machine with at least-count=10; single runs are insufficient.
Files:
fuzz_test.goswar/example_test.goswar_verify_test.goswar/swar_test.gosearch.go
README.md
📄 CodeRabbit inference engine (AGENTS.md)
README.md is a function catalog with benchmark blocks. New exported functions require a README section, and benchmark numbers for touched paths must be regenerated in the same run.
Files:
README.md
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: gofiber/utils
Timestamp: 2026-07-12T12:43:21.196Z
Learning: Evaluate new exported APIs by contract clarity, test coverage, naming consistency, and benchmark evidence for performance claims.
Learnt from: CR
Repo: gofiber/utils
Timestamp: 2026-07-12T12:43:21.196Z
Learning: Explicitly call out sharp-edged API contracts, including unchecked preconditions, approximate results, and unsafe memory semantics.
Learnt from: CR
Repo: gofiber/utils
Timestamp: 2026-07-12T12:43:21.196Z
Learning: Additive API is not a SemVer break, but it creates a long-term maintenance commitment.
🪛 GitHub Actions: Linter / 0_lint _ lint.txt
search.go
[error] 184-184: golangci-lint reported 'confusing-results: unnamed results of the same type may be confusing, consider using named results' (revive). Step: 'golangci-lint run'.
🪛 GitHub Actions: Linter / lint _ lint
search.go
[error] 184-184: golangci-lint (revive): confusing-results: unnamed results of the same type may be confusing, consider using named results.
🪛 GitHub Check: lint / lint
search.go
[failure] 184-184:
confusing-results: unnamed results of the same type may be confusing, consider using named results (revive)
🔇 Additional comments (9)
AGENTS.md (1)
1-20: LGTM!README.md (1)
128-175: New "Search" and "ASCII" benchmark sections align with test functions.The new "Search" (IndexAny2, IndexAny3, IndexFold) and "ASCII" (IsASCII, IndexNonQuotable) benchmark subsections correspond to concrete
Benchmark_*functions confirmed insearch_test.goandbyteseq_test.go. Numbers are freshly generated and properly formatted.As per coding guidelines, new exported functions require a README section, and benchmark numbers for touched paths must be regenerated in the same run. These sections satisfy that requirement for the search and ASCII functions.
Source: Coding guidelines
search.go (1)
20-81: LGTM!swar/swar_test.go (1)
5-5: LGTM!Also applies to: 26-47, 113-162, 181-219
swar/example_test.go (2)
48-63: LGTM!
9-46: 🎯 Functional CorrectnessNo change needed —
swar.WordLenis exported inswar/swar.goand set to8, so this example compiles as written.> Likely an incorrect or invalid review comment.swar_verify_test.go (2)
21-23: LGTM!
122-162: 📐 Maintainability & Code QualityNo change needed here.
ToLowerCopy/ToUpperCopyalready document thefromprecondition, and the only in-repo callers alignfromtoWordLen, so non-word-aligned cases are not part of the current contract.> Likely an incorrect or invalid review comment.fuzz_test.go (1)
1-104: LGTM!
…st.go - parse_swar_test.go's tests and the digit-run benchmark now live in parse_test.go next to the other parser tests, keeping the dropped IsIPv4/IsIPv6 SWAR experiment rationale as a section comment. - swar_verify_test.go becomes swar_test.go. It stays in the root package rather than moving under swar/: it is an integration test of the swar consumers (EqualFold and the strings/bytes case wrappers, which sit above the swar package), and its ASCII fold references are shared with the fuzz targets in fuzz_test.go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
…alog
Review round four (max effort) found no correctness bugs; this applies
its four surviving findings:
- swar: Test_MatchByteMask_Exhaustive now uses the same plain
if-got/want-Fatalf checks as its sibling range test instead of ~8.4M
require.Equal calls, cutting the package's -race test time from ~18s
to ~1.5s (single-run timings, session machine) on every lane of the
six-job CI matrix
- parse_test: the fixed-value and random loops of
Test_Parse8Digits_And_IsEightDigits share one checkParse8 helper
instead of two bytewise-identical check blocks
- caseconv: document the src-is-never-written contract on ToLowerCopy,
ToUpperCopy, and First{Upper,Lower}Index — the safe strings.ToLower/
ToUpper pass an unsafe view over immutable string memory, and that
invariant previously existed nowhere in writing
- README: give the swar package its catalog section (prose; its exports
are documented by godoc contract) and state inside the benchmark
block that Benchmark_Load8_Fusion, an advisory codegen guard, is
deliberately omitted, so the block's go test ./... provenance line
matches what it shows. The omission itself follows the maintainer
review's recorded decision; ContainsFold stays covered by the Search
section's IndexFold rows
Also picks up gofumpt v0.10.0's re-format of two pre-existing
tests = append(...) sites via make format.
Full suite green, golangci-lint at the pre-existing local baseline,
make benchfmt idempotent over the README edits.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 7193124 | Previous: 9f852c9 | Ratio |
|---|---|---|---|
Benchmark_TrimSpaceBytes/fiber/empty (github.com/gofiber/utils/v2) |
0.6268 ns/op 0 B/op 0 allocs/op |
0.3123 ns/op 0 B/op 0 allocs/op |
2.01 |
Benchmark_TrimSpaceBytes/fiber/empty (github.com/gofiber/utils/v2) - ns/op |
0.6268 ns/op |
0.3123 ns/op |
2.01 |
This comment was automatically generated by workflow using github-action-benchmark.
require.True boxed its three variadic message args on every call, so Benchmark_CalculateTimestamp/fiber_asserted reported a nondeterministic 8-15 B/op depending on runtime value caching and ticker attribution -- the source of the 9 -> 14 B/op (1.56x) benchmark-gate alert on a path this PR never touched. Building the failure message only on failure makes the benchmark a deterministic 0 B/op, 0 allocs/op and roughly halves its ns/op; the pass/fail semantics, including the uint32 wraparound behavior at the +-2s bounds, are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
|
Diagnosis of the benchmark-gate alerts — both flagged paths are untouched by this PR, and the evidence points to measurement artifacts rather than regressions.
|
|
Follow-up that settles the placement-artifact diagnosis above: on 7193124 (a test-only change) both previous alerts cleared — In other words: reshuffling the test binary's layout moved the artifact from one ~0.31 ns empty-loop benchmark to another. As long as the gate compares sub-ns empty-loop benchmarks against the cached baseline at a 1.5× threshold, roughly one of them will land on an unlucky address in any given binary — whichever it happens to be. There's nothing in the library code to fix; this one is the gate's call to dismiss. Generated by Claude Code |
3 participants
Convert the ASCII case-conversion hot paths from per-byte table lookups
to SWAR (SIMD within a register) processing of eight bytes per uint64:
tricks plus first-index scans, copy converters, and in-place
converters built on them. Scans and copy converters finish with one
overlapping word; the in-place converters finish byte-wise because an
overlapping word there would stall on store-to-load forwarding.
through the SWAR helpers for inputs of 8+ bytes, keeping the
byte-wise path for shorter inputs where call overhead dominates.
words, with a single overlapping word for the tail.
tightly-sized []byte and AppendInt instead of strings.Builder plus
per-element FormatInt strings.
Benchmarks (linux/amd64, Xeon 2.10GHz):
Adds a 20k-trial randomized test comparing all SWAR paths against
reference implementations across lengths 0-69, including non-ASCII
bytes and case-variant pairs.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01Rei88K2Yzap7wUa5uNnDwb
Summary by CodeRabbit
New Features
Performance
Tests
Documentation