🔥 perf: reduce allocations in routing hot path#4387
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a client exec synchronous fast-path and extracts request/retry/redirect logic into ChangesHot-path performance optimizations across client, routing, and constraints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several performance optimizations across the client, path constraint checking, and routing logic. Specifically, it adds a synchronous fast-path execution when context cancellation is absent, refactors request execution, switches to read-locks for hook execution, pre-parses integer constraints to avoid repeated strconv.Atoi calls during request matching, and caches context values in the router to avoid repeated interface method calls. Feedback on the changes suggests further optimizing the response hook execution by avoiding slices.Clone allocations when no user response hooks are registered.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4387 +/- ##
==========================================
- Coverage 91.40% 91.36% -0.04%
==========================================
Files 132 132
Lines 13120 13142 +22
==========================================
+ Hits 11992 12007 +15
- Misses 711 717 +6
- Partials 417 418 +1
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
path_test.go (1)
346-349: ⚡ Quick winUse a benchmark sink to prevent result-elimination effects.
Both loops ignore
CheckConstraint’s return value. Storing it in a package-level sink bool makes the benchmark more robust against optimization artifacts.Suggested change
+var benchConstraintResult bool + func Benchmark_CheckConstraint(b *testing.B) { @@ b.Run("range_preparsed", func(b *testing.B) { for b.Loop() { - c.CheckConstraint(param) + benchConstraintResult = c.CheckConstraint(param) } }) @@ b.Run("minLen_preparsed", func(b *testing.B) { for b.Loop() { - cLen.CheckConstraint(paramLen) + benchConstraintResult = cLen.CheckConstraint(paramLen) } }) }Also applies to: 360-363
🤖 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 `@path_test.go` around lines 346 - 349, The benchmark loops call c.CheckConstraint(param) but ignore its boolean return value, allowing the compiler to optimize it away; declare a package-level variable (e.g., benchSink bool) and in the "range_preparsed" b.Run closure (and the other similar benchmark at lines 360-363) assign the result to that sink (benchSink = c.CheckConstraint(param)) so the return value is used and the benchmark isn't eliminated by optimizations.
🤖 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 `@client/core.go`:
- Around line 214-222: Replace the manual RLock/RUnlock pattern with a
panic-safe defer: call c.client.mu.RLock() and immediately defer
c.client.mu.RUnlock() (instead of unlocking inside the loop), then iterate over
c.client.builtinResponseHooks and return any error as before; this ensures the
read lock on c.client.mu is always released even if a built-in hook panics or a
later panic is recovered upstream.
In `@path.go`:
- Around line 859-912: CheckConstraint currently returns false whenever
Constraint.intDataValid is false even if Constraint.Data (the string slice) is
present; to restore backward compatibility, when entering numeric cases
(minConstraint, maxConstraint, rangeConstraint, minLen/maxLen/betweenLen/etc.)
detect if intDataValid is false and attempt to parse c.Data (using strconv.Atoi
for each entry) into c.intData, set c.intData and c.intDataValid on success, and
only return false if parsing fails; update the numeric and length case branches
in CheckConstraint to perform this lazy initialization so manually-constructed
Constraint{ID: ..., Data: []string{"5"}} works as before.
---
Nitpick comments:
In `@path_test.go`:
- Around line 346-349: The benchmark loops call c.CheckConstraint(param) but
ignore its boolean return value, allowing the compiler to optimize it away;
declare a package-level variable (e.g., benchSink bool) and in the
"range_preparsed" b.Run closure (and the other similar benchmark at lines
360-363) assign the result to that sink (benchSink = c.CheckConstraint(param))
so the return value is used and the benchmark isn't eliminated by optimizations.
🪄 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
Run ID: 22f9a387-b968-4a3c-80b4-763f15065c0b
📒 Files selected for processing (4)
client/core.gopath.gopath_test.gorouter.go
pageton
force-pushed
the
perf/p0-allocation-reduction
branch
from
6df397c to
6fe2d0e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/core.go (1)
168-181: 💤 Low valueConsider caching the method string to avoid repeated allocations.
string(reqv.Header.Method())is called up to twice per condition evaluation. Inside the retry callback, this allocation repeats for each retry attempt. Since this PR focuses on reducing allocations, caching the method once before the conditionals would be consistent with that goal.♻️ Proposed refactor
cfg := c.getRetryConfig() +methodStr := string(reqv.Header.Method()) var err error if cfg != nil { err = retry.NewExponentialBackoff(*cfg).Retry(func() error { - if c.req.maxRedirects > 0 && (string(reqv.Header.Method()) == fiber.MethodGet || string(reqv.Header.Method()) == fiber.MethodHead) { + if c.req.maxRedirects > 0 && (methodStr == fiber.MethodGet || methodStr == fiber.MethodHead) { return c.client.DoRedirects(reqv, respv, c.req.maxRedirects) } return c.client.Do(reqv, respv) }) } else { - if c.req.maxRedirects > 0 && (string(reqv.Header.Method()) == fiber.MethodGet || string(reqv.Header.Method()) == fiber.MethodHead) { + if c.req.maxRedirects > 0 && (methodStr == fiber.MethodGet || methodStr == fiber.MethodHead) { err = c.client.DoRedirects(reqv, respv, c.req.maxRedirects) } else { err = c.client.Do(reqv, respv) } }🤖 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 `@client/core.go` around lines 168 - 181, The code repeatedly converts reqv.Header.Method() to a string in both branches and inside the retry closure causing extra allocations; before the if cfg != nil block, capture the method once into a local variable (e.g., method := string(reqv.Header.Method())) and use that variable in the conditional checks instead of repeated string(...) calls so the retry.NewExponentialBackoff(...).Retry closure and the else branch reference the cached method; keep the existing checks against fiber.MethodGet and fiber.MethodHead and use the same method variable when calling c.client.DoRedirects(reqv, respv, c.req.maxRedirects) or c.client.Do(reqv, respv).
🤖 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 `@client/core.go`:
- Around line 168-181: The code repeatedly converts reqv.Header.Method() to a
string in both branches and inside the retry closure causing extra allocations;
before the if cfg != nil block, capture the method once into a local variable
(e.g., method := string(reqv.Header.Method())) and use that variable in the
conditional checks instead of repeated string(...) calls so the
retry.NewExponentialBackoff(...).Retry closure and the else branch reference the
cached method; keep the existing checks against fiber.MethodGet and
fiber.MethodHead and use the same method variable when calling
c.client.DoRedirects(reqv, respv, c.req.maxRedirects) or c.client.Do(reqv,
respv).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 05cc4d27-ca31-4a6b-bf1f-5144ac268e9b
📒 Files selected for processing (4)
client/core.gopath.gopath_test.gorouter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- path_test.go
- router.go
- path.go
There was a problem hiding this comment.
Pull request overview
This PR targets allocation reductions in Fiber’s request-routing hot paths and the HTTP client execution pipeline, aiming to lower per-request overhead without changing functional behavior.
Changes:
- Router: cache
CustomCtxinterface method results (detectionPath,path,values) before the per-route match loop to reduce repeated interface dispatch. - Path constraints: pre-parse integer constraint metadata at route registration time and use it during
CheckConstraintto avoid per-requeststrconv.Atoi. - Client: add a synchronous execution fast-path when the context has no cancellation (
Done() == nil), and extract request execution into a shareddoRequest()helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
router.go |
Caches CustomCtx values once per nextCustom call to avoid repeated interface calls inside the match loop. |
path.go |
Adds pre-parsed integer metadata for numeric constraints and uses it during constraint checks to avoid per-request parsing. |
path_test.go |
Adds a new benchmark for pre-parsed constraint checks. |
client/core.go |
Adds sync fast-path execution and refactors common request logic into doRequest(). |
Comments suppressed due to low confidence (1)
client/core.go:200
preHooksnow usesRLock()when cloning user hooks, but still takes a fullLock()while iterating built-in hooks. Built-in request hooks appear to only read from the client and mutate the request, soRLock()should be sufficient and avoids blocking concurrent readers unnecessarily (and aligns with the PR description about switching hooks toRLock).
c.client.mu.RLock()
userHooks := slices.Clone(c.client.userRequestHooks)
c.client.mu.RUnlock()
for _, f := range userHooks {
|
@pageton See review comments above. The changes are breaking with no fallback. We do not encourage adding breaking changes unless it is a security issue. |
pageton
force-pushed
the
perf/p0-allocation-reduction
branch
from
74ce1f9 to
054c2c2
Compare
|
@gaby Fixed. Also added |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
path.go (1)
868-967: ⚡ Quick winConsider extracting fallback parsing into helper functions to reduce duplication.
The fallback parsing logic when
intDataValidis false is duplicated across all seven numeric constraint cases. Extracting this into helper functions would improve maintainability without impacting hot-path performance (helpers would only execute in the fallback path).♻️ Proposed refactor: extract parsing helpers
Add these helper functions before
CheckConstraint:+// parseSingleConstraintInt parses a single integer from c.Data[0] +// Returns the parsed value and true on success, or 0 and false on error +func (c *Constraint) parseSingleConstraintInt() (int, bool) { + if len(c.Data) == 0 { + return 0, false + } + v, err := strconv.Atoi(c.Data[0]) + if err != nil { + return 0, false + } + return v, true +} + +// parseTwoConstraintInts parses two integers from c.Data[0] and c.Data[1] +// Returns the parsed values and true on success, or 0,0 and false on error +func (c *Constraint) parseTwoConstraintInts() (int, int, bool) { + if len(c.Data) < 2 { + return 0, 0, false + } + v0, err0 := strconv.Atoi(c.Data[0]) + v1, err1 := strconv.Atoi(c.Data[1]) + if err0 != nil || err1 != nil { + return 0, 0, false + } + return v0, v1, true +} + func (c *Constraint) CheckConstraint(param string) bool {Then simplify each case. Example for
minLenConstraint:case minLenConstraint: limit := c.intData[0] if !c.intDataValid { - v, parseErr := strconv.Atoi(c.Data[0]) - if parseErr != nil { + var ok bool + limit, ok = c.parseSingleConstraintInt() + if !ok { return false } - limit = v } if len(param) < limit { return false }Example for
betweenLenConstraint:case betweenLenConstraint: lo := c.intData[0] hi := c.intData[1] if !c.intDataValid { - v0, parseErr := strconv.Atoi(c.Data[0]) - if parseErr != nil { - return false - } - v1, parseErr := strconv.Atoi(c.Data[1]) - if parseErr != nil { + var ok bool + lo, hi, ok = c.parseTwoConstraintInts() + if !ok { return false } - lo = v0 - hi = v1 } length := len(param) if length < lo || length > hi { return false }Apply the same pattern to all seven numeric constraint cases.
🤖 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 `@path.go` around lines 868 - 967, The numeric-constraint branches in CheckConstraint duplicate fallback parsing for c.Data; extract two helpers (e.g., parseFallbackInt(c *constraint, idx int) (int, bool) and parseFallbackTwoInts(c *constraint) (int, int, bool)) that return parsed ints and a success flag when c.intDataValid is false, and use c.intData values when true; then replace the repeated strconv.Atoi blocks in minLenConstraint, maxLenConstraint, lenConstraint, betweenLenConstraint, minConstraint, maxConstraint, and rangeConstraint with calls to these helpers (or direct use of c.intData when intDataValid) so the hot path remains unchanged and all fallback parsing is centralized.
🤖 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 `@path.go`:
- Around line 868-967: The numeric-constraint branches in CheckConstraint
duplicate fallback parsing for c.Data; extract two helpers (e.g.,
parseFallbackInt(c *constraint, idx int) (int, bool) and parseFallbackTwoInts(c
*constraint) (int, int, bool)) that return parsed ints and a success flag when
c.intDataValid is false, and use c.intData values when true; then replace the
repeated strconv.Atoi blocks in minLenConstraint, maxLenConstraint,
lenConstraint, betweenLenConstraint, minConstraint, maxConstraint, and
rangeConstraint with calls to these helpers (or direct use of c.intData when
intDataValid) so the hot path remains unchanged and all fallback parsing is
centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c97564a0-7947-4bfc-8918-e392ec76470a
📒 Files selected for processing (2)
path.gopath_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- path_test.go
## Changes ### client/core.go — sync fast-path + RLock for hooks - Add execSync fast-path: when context has no cancellation (Done() == nil), execute HTTP requests synchronously without goroutine/channel overhead - Extract shared doRequest() helper to eliminate duplicated request execution logic between async and sync paths - Change Lock to RLock in preHooks/afterHooks: builtin hooks only read the client hook slice and mutate the request object, not client state ### path.go — pre-parse constraint integers at registration time - Add intData [2]int and intDataValid bool to Constraint struct - Parse strconv.Atoi for minLen/maxLen/len/betweenLen/min/max/range constraints once at route registration instead of on every request - Use pre-parsed intData in CheckConstraint with validity guard ### router.go — cache interface method calls in nextCustom - Extract c.getDetectionPath(), c.Path(), c.getValues() before the route matching loop to avoid repeated interface method calls per iteration - Apply same caching to the method-not-allowed scan path ## Benchmark Results (AMD Ryzen 5 7600X, goos:linux/goarch:amd64) client/core.go: Benchmark_Client_Request_Parallel: 2075ns -> 1071ns (-48.39%, p=0.000) Benchmark_Client_Request: 80 B -> 32 B (-60.00%) allocs/op: 2 -> 1 (-50.00%) path.go (constraint check): Benchmark_CheckConstraint/range: ~10ns/op, 0 allocs (was ~50-100ns with strconv.Atoi) Benchmark_CheckConstraint/minLen: ~4ns/op, 0 allocs router.go: Benchmark_Router_Next: no regression (0 allocs maintained)
…onstraint structs
CheckConstraint now falls back to parsing Data on the fly when
intDataValid is false, restoring the pre-PR behavior for callers
that construct Constraint{ID, Data} directly without going through
route registration. The pre-parsed fast path (0 allocs) is still
used for route-registered constraints.
Adds Test_ConstraintCheckConstraint_ManualData covering all 7
numeric constraint types with manually-constructed constraints.
pageton
changed the title
|
@pageton can you post the before and after bench to the last state of this code |
pageton
force-pushed
the
perf/p0-allocation-reduction
branch
from
bc26660 to
4e70951
Compare
|
@ReneWerner87 Here are the benchmark results comparing base ( Environment: Linux/amd64, AMD Ryzen 5 7600X 6-Core, Go 1.26.2 client/core.go — Client HTTP layer
path.go — Constraint checking (new benchmark, after only)
Previously, each numeric constraint call performed The fallback path (for manually-constructed constraints without pre-parse) preserves the old behavior by parsing router.go — Routing loop
No regressions. Zero allocations maintained. Slight improvement in |
The constraint optimization (path.go) introduced a backward-compatibility regression for manually-constructed Constraint structs and will be submitted as a separate PR after proper review.
|
@pageton this |
|
@ReneWerner87 I will review the issue and carefully check everything. If there is no significant difference in performance, I will close the PR. |
|
This pull request has been closed because it does not provide a sufficient performance improvement to justify the changes. |
Summary
Reduces per-request allocations in three hot paths: the client HTTP layer, route constraint matching, and the
CustomCtxrouting loop.Changes
client/core.go— sync fast-path + RLock for hooksDone() == nil), execute HTTP requests synchronously viaexecSync()without goroutine/channel overhead. Most client requests don't need cancellation.doRequest()helper: Extracted common request execution logic (copy, retry, redirect) from both async and sync paths to eliminate duplication.preHooks/afterHookschanged fromLocktoRLock— builtin hooks only read the client's hook slice and mutate the request, not client state.path.go— pre-parse constraint integers at registration timeintData [2]intandintDataValid booltoConstraintstructstrconv.AtoiforminLen/maxLen/len/betweenLen/min/max/rangeconstraints now runs once at route registration instead of on every requestCheckConstraintuses pre-parsed data with a validity guard — invalid metadata gracefully falls back toreturn falserouter.go— cache interface calls innextCustomc.getDetectionPath(),c.Path(),c.getValues()before the route matching loop to avoid repeated interface method dispatch per iterationBenchmark Results
Notes
execSync) includesrecoverfor transport panics — this is necessary because the client package is independent of the server's recover middleware. Validated byTest_Exec_Func/panic_in_transport_returns_error.Client_Request_Send_ContextCancelunchanged (~5.1µs) — it correctly uses the async path since it has a cancellable context.strconv.Atoiper request for apps usingminLen,maxLen,range, etc.