⚡ perf: reduce avoidable work in the request hot path#4490
Conversation
app.ErrorHandler normalized the request path on every handler error and router 404 even though the mount list is empty for non-mounted apps, copying the path each time (a heap allocation once the path outgrows the stack-allocation limit). Return early instead; Benchmark_Router_NotFound improves by ~4%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The variadic c.Method() call in the route==nil fallback pushed Route to inline cost 88 (budget 80), blocking inlining into hot callers such as Params and FullPath. Using app.method(c.methodInt) directly, which is what Method() resolves to without an override, makes Route inlinable; Benchmark_Ctx_Params improves by ~4%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The bucket-scan loops in next/nextCustom compared the index against a len(tree)-1 copy, and getMatch compared the segment length against a stale local, so the compiler kept bounds checks on tree[indexRoute] and detectionPath[:i] for every candidate route. Clamping the start index and comparing unsigned against the live length lets BCE remove both, verified with -d=ssa/check_bce. The cold 405 fallback loops are unchanged because they read the index after the loop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThis PR applies several performance-oriented micro-optimizations: an early-return fast path in ErrorHandler when no sub-apps are mounted, a variadic-call-avoiding Method computation in DefaultCtx.Route's fallback path, a bounds-check-free unsigned comparison in path segment matching, and restructured loop iteration in router next/nextCustom functions. ChangesRouting performance tweaks
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4490 +/- ##
==========================================
- Coverage 92.91% 92.91% -0.01%
==========================================
Files 138 138
Lines 13609 13607 -2
==========================================
- Hits 12645 12643 -2
Misses 597 597
Partials 367 367
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:
|
There was a problem hiding this comment.
Pull request overview
This PR applies a set of small, behavior-preserving optimizations aimed at reducing avoidable work on Fiber’s request hot path (routing + error handling), with the goal of improving latency without introducing allocations.
Changes:
- Skip mount-prefix normalization and lookup in
App.ErrorHandlerwhen there are no mounted sub-apps. - Make
(*DefaultCtx).Routecheaper/inlinable by avoiding the variadicMethod()fallback call. - Restructure hot route-scan loops and a const-segment check to enable better bounds-check elimination by the compiler.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
app.go |
Adds an early return in App.ErrorHandler when no mount prefixes exist to avoid path normalization/copy work. |
ctx.go |
Replaces c.Method() with c.app.method(c.methodInt) in the nil-route fallback to reduce inlining cost. |
router.go |
Adjusts next / nextCustom scanning loops to start from a clamped index and iterate with indexRoute < len(tree) for BCE. |
path.go |
Uses an unsigned length guard to keep detectionPath[:i] slice bounds-check free in routeParser.getMatch. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router.go (1)
291-320: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCold 405-fallback loop unchanged — good, but same pattern could get the same treatment later.
This secondary scan loop still uses the old
lenr/indexRoute++pattern. The PR explicitly leaves this alone since it's the cold path, which is a reasonable scoping decision, but worth flagging as a candidate for a consistent BCE-friendly rewrite in a future pass if this path ever becomes hot (e.g., heavy 405 traffic).🤖 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 `@router.go` around lines 291 - 320, The 405-fallback scan in router.go still uses the older lenr/indexRoute++ traversal pattern, so if you decide to align it with the rest of the routing code, refactor this loop in the same style as the other route-walk logic around treeStack/treeHash by avoiding manual post-increment indexing and keeping the bounds-check-friendly access pattern consistent. This is a low-priority follow-up for Router route matching, especially if the fallback path becomes hot later.
🤖 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 `@router.go`:
- Around line 291-320: The 405-fallback scan in router.go still uses the older
lenr/indexRoute++ traversal pattern, so if you decide to align it with the rest
of the routing code, refactor this loop in the same style as the other
route-walk logic around treeStack/treeHash by avoiding manual post-increment
indexing and keeping the bounds-check-friendly access pattern consistent. This
is a low-priority follow-up for Router route matching, especially if the
fallback path becomes hot later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c4ebbef6-ab98-44a2-ab14-9a456d38b12b
📒 Files selected for processing (4)
app.goctx.gopath.gorouter.go
gaby
changed the title
Summary
Three small, behavior-preserving optimizations in the request hot path:
app.ErrorHandler: return early when no sub-apps are mounted. The mount lookup normalized (copied) the request path on every handler error and every router 404, even thoughappListKeysis empty for non-mounted apps, where the whole prefix loop is dead code. For paths beyond the stack-allocation limit this copy is a heap allocation.DefaultCtx.Route: theroute == nilfallback called the variadicc.Method(), pushingRouteto inline cost 88 (budget 80). Usingapp.method(c.methodInt)directly, which is whatMethod()resolves to without an override, makesRouteinlinable into hot callers such asParamsandFullPath.next/nextCustomcompared the index against alen(tree)-1copy, andgetMatchcompared the segment length against a stale local, so the compiler kept bounds checks ontree[indexRoute]anddetectionPath[:i]for every candidate route. Clamping the start index withmax(..., 0)and comparing unsigned against the live length lets BCE remove both. The cold 405 fallback loops are unchanged because they read the index after the loop.Benchmarks
benchstat vs current main, Apple M2 Pro,
-count 10:All benchmarks stay at 0 B/op, 0 allocs/op.
Verification
go test .: 1400 tests passmake lint: 0 issues-d=ssa/check_bce: no bounds check left ontree[indexRoute](both hot loops) ordetectionPath[:i]; the cold 405 loops intentionally keep theirs-gcflags=-m:can inline (*DefaultCtx).Route, inlined at its in-package call sites🤖 Generated with Claude Code