perf(http): cut six per-request allocations from the trie router

[aryanmehrotra]

Description:

Four changes on the trie matcher's path. None touch the default mux path or any public signature.

BenchmarkRequestPath/trie/static/routes=100:

	B/op	allocs/op	ns/op
before	1072	14	~570
after	536	8	~264

1. Memoize the composed middleware chain per route. composeMiddleware calls every middleware constructor, each returning a fresh closure — so running it per request allocated one closure per middleware, per request. Five in a default app; an allocation profile put it at ~13% of all objects allocated while serving. The chain depends only on route + handler, both fixed once the router is built.

2. Store the path-parameter map only when it holds something. gorilla/mux allocates a Vars map on every successful match (route.go:101), even for a route with no parameters — costing a context node and a shallow Request copy per request to carry nothing.

3. trieNode.collect returns its slice instead of taking a *[]*routeEntry out-parameter, which stopped the slice header escaping to the heap.

4. headerCarrier.Values no longer canonicalizes. It went through http.Header.Values, which canonicalizes whatever key it gets. "baggage" is not already canonical, so that allocated the canonical string on every request — for the one header the W3C propagators always ask about. It now uses the same canonical-key table Get does.

The old comment claiming baggage was absent from that table was simply wrong; it has always been there.

Breaking Changes (if applicable):

⚠️ Two behaviour changes, both confined to the trie path (opt-in via GOFR_ROUTER), neither affecting the default mux path:

mux.Vars(r) returns nil rather than an empty non-nil map when a route has no path parameters. Every read stays correct — indexing yields the zero value, len is 0, ranging does nothing, which is all Request.PathParam and handlers do with it. An explicit != nil check would see a different answer than under the mux matcher.

Middleware registered after a route's first request is not in that route's cached chain. GoFr registers all middleware before Run, and the trie index already assumed the same.

Additional Information:

• No new dependencies.
• TestTrieRequestAllocationsDoNotRegress counts allocations so a later change cannot give them back silently. It uses five real middleware closures — a guard built on a no-op middleware cannot fail, so it would have been worthless.
• The guard carries //go:build !race: the race detector adds allocations, so an exact count cannot hold under it.
• docs/advanced-guide/routing-performance/page.md documents the trie router's behaviour.

Checklist:

• I have formatted my code using goimport and golangci-lint.
• All new code is covered by unit tests.
• This PR does not decrease the overall code coverage.
• I have reviewed the code comments and documentation for clarity.

[PiyushSingh-ZS]

All four optimizations look sound to me, and the numbers are worth having. Most of what follows is small; one comment actively contradicts the code beneath it and should be fixed before merge.

Verified on the branch

• serveMatched is reached only from serveTrie. ServeHTTP branches on rou.useTrie and otherwise delegates straight to rou.Router.ServeHTTP, so the default mux path genuinely does not see any of this. That is the claim the whole risk assessment rests on and it holds.
• mux.Vars has exactly one non-test consumer in the framework — request.go (pathParams: mux.Vars(r)) — and pathParams is only ever read (r.pathParams[key]), never written. So a nil map is safe there; there is no nil-map-write waiting to happen.
• canonicalPropagationKeys does contain headerBaggage. The old comment claiming otherwise was wrong, as the PR says, and http.Header.Values would indeed have canonicalized "baggage" on every request.
• The own sync.Map guard is the subtle part of the chain cache and I think it is correct. A route under PathPrefix(...).Subrouter() can arrive with a handler mux wrapped for this request while match.Route still points at the inner route, so keying the cache on the route alone would pin the first wrapper forever. Restricting the cache to routes GoFr registered itself is the right fence, and LoadOrStore rather than Store is the right primitive.

The allocation guard's doc comment says the opposite of its code

