refactor(h1): delete the unused SIMD header scan and stop advertising a SIMD parser (celeris#424) - #697
Conversation
… a SIMD parser (celeris#424) findHeaderEnd (protocol/h1/findheader_*: SSE2/AVX2 on amd64, NEON on arm64, a scalar loop elsewhere) was the only assembly in the tree, and it has had no production caller since 96581bc (#359, 2026-06-17), which dropped the parser's upfront header-block scan as double work. #424, filed a month later, asked CI to exercise the arm64 path of a routine nothing calls. What remained was the routine, its own tests and benchmarks, and a README feature bullet ("SIMD HTTP parser -- SSE2 (amd64) and NEON (arm64) with a generic SWAR fallback") describing code that no request runs. At 9f4d89b, a search for the name followed by "(" in non-test Go files finds only the three declarations. The same query finds parser.go:118's live parseHeaders call, and at 96581bc~1 it finds the removed `if findHeaderEnd(remaining) < 0`. No go:linkname names it. This deletes the six findheader files, the five findHeaderEnd tests and two benchmarks, and a comment that named it. The README bullet now describes what the parser does: header and body slices alias the read buffer (the package doc's own claim). After the deletion, `go build ./...` and `go vet ./protocol/...` pass for linux/{amd64,arm64,386,riscv64} and darwin/{amd64,arm64}, which covers every architecture that had an implementation. `go test ./protocol/h1` passes 140/140. go.mod is unchanged (tidy -diff clean). Control: a reintroduced call fails to build ("undefined: findHeaderEnd") on amd64 and arm64.
FumingPower3925
added
documentation
The README bullet and the protocol/h1 package doc said header and body slices alias "the connection's read buffer". That is true of the parser's return values relative to the buffer it is given, not of the engine path: epoll and io_uring hand the parser their read buffer only for a request that arrives whole in one read, has no body or a Content-Length body, and runs inline. A request that spans reads (its headers gathered in the connection's H1 buffer, a fixed-length body in bodyBuf, internal/conn/h1.go), a chunked body, or a request for an async handler (asyncInBuf) is served from a per-connection buffer. The package doc also listed the std engine as a consumer; std uses net/http's parser.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Repository: goceleris/celeris/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (8)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe changes remove architecture-specific HTTP/1 header-scanning implementations and their tests and benchmarks. Parser documentation and the README now describe parser buffer usage, and a parser comment names ChangesHTTP/1 parser cleanup and buffer documentation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~12 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to No actionable issue remains from the supplied evidence; the parser cleanup is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation Issue ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Merging this PR will degrade performance by 2.89%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary
#424 asks CI to exercise the arm64 NEON header parser. That routine,
findHeaderEnd(protocol/h1/findheader_*), has had no production caller since 96581bc (#359, 2026-06-17). That is a month before #424 was filed (2026-07-20). This PR resolves #424 in v1.6.0 by removing the dead code and the README claim built on it. Adding CI coverage for code no request runs would have been the wrong fix.Fixes #424
Evidence
The scripts and outputs are in
evidence/celeris-673-679-653-424/lane-20260926/424/(per-finding index for review round 2:ROUND2.md):callers-at-base.sh/.txtbuild-after-removal.sh/.txtcontrol-caller-detected.sh/.txtround2-checks.shand its four logsgit grep -w findHeaderEnd, in non-test files, finds only these:findheader_{amd64,arm64,generic}.go)TEXTsymbols and their header commentsparser.go(, excluding thefunclines) finds none. Nogo:linknamenames it.p.parseHeaders(req)call atparser.go:118.96581bc~1it finds the call h1: eliminate redundant upfront findHeaderEnd whole-block scan (headers CRLF-scanned twice/req) #359 removed:parser.go:93: if findHeaderEnd(remaining) < 0.git grep -wfor the call query.-walso requires a non-word character after the match, so it silently dropped every call, and both controls came back empty. That is why the query now spells out its left boundary.go build ./...andgo vet ./protocol/...pass for linux/amd64, linux/arm64, linux/386, linux/riscv64, darwin/amd64 and darwin/arm64. Together these cover every architecture that had an implementation: asm on amd64/arm64, the generic loop on the rest.go test -v ./protocol/h1gives 140 PASS, 0 FAIL, 0 SKIP (again at 5ca6f6e:round2-h1-vet-and-v.log).go mod tidy -diffis clean at 5ca6f6e: rc 0, no diff (round2-tidy-diff.log).run ./...at 5ca6f6e: 0 issues natively (round2-golangci-native.log) and with GOOS=linux (round2-golangci-linux.log). The CI Lint job of the same head also reports 0 issues in all five of its lint steps (run 36254991605, job 108439961458;ci-36254991605-lint.log).findHeaderEndput back intoparser.go, the build fails on amd64 and arm64 withundefined: findHeaderEnd, so the compile check does see a caller.Changes
protocol/h1/findheader{,_amd64,_arm64,_generic}.go,findheader_amd64.sandfindheader_arm64.s. These were the only assembly in the repository.TestFindHeaderEnd_*tests and the twoBenchmarkFindHeaderEnd*benchmarks.parser.go: the comment that named the function now says "CRLFCRLF scan".README.md:35: the bullet "SIMD HTTP parser — SSE2 (amd64) and NEON (arm64) with a generic SWAR fallback" becomes a Zero-copy HTTP/1.1 parser bullet. The old bullet described code that no request ran, and there was never a SWAR path. The new bullet (exact since review round 2) says:Content-Lengthbody, and runs inline.bodyBuforasyncInBuf(internal/conn/h1.go: headers split across reads at :579, a fixed-length body spanning reads at :527-530 and :673, a chunked body at :633-680, an async route at :600;engine/epoll/loop.go:1281,engine/iouring/worker.go:2791).protocol/h1/doc.go(review round 2): the package doc made the same "alias the connection's read buffer" claim. It now says the slices alias the buffer passed toParser.Reset: an engine's read buffer, or a per-connection buffer the engine gathered the request into. It also listed the std engine as a consumer. std uses net/http's parser; onlyinternal/conn(epoll, io_uring) importsprotocol/h1.src/pages/index.astro:75) is fixed in docs: EngineMetrics.Throughput always reads 0; the parser is not SIMD (celeris#653, celeris#424) docs#73.No hot-path change: the deleted code was unreachable from any request.
CI run 36254991605 on 5ca6f6e: 9/9 green. Run 36241726098 on 1d00335 was also 9/9 green.
Release notes
Summary by CodeRabbit