Resolve URL mapping names from the match rather than the request - #16150
Open
codeconsole wants to merge 5 commits into
Open
Resolve URL mapping names from the match rather than the request#16150codeconsole wants to merge 5 commits into
codeconsole wants to merge 5 commits into
Conversation
Adds a `grails-web-benchmarks` module containing JMH benchmarks for the
HTTP request-processing hot path, ported from the sibling performance
branch. The module lives in its own `jmh` source set, is not published,
and is not wired into `build`/`check` - the only way to run it is the
explicit task:
./gradlew :grails-web-benchmarks:jmh
./gradlew :grails-web-benchmarks:jmh -PjmhArgs="-wi 1 -i 1 -f 1 UrlMapping"
`ControllerMappingCollectionBenchmark` measures
`GrailsControllerUrlMappings.matchAll`, which is what
`UrlMappingsHandlerMapping.getHandlerInternal` calls on every request,
and `UrlMappingBenchmark` measures the cached matcher underneath it, so
the cost of the uncached wrapper can be read against the work it wraps.
A mapping such as "/$controller/$action?/$id?" holds a closure for each
name it captures, and until now that closure answered by reaching for the
parameters bound to the current thread:
GrailsWebRequest webRequest = (GrailsWebRequest) RequestContextHolder.currentRequestAttributes();
return webRequest.getParams().get(name);
The value it was reaching for is one the match already holds. Because the
mapping is built once and shared by every request it matches, the closure
could not read it directly, so `collectControllerMappings` had to call
`webRequest.resetParams()` and `info.configure(webRequest)` for *every*
candidate just to be able to read `info.controllerName` and look the
candidate up - rebuilding the parameter map once per candidate, and then
once more in `UrlMappingsHandlerMapping` for the winner, because after the
loop the parameters described the last candidate rather than the winner.
The evaluator now carries only the name of the token it resolves;
`AbstractUrlMappingInfo`, which is created per match and does hold the
captured values, resolves it from its own parameters. Candidates are
identified without touching the request, so the parameter map is built
once per request, for the winner.
Mappings that compute a name with a closure of their own - the documented
"/$controller" { action = { params.goHere } }
- still read request state, and still have the request configured before
their names are read. `UrlMappingInfo.isNameResolutionRequestDependent()`
is what tells the two apart; it defaults to true, so an implementation
outside the framework keeps the behaviour it has today.
Behaviour change: a request parameter no longer stands in for a token the
URI did not capture. Under "/$controller/$action?", a request for
`/article?action=gallery` now routes to the controller's default action
instead of to `gallery`.
Adds an upgrade note for the routing change - a request parameter no longer stands in for a token the URI did not capture - with the before and after for `/article?action=gallery`, and states the rule where the guide introduces dynamic controller and action names.
…anch The request-path performance branch adds sections 45 and 46, so this becomes 47 and the two can merge in either order without a docs conflict.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16150 +/- ##
==================================================
+ Coverage 52.6311% 52.6442% +0.0131%
- Complexity 18436 18450 +14
==================================================
Files 2037 2038 +1
Lines 96500 96514 +14
Branches 16860 16864 +4
==================================================
+ Hits 50789 50809 +20
+ Misses 38284 38274 -10
- Partials 7427 7431 +4
🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: be305f9 Learn more about TestLens at testlens.app. |
codeconsole
requested review from
jdaugherty,
matrei and
sbglasius
and removed request for
jdaugherty and
matrei
August 15, 2026 19:09
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
1 participant
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.
Resolves captured URL mapping names from the match itself rather than from the dispatching
request, which lets
collectControllerMappingsstop rebuilding the parameter map for everycandidate mapping on every request.
Measured
ControllerMappingCollectionBenchmark.oneCandidateControllerMappingCollectionBenchmark.twoCandidatesControllerMappingCollectionBenchmark.fourCandidatesUrlMappingBenchmark.matchCachedHitcollectControllerMappingsruns in full on every request even when the URL mapping cache hits,so this is on the always-on path. It still costs ~88x the cached match it wraps; what remains is
ControllerKeyallocation, the controller map lookup and the sort.Why it was slow
A mapping's
controllerName/actionName/namespacecould be a Closure that readRequestContextHolder.currentRequestAttributes().getParams()— reaching for thread-local state toread a value the mapping already held in its own
params. To ask a candidate "which controller areyou?", the framework had to call
webRequest.resetParams()andinfo.configure(webRequest)first,per candidate, cloning the parameter map each time.
Those names now resolve from the match.
configure()is still called for mappings whose names aregenuinely request-dependent.
Deliberately unchanged
action = { params.goHere }is a documented feature — those closures are meant to read the request,and they still do, taking the old path.
Behaviour change
A request parameter no longer stands in for a token the URI does not capture. Under
"/$controller/$action?",/article?action=gallerynow routes to the default action rather thangallery; a query string could previously steer which action ran.params.actionis still boundeither way. Documented as section 47 of the 8.0 upgrade guide and in the embedded-variables guide.
This required changing one existing assertion, in
UrlMappingParameterTests.testNotEqual, which isworth a reviewer's attention. That test originally asserted the mapping did not match
(b84ef59, GRAILS-2297); when it moved to
UrlMappingsUnitTestthe fall-through to the defaultmapping made
infonon-null and acontrollerName == 'foo'assertion was substituted — 'foo' beinga value seeded into the request params by the test itself, i.e. an artifact of the thread-local
resolution rather than the
notEqualconstraint the test is named for. It now asserts the value theURI actually captured.
Notes
grails-web-benchmarks(opt-in, not part ofbuild/check). The same module is added byReduce per-request work across the request path #16149; if that lands first this rebases and drops that commit.
UrlMappingInfogainsisNameResolutionRequestDependent()as a default method returningtrue,so third-party implementations keep current behaviour.