// The ceiling is a ceiling, not the measurement: it sits above the figure the
// commit quotes so ordinary variation in the runtime or a dependency does not
// fail the build, while a regression of the size these changes made does.
func TestTrieRequestAllocationsDoNotRegress(t *testing.T) {
	// Exact, not a ceiling. With headroom this caught only the largest of the three
	// savings here: reverting the chain memoization costs five allocations and was
	// caught, but the empty-Vars skip costs two and the collect signature one, and
	// both slipped under a tolerance wide enough to absorb toolchain drift.
	const wantAllocs = 7
	...
	if got != wantAllocs {

The godoc paragraph describes a tolerance; the inline comment and the code implement an exact match. The inline reasoning is the better of the two — a tolerance wide enough to absorb toolchain drift really would hide the 2-alloc and 1-alloc savings — so I would just delete the godoc paragraph. As it stands, the next person to hit this after a Go upgrade will read the godoc first, conclude the assertion is wrong, and loosen it.

Also worth a word: the guard pins 7 while the PR table quotes 8 allocs/op. Different fixtures (the guard uses five middlewares and a nopWriter; the benchmark uses three and a real chain), which is fine — but stating that in the comment stops it reading as an inconsistency.

mux.Vars(r) returning nil deserves an explicit maintainer sign-off

The analysis is right — indexing, len and range all behave identically, only an explicit != nil differs — and it is documented in docs/advanced-guide/routing-performance/page.md. Being opt-in behind GOFR_ROUTER softens it a lot.

My only concern is the shape of the failure: a user flips an env variable described as a performance toggle and gets a silent semantic change in any handler doing if mux.Vars(r) != nil. That is a hard one to debug because nothing in the symptom points at the router. I do not think it needs to block — just that it should be a decision someone made out loud rather than something that rides in on a perf PR.

Middleware registration ordering is now load-bearing

Middleware registered after a route's first request is not in that route's cached chain.

True, and GoFr does register everything before Run. But that moves from a convention to a silent correctness dependency: register a middleware late and it simply does not run for already-served routes, with no error anywhere. A cheap log.Error (or panic in dev) from Use/UseMiddleware when chains is already non-empty would turn that into a loud failure for the cost of one atomic read.

Conflict with #4167

Both PRs edit the route registration inside setupGraphQL in pkg/gofr/gofr.go:

• this one: NewRoute().Methods(...).Path("/graphql") -> router.Add(http.MethodPost, "/graphql", ...)
• #4167: if a.graphqlManager != nil -> if a.graphQLActive()

Whichever lands second needs a manual merge, and this one's change is functionally significant — Add calls markOwned, so /graphql becomes chain-cache eligible. Worth flagging to whoever sequences them.

[Umang01-hash]

Verified independently at head 2aa101bf (the router files are unchanged from my review; the head just merged in newer development) — builds, go vet, golangci-lint --new-from-rev (0 issues), and go test -race ./pkg/gofr/http/... all green; E2E in both mux and trie modes confirms params, param-free routes, 404, and that the memoized chain still runs the full Tracer→Logging chain (trace/span IDs present in logs). Chain-cache is race-safe: rou.mws's last write (WS middleware) precedes ListenAndServe and the lazy trie build, and the own + MatchErr guards keep subrouter/non-owned routes uncached. Alloc guard pins 7 exactly and goes red on reverting any of the three cuts. All opt-in behind GOFR_ROUTER=trie, so default deployments are untouched.

+1 to @PiyushSingh-ZS's review — agree all four optimizations are sound. Two of his non-blocking points are worth doing:

• Middleware-registration order is now load-bearing; a log.Error/dev-panic from Use/UseMiddleware when chains is already populated would turn a silent "middleware doesn't run" into a loud failure.
• Conflict with #4167 — both edit the /graphql registration in setupGraphQL; this PR's switch to router.Add makes /graphql chain-cache-eligible, so whoever merges second needs a manual reconcile.

And the alloc-guard godoc paragraph should be dropped (it contradicts the exact-match code beneath it). None block merge. LGTM.

[aryanmehrotra]

Thanks both. Everything actionable is done at 23bed3b27; one item is a maintainer decision I'd rather not make by merging.

The contradicting godoc paragraph is gone. You're right that it was the dangerous half: the next person to hit this after a Go upgrade reads the godoc first, concludes the assertion is wrong, and loosens it. The replacement paragraph also states why the guard pins 7 while the table quotes 8 — different fixtures (five middlewares and a nopWriter here, three and a real writer in the benchmark), so it stops reading as an inconsistency.

Late middleware registration is now loud. Both of you asked for this and it was the right call:

ERROR  2 middleware(s) registered after the router began serving: they will NOT run for any
       route that has already been requested, because GOFR_ROUTER memoises each route's chain.
       Register every middleware before starting the server.

Two deliberate limits:

• Error, not panic — the router may already be serving traffic, and taking a live process down over it would be worse than the bug.
• Trie-only — mux composes the chain per request, so a late registration genuinely takes effect there. Warning in mux mode would train users to ignore the message.

Cost is one atomic.Bool read per Use call, which happens a handful of times at startup. TestLateMiddlewareRegistrationIsReported and ...SilentInMuxMode pin both halves; the first fails if the report is removed.

The routing-performance page gains the registration-order caveat alongside the existing mux.Vars note.

⚠️ mux.Vars(r) returning nil — still open, deliberately. I agree with your framing: it should be a decision someone made out loud, not something that rides in on a perf PR. It's documented in docs/advanced-guide/routing-performance/page.md and it's behind GOFR_ROUTER=trie, but I haven't treated that as the sign-off. @PiyushSingh-ZS @Umang01-hash — is an explicit mux.Vars(r) != nil differing under the trie matcher acceptable, or should the trie allocate the empty map? Allocating it gives back one of the four savings, so it's a real trade either way and I'd rather have the answer than assume one.

Conflict with #4167 noted. #4167 has since been rebased and the setupGraphQL change there is now graphQLLinked && a.graphqlManager != nil; whichever lands second still needs the manual reconcile, and this one's switch to router.Add is the functionally significant half since Add calls markOwned.

[Umang01-hash]

Re-reviewed at 5078e339 — all of @PiyushSingh-ZS's feedback is resolved:

• Late-registration is now loud (reportLateRegistration + UseLogger): trie-only, error-not-panic, and cached.Store(true) sits on the cache-miss path so there's no new per-request cost (alloc guard still pins 7). Two revert-red tests confirm it fires once on a late registration and stays silent on the startup path and in mux mode.
• Alloc-guard godoc no longer contradicts the code — the 'ceiling' paragraph is replaced with the 7-vs-8 fixture explanation.
• mux.Vars(r) nil behaviour is documented.

Gates green at head: build, go vet, golangci-lint --new-from-rev (0 issues), go test -race ./pkg/gofr/http/..., plus the late-reg/chain-cache/alloc/empty-vars tests. No false positives from framework callers, and the rou.mws append-during-serving race is pre-existing (unchanged) and only reachable via the exact late-registration path this now reports.

One item is inherently cross-PR: #4167 still edits the same /graphql registration line (it keeps NewRoute(), this uses router.Add, which makes /graphql chain-cache-eligible) — whoever merges second needs a manual reconcile. LGTM.