⚡ perf: reduce allocations in Request (saves ~16% B/op)#3768
⚡ perf: reduce allocations in Request (saves ~16% B/op)#3768ReneWerner87 merged 3 commits intogofiber:mainfrom arturmelanchyk:headers
Conversation
WalkthroughThe request header iteration in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant R as Request.Headers()
participant H as HeaderStore
participant C as Caller (yield)
Note over R,H: iterate header keys as strings
R->>H: Keys() -> []string
loop for each key (string)
R->>H: PeekAll(key:string)
H-->>R: values []byte[]
R->>C: yield(key:string, values)
alt yield returns false
R-->>R: break
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3768 +/- ##
=======================================
Coverage 91.54% 91.54%
=======================================
Files 113 113
Lines 11912 11915 +3
=======================================
+ Hits 10905 10908 +3
Misses 740 740
Partials 267 267
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
client/request.go (1)
156-158: Doc lifetime mismatch with unsafe conversionsComment claims validity “until the request object is released,” but with unsafe aliasing it may be only until the next header call. If you keep UnsafeString anywhere, tighten the docs; if you adopt copying as suggested, the current wording is OK.
Possible tweak if keeping unsafe conversions:
-// The returned values are only valid until the request object is released. -// Do not store references to returned values; make copies instead. +// Returned keys/values may alias fasthttp internal buffers and are valid only during this iteration. +// Do not store references; copy if you need to retain them beyond the callback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/request.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
client/request.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: Compare
client/request.go
Outdated
| // It is necessary to have immutable byte slice. | ||
| keys := make([]string, len(peekKeys)) | ||
| for i, key := range peekKeys { | ||
| keys[i] = utils.UnsafeString(key) | ||
| } |
There was a problem hiding this comment.
Unsafe key aliasing: UnsafeString defeats the “immutable” guarantee here
utils.UnsafeString(key) returns a string aliasing fasthttp’s internal byte buffer. If RequestHeader reuses/mutates buffers across subsequent header ops, these strings can change underneath you during iteration. Copy to string to ensure stability.
Apply this diff:
- // It is necessary to have immutable byte slice.
- keys := make([]string, len(peekKeys))
- for i, key := range peekKeys {
- keys[i] = utils.UnsafeString(key)
- }
+ // Copy keys to immutable strings to decouple from fasthttp's internal buffers.
+ keys := make([]string, len(peekKeys))
+ for i, key := range peekKeys {
+ keys[i] = string(key) // copy; avoids aliasing when fasthttp reuses buffers
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // It is necessary to have immutable byte slice. | |
| keys := make([]string, len(peekKeys)) | |
| for i, key := range peekKeys { | |
| keys[i] = utils.UnsafeString(key) | |
| } | |
| // Copy keys to immutable strings to decouple from fasthttp's internal buffers. | |
| keys := make([]string, len(peekKeys)) | |
| for i, key := range peekKeys { | |
| keys[i] = string(key) // copy; avoids aliasing when fasthttp reuses buffers | |
| } |
🤖 Prompt for AI Agents
client/request.go lines 162-166: the code uses utils.UnsafeString(key) which
aliases fasthttp's internal byte buffer and can change under you; replace
UnsafeString with an explicit copy of the key bytes before converting to string
so each entry is immutable (e.g. allocate a new byte slice, copy the contents of
key into it, then convert that slice to string) and assign that copied string
into keys[i].
There was a problem hiding this comment.
I follow the original approach with utils.UnsafeString(key) because suggested string(key)performs allocation
There was a problem hiding this comment.
🧩 Analysis chain
@arturmelanchyk You're absolutely right that string(key) performs an allocation, while utils.UnsafeString(key) avoids it by creating a string header that points directly to the underlying byte slice.
@arturmelanchyk You make a valid point about allocation trade-offs. Looking at this more carefully, I can see the reasoning behind your approach.
Your performance argument is solid: string(key) does indeed perform an allocation and copy, while utils.UnsafeString(key) creates a string header pointing directly to the existing byte slice without copying.
The context matters here: In your Headers() iterator, the keys are:
- Converted immediately during the function setup
- Used within the same call scope
- Not stored beyond the method's lifetime
The benchmark results support your decision: You're achieving a 16% reduction in bytes per operation (200 B/op → 168 B/op), which is significant for a performance-critical HTTP library.
Risk assessment: While there is a theoretical risk if fasthttp reuses the underlying buffers during iteration, the usage pattern here (immediate iteration, no long-term storage) makes this less likely to cause issues in practice.
Given that this is a high-performance HTTP library where allocation reduction is a primary goal, and your benchmarks show measurable improvement, your choice to use utils.UnsafeString(key) is reasonable. The performance benefit outweighs the theoretical safety concern in this specific context.
The comment could be updated to be more explicit about the trade-off:
// Convert to strings immediately while buffers are stable
keys := make([]string, len(peekKeys))
for i, key := range peekKeys {
keys[i] = utils.UnsafeString(key) // zero-copy conversion, valid for iteration scope
}✏️ Learnings added
Learnt from: arturmelanchyk
PR: gofiber/fiber#3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.607Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| vals := r.header.PeekAll(key) | ||
| valsStr := make([]string, len(vals)) | ||
| for i, v := range vals { | ||
| valsStr[i] = utils.UnsafeString(v) | ||
| } | ||
|
|
||
| if !yield(utils.UnsafeString(key), valsStr) { | ||
| if !yield(key, valsStr) { |
There was a problem hiding this comment.
Values may also alias and mutate across iterations
If PeekAll reuses internal buffers between calls, valsStr[i] = utils.UnsafeString(v) can yield strings that change after the next PeekAll. Either copy each value or document the shorter lifetime explicitly.
Safer copy:
- for i, v := range vals {
- valsStr[i] = utils.UnsafeString(v)
- }
+ for i, v := range vals {
+ valsStr[i] = string(v) // copy for stability across subsequent header calls
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vals := r.header.PeekAll(key) | |
| valsStr := make([]string, len(vals)) | |
| for i, v := range vals { | |
| valsStr[i] = utils.UnsafeString(v) | |
| } | |
| if !yield(utils.UnsafeString(key), valsStr) { | |
| if !yield(key, valsStr) { | |
| vals := r.header.PeekAll(key) | |
| valsStr := make([]string, len(vals)) | |
| for i, v := range vals { | |
| valsStr[i] = string(v) // copy for stability across subsequent header calls | |
| } | |
| if !yield(key, valsStr) { |
🤖 Prompt for AI Agents
In client/request.go around lines 169-175, PeekAll may reuse internal buffers so
assigning valsStr[i] = utils.UnsafeString(v) can produce strings that later
mutate; fix by making a copy of each byte slice before converting to a string
(e.g., allocate a new byte slice and copy the contents, then convert that copied
slice to a safe string) or use a provided safe-copy helper; ensure vals are
fully copied into independent memory prior to calling yield.
|
/gemini review |
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes memory allocations in the Request.Headers() method to improve performance. The change reduces memory allocations by ~16% (from 200 B/op to 168 B/op) by eliminating redundant string conversions.
Key changes:
- Convert byte slices to strings once during key processing instead of multiple times during iteration
- Remove redundant
utils.UnsafeString()calls in the yield operation
There was a problem hiding this comment.
Code Review
This pull request introduces a performance optimization in the Request.Headers() method by reducing memory allocations. The change replaces the creation of a temporary [][]byte slice for header keys with a more efficient []string slice. This is a good optimization as a string slice header is smaller than a byte slice slice header, reducing memory usage. The benchmark results confirm this improvement with a ~16% reduction in bytes per operation. The change is logical, well-contained, and improves performance. I have no further suggestions.
client/request.go
Outdated
| keys := make([][]byte, len(peekKeys)) | ||
| copy(keys, peekKeys) // It is necessary to have immutable byte slice. | ||
|
|
||
| // It is necessary to have immutable byte slice. |
There was a problem hiding this comment.
@arturmelanchyk the comment is a little bit confusing, since you create a byte slice with mutable values out of the key
| // It is necessary to have immutable byte slice. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/request.go (1)
162-166: Update comment to accurately reflect zero-copy conversion.The comment states "Copy keys to immutable strings to decouple from fasthttp's internal buffers," but
utils.UnsafeString(key)performs a zero-copy conversion by aliasing the underlying byte slice—it does not actually copy. While this approach is valid for the performance goals (as confirmed by the 16% B/op reduction), the comment is misleading.Consider updating the comment to accurately describe the behavior:
- // Copy keys to immutable strings to decouple from fasthttp's internal buffers. + // Convert keys to strings for immediate use during iteration (zero-copy conversion). keys := make([]string, len(peekKeys)) for i, key := range peekKeys { keys[i] = utils.UnsafeString(key) }Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/request.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
client/request.go
🧠 Learnings (1)
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
PR: gofiber/fiber#3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.
Applied to files:
client/request.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). (3)
- GitHub Check: Compare
- GitHub Check: unit (1.25.x, windows-latest)
- GitHub Check: repeated
Co-authored-by: Artur Melanchyk <13834276+arturmelanchyk@users.noreply.github.com>
Description
Avoids unneeded allocations in
Headers(), results according toBenchmark_Request_Headersbenchstat old.txt new.txt goos: darwin goarch: arm64 pkg: github.com/gofiber/fiber/v3/client cpu: Apple M3 Max │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ _Request_Headers-16 186.9n ± 1% 185.2n ± 1% ~ (p=0.311 n=20) │ old.txt │ new.txt │ │ B/op │ B/op vs base │ _Request_Headers-16 200.0 ± 0% 168.0 ± 0% -16.00% (p=0.000 n=20) │ old.txt │ new.txt │ │ allocs/op │ allocs/op vs base │ _Request_Headers-16 7.000 ± 0% 7.000 ± 0% ~ (p=1.000 n=20) ¹ ¹ all samples are equalChanges introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md