⚡ perf(adaptor): cut allocations on the net/http bridge - #4559
Merged
Conversation
The net/http -> Fiber direction allocated a fresh 32 KiB buffer for every
single request: io.Copy could not use a shortcut because neither
io.LimitedReader nor fasthttp's body writer implements WriterTo/ReaderFrom,
so it fell back to allocating its own buffer. Copy the body with a pooled
buffer and a LimitedReader that lives in the pooled ctx instead, and skip
the copy machinery altogether for http.NoBody.
The response header copy-back allocated a string per key, a string per
value and a one-element slice per header. Pack every key and value into a
single string that the map entries re-slice, and carve the single-valued
entries out of one shared []string, so the copy-back costs two allocations
regardless of how many headers the response carries. Keys still go through
textproto.CanonicalMIMEHeaderKey, so the result matches Header.Add.
Also resolve the "ip:port" remote address into scratch space carried by the
pooled ctx, read the two config fields the handler needs once at
construction (App.Config returns a 624-byte struct by value) and call the
fasthttp adaptor directly in HTTPMiddleware instead of rebuilding
HTTPHandler's wrapper on every request.
Add benchmarks for the previously uncovered HTTPMiddleware and FiberApp
paths, plus tests for the header packing, the shared backing array and the
reused remote address.
FiberHandlerFunc/No_Content 10326n -> 901n 32986B -> 149B 7 -> 1 allocs
FiberHandlerFunc_Parallel/1MB 5286n -> 404n 32992B -> 147B 7 -> 1 allocs
FiberApp 22608n -> 3606n 33300B -> 1146B 25 -> 12 allocs
FiberApp_Parallel 13400n -> 942n 33290B -> 404B 14 -> 3 allocs
FiberApp_Body 29501n -> 6163n 37436B -> 5229B 21 -> 14 allocs
HTTPMiddleware 11719n -> 8793n 2240B -> 2225B 25 -> 24 allocs
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QQgWGL4o639y2MP1hJjm9E
Contributor
WalkthroughThe net/http–Fiber adaptor now uses pooled buffers and scratch storage for request bodies, response headers, and remote addresses. Request population and handler execution are streamlined, configuration is captured once, and tests and benchmarks cover the updated conversion paths. ChangesAdaptor optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant HTTPClient
participant HTTPMiddleware
participant FiberApp
participant HTTPResponse
HTTPClient->>HTTPMiddleware: send request and headers
HTTPMiddleware->>FiberApp: populate fasthttp request and copy limited body
FiberApp-->>HTTPMiddleware: return response and execution error
HTTPMiddleware->>HTTPResponse: copy canonical response headers
HTTPResponse-->>HTTPClient: send adapted response
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
gaby
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4559 +/- ##
==========================================
+ Coverage 93.22% 93.25% +0.03%
==========================================
Files 140 140
Lines 14685 14747 +62
==========================================
+ Hits 13690 13753 +63
+ Misses 622 619 -3
- Partials 373 375 +2
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@middleware/adaptor/adaptor_test.go`:
- Around line 1991-1999: Ensure each benchmark iteration uses a fresh
httptest.ResponseRecorder: in middleware/adaptor/adaptor_test.go lines
1991-1999, move w := httptest.NewRecorder() into the b.Loop() body; in lines
2017-2025, create the recorder inside each pb.Next() iteration for every
goroutine. Do not reuse or accumulate recorder state between requests.
In `@middleware/adaptor/adaptor.go`:
- Around line 472-478: Update the HTTP adapter error-call site to invoke
app.ErrorHandler so mounted sub-applications resolve their path-specific
handlers. Keep the cached cfg.BodyLimit/maxBodySize initialization unchanged,
but remove or stop using the cached errorHandler value in adaptor construction.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df0abd9b-47c5-4284-9528-b03decef9d14
📒 Files selected for processing (2)
middleware/adaptor/adaptor.gomiddleware/adaptor/adaptor_test.go
Resolving the remote address into storage carried by the pooled ctx made RemoteAddr()/RemoteIP() unsafe to keep past the handler: the next request that picked up the same pool entry overwrote it, so code that stashes the address — an async access log, a connection registry, a stream writer goroutine — silently reported a later request's client. Three requests from 1.1.1.1, 2.2.2.2 and 3.3.3.3 that each retained the address all ended up reading 3.3.3.3. Give every request its own address again, but keep the IP storage inline next to the net.TCPAddr so the fast path costs one allocation rather than the two it took before this branch. The IP slice is capped at its length so appending to it cannot write into the rest of the buffer. Also give Benchmark_FiberApp_Body and Benchmark_FiberApp_Parallel a fresh recorder per iteration. Reusing one accumulated the response header map and body across iterations, which kept copyResponseHeaders permanently on the "key already present" branch and made FiberApp_Parallel's alloc count incomparable to the otherwise identical FiberApp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QQgWGL4o639y2MP1hJjm9E
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
middleware/adaptor/adaptor.go (1)
579-579: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueCall
errorHandleras a bare statement.The error is only used to send the error response, so
_ = errorHandler(ctx, err)is unnecessary; useerrorHandler(ctx, err)instead.🤖 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 `@middleware/adaptor/adaptor.go` at line 579, Update the error-handling call in the relevant adaptor flow to invoke errorHandler(ctx, err) directly as a bare statement, removing the unnecessary blank assignment while preserving the existing arguments and behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@middleware/adaptor/adaptor.go`:
- Line 579: Update the error-handling call in the relevant adaptor flow to
invoke errorHandler(ctx, err) directly as a bare statement, removing the
unnecessary blank assignment while preserving the existing arguments and
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 80d72d38-dfca-45d7-b20c-8bb6443ddc70
📒 Files selected for processing (2)
middleware/adaptor/adaptor.gomiddleware/adaptor/adaptor_test.go
Caching cfg.ErrorHandler at construction pinned the root app's handler. App.ErrorHandler resolves a mounted sub-app's handler from the request path, so that lookup belongs on the request. BodyLimit stays cached — it is a plain value and the App.Config copy it avoids is 624 bytes per request. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QQgWGL4o639y2MP1hJjm9E
ReneWerner87
approved these changes
Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Cuts allocations and per-request work in
middleware/adaptor, mostly on the net/http → Fiber direction (FiberApp,FiberHandler,FiberHandlerFunc).The dominant cost was invisible:
io.Copy(req.BodyWriter(), limitedReader)allocated a fresh 32 KiB buffer for every single request, including requests with no body at all.io.Copyonly skips its internal buffer when the source implementsio.WriterToor the destination implementsio.ReaderFrom—*io.LimitedReaderdoes neither, and neither does fasthttp'srequestBodyWriter. So every request throughFiberApppaid a 32 KiB allocation regardless of body size.Four changes, all internal to the package:
io.CopyBufferwith a buffer from the pool the package already keeps, and anio.LimitedReaderthat lives in the pooled ctx.http.NoBodyskips the copy machinery entirely.dst.Add(string(k), string(v))per header, costing a key string, a value string and a one-element slice each. Keys and values are now packed into one string that the map entries re-slice, and the single-valued entries are carved out of one shared[]string, so the copy-back is two allocations regardless of header count. Keys still go throughtextproto.CanonicalMIMEHeaderKey, so results are identical toHeader.Add."ip:port"fast path co-locates the IP storage with thenet.TCPAddrin one allocation instead of two (address + IP slice). Deliberately not pooled:RemoteAddr()/RemoteIP()hand this value to user code that may keep it past the handler.App.Config()returns a 624-byte struct by value; the body limit is now read once at construction rather than copied per request.HTTPMiddlewarecalls the fasthttp adaptor directly instead of rebuildingHTTPHandler's wrapper on every request.No exported API changes, and no behavior changes.
Changes introduced
Benchmarks: 6 runs per side,
-benchtime 2s, compared withbenchstatagainst84ecfab. Every changed row below isp=0.002.FiberHandlerFunc/No_ContentFiberHandlerFunc_Parallel/No_ContentFiberAppFiberApp_ParallelFiberApp_BodyHTTPMiddlewareHTTPHandlerWithContextHTTPHandlerBenchmark_HTTPMiddleware,Benchmark_FiberApp,Benchmark_FiberApp_BodyandBenchmark_FiberApp_Parallelare new — those paths had no benchmark coverage at all.Changelog/What's New:
middleware/adaptorno longer allocates a 32 KiB buffer per request on the net/http → Fiber path; requests throughFiberAppdrop from 25 to 12 allocations and from ~33 KiB to ~1.2 KiB per request.API Longevity: no exported identifiers added, removed or changed. The whole diff is package-internal.
Documentation Update: not needed — no exported API or documented behavior changed.
Migration Guide: not needed — no breaking change.
API Alignment with Express: N/A, internal performance work.
Examples: N/A.
Type of change
Checklist
/docs/— untouched, nothing user-facing changed.New tests cover the header packing (multi-value headers, non-canonical keys, a destination that already holds values, and that entries sharing the
[]stringbacking array stay independent when appended to), that response header strings do not alias the pooled staging buffer, that thehttp.NoBodyskip still reportsContent-Length: 0, and that a remote address retained by a handler still reports its own client after the ctx pool recycles the entry.Verification: full repo suite passes,
go test -race ./middleware/adaptor/clean,golangci-lintv2.12.2 (the version pinned inlint.yml) reports 0 issues.Not included, deliberately
Two larger wins were measured but left out, since both need a maintainer decision rather than a drive-by:
HTTPHandleris unchanged at 18 allocs/op. All of it is insidefasthttpadaptor.NewFastHTTPHandler, which spawns a goroutine and anio.Pipeper request (~10 allocs) to supportFlush/Hijack. Reducing it means forking that concurrency-sensitive upstream code into Fiber.FiberAppstill callsapp.Handler()per request, which takes the globalapp.mutexand runsstartupProcess()every time. Hoisting it behind async.Oncemeasured −36% onFiberApp_Parallelat 4 cores (scaling 2.1× → 3.3×) — the single largest remaining win. It is not in this PR because it silently drops the implicit tree rebuild for routes registered at runtime, so those would 404 unless the user callsapp.RebuildTree(). That is a routing-semantics change and shouldn't ship under a perf banner.