Reduce per-request work across the request path - #16149
Open
codeconsole wants to merge 23 commits into
Open
Conversation
The request Grails exposed to controllers, tag libraries and GSPs was replaced with the resolved MultipartHttpServletRequest for file uploads. That discarded every request wrapper contributed after multipart resolution - the hidden HTTP method filter, Spring Security, and any application filter - and required a mutable pointer on GrailsWebRequest plus propagation code to maintain it. The request is now always the outermost request, and multipart capabilities are discovered from its wrapper chain via WebUtils.resolveMultipartRequest. When the DispatcherServlet resolves a request Grails had already bound, the wrapper sits above that request and cannot be reached by unwrapping, so it is also published as a request attribute. - Add WebUtils.resolveMultipartRequest and isMultipartContentType - Add the MultipartRequest read surface to HttpServletRequestExtension so request.getFile(..) and friends keep working, failing loudly rather than returning null when the request is not a resolved multipart request - Populate GrailsParameterMap through discovery rather than an instanceof check - Replace GrailsWebRequest.setMultipartRequest and the multipart branch in getCurrentRequest with multipartRequestResolved, which only invalidates the cached params (apachegh-13837) - Return the processed request from GrailsDispatcherServlet.checkMultipart, so the dispatch runs against it as Spring MVC expects - Delete the unreachable multipart resolution in DefaultUrlMappingInfo, along with the undocumented grails.web.disable.multipart setting - Make the SpringSecurityUtils multipart branch functional again; it read an attribute only the deleted DefaultUrlMappingInfo code ever wrote request instanceof MultipartHttpServletRequest and casts to that type no longer work; documented in the 8.0 upgrade guide.
isMultipartContentType had no production caller - only the test written for it. Condense the six per-method javadoc blocks on the file upload accessors into one.
…ntext GrailsWebRequest built a GrailsApplicationAttributes on every request, through a reflective Constructor.newInstance. That object holds no request state - it caches the beans its own comment calls "used very often" (template engine, GrailsApplication, GroovyPagesUriService, MessageSource, plugin manager) - so building one per request paid for the reflection and then discarded all five caches immediately. It is now created on first use and cached in the servlet context, and rebuilt only if the ApplicationContext it resolved against is no longer current, so a replaced or restarted context (as happens between tests) is never served a stale instance. Its lazily populated fields become volatile now that one instance is shared across request threads. Also stop allocating a UrlPathHelper per request or per call. Spring exposes UrlPathHelper.defaultInstance and none of the four Grails instances were configured, so they can share it.
…ceptors UrlMappingsHandlerMapping.getHandlerExecutionChain re-implemented the loop from AbstractHandlerMapping, so Grails-mapped requests silently missed whatever Spring added to that method later. Currently that is the API version deprecation interceptor, which means the Deprecation, Sunset and Link headers configured by spring.mvc.apiversion.* were never emitted for a Grails-mapped request. It now calls super and inserts the WebRequestInterceptors at the front, which keeps the "OSIV must run first" ordering that motivated the override. The two Grails interceptors are stateless, so they become shared instances instead of two allocations per request, and the @CompileDynamic MappedInterceptor cast helper goes away with the copied loop. Also: - Keep UrlMappingsHandlerMapping's own UrlPathHelper. UrlPathHelper.defaultInstance is read-only and that field is protected, so pointing it at the shared instance would break any subclass configuring it. It is a singleton bean, so there was no per-request allocation to save there anyway. - Restore the previous LocaleContext in GrailsWebRequestFilter rather than clearing it, so a LocaleContext set by a filter outside Grails survives, matching Spring's own RequestContextFilter.
GrailsParameterMap's constructor defensively copied request.getParameterMap() into a LinkedHashMap before walking it. updateNestedKeys only ever reads that map - every put it makes goes into wrappedMap or a nested map it created - so the copy was only ever needed to merge uploaded files in. The servlet map (immutable per the servlet contract) is now walked directly, and copied only when there are multipart files to merge, removing a map allocation and a full entry copy per request for every non-upload request.
A URL mapping cache miss was the most expensive thing in the request path by three orders of magnitude - 1964 ns for a URI only the default mapping serves, against 2.5 ns for a cache hit - because every miss ran a linear scan allocating a Matcher for each of the ~56 compiled patterns in a mid-size application. RegexUrlMapping now records each pattern's slash count at parse time and skips patterns whose segment count rules them out. Every construct convertToRegex emits is bounded to a single path segment except ".*", which comes only from a "**" token, so a pattern without "**" can only match a URI with exactly its slash count or one more, the extra one coming from the trailing "/??" every pattern ends with. Patterns containing "**" are never skipped. Candidates are skipped, never reordered, so the scan still returns the first mapping that matches and declaration precedence is unchanged. This replaces the patternByTokenCount map, which built exactly this index and was never read by anything. The holder computes the URI's slash count once per request rather than once per mapping, and hoists the per-candidate LOG.isDebugEnabled() call out of the three scan loops.
The adapter passed its callback to observe() as `{ -> i.before() } as BooleanSupplier`.
Groovy evaluates that coercion before entering observe(), so it ran even when the
ObservationRegistry is a no-op, and DefaultGroovyMethods.asType routes it through
CachedSAMClass.coerceToSAM to Proxy.newProxyInstance. Every matched interceptor
therefore cost a Closure, a Class[], a ConvertedClosure and a JDK dynamic proxy per
phase per request, with each callback dispatching reflectively through
ConversionHandler rather than calling the interceptor directly.
The phase is now a private enum that dispatches straight to before()/after(), so the
default path is a field read, a no-op check and an interface call. In the compiled
class groovy.lang.Reference references drop from 15 to 0 and the two closure classes
are gone. The observing path is structurally unchanged.
Also caches the logical interceptor name per class rather than recomputing it per
interceptor per phase per request, and reverses the matched-interceptor list in place
rather than copying it - the reversed list is stored back under the request attribute
and read by afterCompletion, so the ordering remains observable and unchanged.
Adds coverage for the observation path, which previously had none, including the
null-registry branch, the no-op registry branch, and error recording.
The controller AST transformer emitted the ALLOWED_METHODS_HANDLED request-attribute guard twice into the same generated wrapper - once from convertToMethodAction and again from wrapMethodBodyWithExceptionHandling - producing two byte-identical blocks where the second could never do anything, because the first had already set the attribute. It also emitted the guard, and its finally-block cleanup, for controllers that declare no allowedMethods at all. An action on a controller with no allowedMethods was paying four dynamic request property gets, two getAttribute, a setAttribute, a removeAttribute and a compareEqual per request for a guard that could never fire. Each request property get goes through an indy callsite and RequestContextHolder, so this was not free. The duplicate emission is removed, and the bookkeeping is now generated only for controllers that declare a non-empty allowedMethods map. Gating it per action rather than per controller looks equivalent but is not: the marker means "an action has already begun handling this request" (apachegh-11444), so an unrestricted action must still set it, or a restricted action it invokes programmatically starts rejecting the request. Controllers that use allowedMethods generate byte-identical code to before. Adds coverage for the command-object path, which had none.
…okup Binding a command object resolved the DataBindingSourceRegistry, the MimeTypeResolver and the GrailsWebDataBinder from the bean factory on every request, each with a containsBean followed by a getBean, and did so twice because bindObjectToInstance runs createDataBindingSource again. Holders.findApplication() is itself a getBean rather than a field read, and was called twice more per bind. These now resolve once per ApplicationContext, held in a single-entry volatile cache. A map keyed by ApplicationContext would retain every context ever seen, since the cached beans reference the context, so a single entry replaced whenever a different context appears is both cheaper and the correct invalidation signal for dev restarts and test contexts. Separately, getBindingIncludeList used getDeclaredField to look up the AST-injected whitelist field and cached the result after the call. For any class the transformer did not touch - inner-class command objects, precompiled classes, plain POJOs - that call threw, control jumped past the caching, and the exception was reconstructed on every subsequent bind. It now uses ReflectionUtils.findField, which returns null, and caches the negative result too, while still requiring the field to be declared on the class itself so an untransformed subclass does not inherit its parent's whitelist.
…ing paths Four lookups repeated per request, all resolving to values that are stable: - Every redirect read the controller's static namespace field reflectively, through a hierarchy walk plus makeAccessible plus Field.get. The in-code comment already noted this was avoidable. Now cached per controller Class; a reloaded class is a different Class object, so a stale namespace cannot be served. - Every redirect allocated a ResponseRedirector and called three setters on it. That object holds only configuration and takes the request, response and arguments per call, so one is now built lazily and reused. Each of its setters clears the cached instance, so a configuration change after the first redirect is still honoured. - Every template render resolved CompositeViewResolver from the bean factory. It is now held in a field, matching how this trait already caches the plugin manager, mime utility and layout selector. - The domain map constructor resolved the GrailsApplication and PersistentEntity, discarded them, then resolved both again to autowire the instance. They are now resolved once and passed down. The redirector is held in an AtomicReference rather than a volatile field: Groovy's trait field remapping drops the volatile modifier, and unlike the other cached values this object is constructed here after its setters run, so it needs safe publication.
GrailsWebRequest.getCurrentRequest() returned the resolved MultipartHttpServletRequest in place of the request Grails was bound to. That substitution is gone, so the method is now literally `return getRequest();`. The two can never disagree. getRequest() is final on Spring's ServletRequestAttributes and fixed at construction, and nothing wraps or replaces the request for the lifetime of a GrailsWebRequest: includes and forwards wrap only the response and dispatch the same request object, layout decoration swaps the response and re-renders against the original request, and async builds a new GrailsWebRequest around the request it is given. The two places that do cope with a later request wrapper avoid this method entirely - multipart through WebUtils.resolveMultipartRequest, and Spring Security by binding a fresh DelegatingGrailsWebRequest. All 65 framework call sites now use getRequest(). The method is deprecated rather than deleted so plugins keep compiling; removing it is a separate decision. - Move the "always the outermost request" note to the class javadoc, where it outlives the deprecated method - Keep getCurrentRequest in DelegatingGrailsWebRequest's @DeleGate exclusions. Delegating it would hand back the request from earlier in the filter chain, which is what that filter exists to prevent. Both reasons the exclusion list exists are now written down - Cover that filter with a spec; it had none - Stop JsonViewTemplateResolverSpec mocking GrailsWebRequest and stubbing getCurrentRequest(). It relied on the deprecated method being the only stubbable request accessor and produced an object whose two accessors disagreed; it now drives a real GrailsWebRequest over a MockHttpServletRequest
Covers controller action invocation (with and without allowedMethods, and a command-object action), the interceptor chain with a no-op and an observing registry, and collectControllerMappings - the uncached wrapper that runs on every request even when the URL mapping cache hits. The existing benchmarks measured GrailsWebRequest construction (12 ns) and multipart resolution (1.7 ns), neither of which is where request time goes.
getRequest() is final on ServletRequestAttributes, so getCurrentRequest() was the only stubbable request accessor on GrailsWebRequest. Tests that mocked it will see framework code take a different path now that it calls getRequest() directly.
This was referenced Aug 15, 2026
The filter sets the locale from the request unconditionally, but restored the previous LocaleContext only on the outermost dispatch. An include or forward therefore left the enclosing request with the locale it had installed, and replaced any TimeZoneAwareLocaleContext with a plain SimpleLocaleContext for the remainder of that request. The restore now happens on every invocation, matching the unconditional set. Only the GrailsWebRequest handling stays branched, since an include restores the previous web request rather than clearing it. This filter had no test coverage; adds one, including a case that fails without the change.
…8.0.x Upstream landed the mass-assignment hardening (apache#15947) and the clearMissing work (apache#15950), both of which rewrote the DataBindingUtils methods this branch had touched in "Cache the data binding collaborators and the databinding whitelist lookup". The conflict is resolved in favour of upstream everywhere the two overlap, so that the deny-by-default binding behaviour is exactly the one upstream shipped. Superseded by upstream and dropped from this branch: * The whitelist include-list caching in getBindingIncludeList. Upstream's rewrite already caches the negative result behind a NO_BINDING_INCLUDE_LIST sentinel and resolves the runtime bindable names only on a cache miss, and it keys the cache on whether deny-by-default is enabled, which this branch's single cache could not express. The method is taken from upstream verbatim. * The resolveBindingIncludeList helper. Upstream's getField / getPairedField / getStaticListFieldValue replace it and fix the same defect: the lookup no longer lets getDeclaredField throw for a class the AST transform never enhanced, so nothing is owed here any more. The helper also honoured only a whitelist declared on the class itself, whereas upstream deliberately walks the superclass chain, so DataBindingUtilsSpec now asserts that an inherited whitelist applies. Its test of the private include-list cache is dropped: the negative result is still covered through the public binding API, and upstream now keeps two caches rather than the one the test reached into. Kept from this branch: * The ContextBoundBeans cache of the data binding collaborators, which upstream does not touch. * Resolving the GrailsApplication once per bind and passing it down. It now travels through a private bindObjectToDomainInstance overload which runs upstream's include normalisation, so the include.isEmpty() / NO_BINDABLE_PROPERTIES handling and the clearMissing && explicitInclude gating apply on every path, including bindToCollection.
…er tests Holders keeps its application discovery strategies in a static list and consults them in registration order, and tests share a JVM fork. The spec registered its own strategy but did not clear the list first, so a strategy left behind by an earlier test - holding an application context that had since been closed - was asked first and threw IllegalStateException before the spec's strategy was reached. Clearing in setup as well as cleanup makes the spec independent of whatever ran before it.
codeconsole
requested review from
jamesfredley,
jdaugherty and
matrei
and removed request for
jdaugherty and
matrei
August 15, 2026 21:53
Contributor
|
Just as a FYI, this is breaking Grails 7 plugin interceptors:
At this point, I'm not sure how much that matters, but I wanted to call it out. |
PR apache#16071 landed a `grails-benchmarks` module using the same package root as `grails-web-benchmarks`, with a comparison tool (`BenchmarkComparator`, `JmhCompare`, `CommentPoster`, sharding and golden-file tests) that automates the before/after comparison this branch was doing by hand. Move the request-path benchmarks into it and delete `grails-web-benchmarks`. Ported, regrouped into the package-per-subsystem layout the report aggregates on, and reworked to that module's conventions - benchmarks in `src/jmh/java`, setup in Groovy fixtures under `src/main/groovy`: controllers ControllerActionBenchmark, ControllerMappingCollectionBenchmark interceptors InterceptorChainBenchmark web GrailsWebRequestBenchmark, MultipartResolutionBenchmark, RequestPropertyAccessBenchmark Because the fixtures live in `main`, which the jmh plugin puts on the jmh compile classpath, the `UrlMappingsDefinition` / `InterceptorFactory` / `RequestPropertyReader` interfaces and their `Class.forName` lookups are gone - the Java benchmarks call the Groovy fixtures directly. `UrlMappingBenchmark` is dropped. Its `matchCachedHit` and `matchRestfulUriCacheMiss` duplicate `UrlMappingsBenchmark.matchWarmCache` and `matchColdVariedKeys`. The one shape it measured that upstream did not - a cold URI that only the catch-all `"/$controller/$action?/$id?"` mapping can serve, so every earlier mapping is considered and rejected before the match succeeds - is added to `UrlMappingsBenchmark` as `matchColdCatchAllFallThrough`. On the existing fixture that costs ~568 ns against ~411 ns for a cold URI the first mapping serves. BASELINE.md is dropped: paired before/after numbers are what the new report tooling produces per pull request. Its measured results, from two full suites run back to back on an idle M4 Max under JDK 21.0.7 (2 forks, 5x1s warmup + 5x1s measurement), were: ControllerActionBenchmark.plainAction 34.795 -> 3.592 -89.7% InterceptorChainBenchmark.oneInterceptorNoOpRegistry 295.293 -> 127.541 -56.8% InterceptorChainBenchmark.threeInterceptorsNoOp 1116.942 -> 545.608 -51.2% InterceptorChainBenchmark.threeInterceptorsObserving 1972.929 ->1134.393 -42.5% GrailsWebRequestBenchmark.construct 16.283 -> 11.710 -28.1% UrlMappingBenchmark.matchRestfulUriCacheMiss 1549.992 ->1287.754 -16.9% UrlMappingBenchmark.matchDefaultMappingUriCacheMiss 1881.323 ->1595.361 -15.2% The request-attribute counts `ControllerActionBenchmark` prints at setup are the independent evidence for the controller result and are unchanged by the move: an action on a controller with no `allowedMethods` performs no attribute operations at all, while one that declares `allowedMethods` still performs 2/1/1. `grails-benchmarks/build.gradle` gains `:grails-controllers`, `:grails-web-databinding`, `:grails-mimetypes`, `micrometer-observation` and `spring-webmvc`. The suite stays opt-in: `build` compiles the benchmarks, only the explicit `jmh` task runs them.
codeconsole
added a commit
to codeconsole/grails-core
that referenced
this pull request
Aug 17, 2026
PR apache#16071 landed `grails-benchmarks`, which uses the same package root as the `grails-web-benchmarks` module this branch added, overlaps it, and automates the before/after comparison with `BenchmarkComparator` / `JmhCompare`. The request-path branch (apache#16149) folds the benchmarks worth keeping into `grails-benchmarks`, so this branch does not carry a second copy. Nothing is lost. What this branch changes - captured URL mapping names resolved from the match rather than from the bound request, which lets `collectControllerMappings` stop calling `resetParams()` and `configure(webRequest)` once per candidate - is measured by `ControllerMappingCollectionBenchmark` in `grails-benchmarks`, whose `twoCandidates` and `fourCandidates` URIs both match through the catch-all `"/$controller/$action?/$id?"` mapping and so exercise captured-name resolution on every candidate.
Three caches were added as Groovy trait fields. A trait is a mixin and its fields are replicated into every implementing class, so that is per-controller scope for things that are application scoped. Groovy also remaps trait statics per implementing class, so the "static" namespace cache was really one Class-keyed map per controller class, each holding a single entry; and the redirector needed an AtomicReference only because Groovy silently drops volatile on trait fields. - The namespace is a static property of the controller class and DefaultGrailsControllerClass already resolves it at construction, so the reflective read and its cache are both gone - the namespace now comes from the artefact registry, keyed on the class issuing the redirect rather than the one currently executing. - CompositeViewResolver moves to DefaultGrailsApplicationAttributes, alongside the other beans it describes as used very often. That object is one instance per servlet context, so the resolver is now resolved once for the application rather than once per controller. - The ResponseRedirector cache is removed rather than relocated. Its inputs are uniformly injected, so it is effectively a singleton, but the trait's setters are public API and are exercised after construction - RedirectMethodTests registers a listener on a live controller and expects the next redirect to notify it - so a shared instance would need a per-controller override path anyway. It is built per redirect again, as before. This also removes the fields from Interceptor, which implements the same trait.
Neither had a benchmark, which is how three unmeasured caches ended up in the controller traits.
Contributor
Author
|
@matrei agreed, wasn't benefiting the overall design. I think better without it. I revised the trait. Thanks |
…8.0.x # Conflicts: # grails-doc/src/en/guide/upgrading/upgrading80x.adoc
…8.0.x # Conflicts: # grails-spring-security/plugin/src/main/groovy/grails/plugin/springsecurity/web/UpdateRequestContextHolderExceptionTranslationFilter.groovy
✅ All tests passed ✅🏷️ Commit: f42924d Learn more about TestLens at testlens.app/docs. |
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
3 participants
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.
Reduces per-request work across the request path, and adds an opt-in JMH module so the
changes are measured rather than asserted.
Measured
Before/after on the same machine and JDK (21.0.7, Apple M4 Max), 2 forks x (5 warmup +
5 measurement):
ControllerActionBenchmark.plainActionInterceptorChainBenchmark.oneInterceptorNoOpRegistryInterceptorChainBenchmark.threeInterceptorsNoOpRegistryGrailsWebRequestBenchmark.constructUrlMappingBenchmark.matchRestfulUriCacheMissUrlMappingBenchmark.matchDefaultMappingUriCacheMissNothing regressed. Run them with:
What changed
requestis no longer replaced by the resolvedMultipartHttpServletRequest.It is discovered from the wrapper chain instead, so the security and method-override
wrappers survive.
request.getFile(..),params.myFileandbyte[]binding are unchanged.the five beans they cache. Now resolved once per servlet context.
AbstractHandlerMappinginstead of duplicating it, soGrails-mapped requests no longer miss what Spring adds there (currently the API version
deprecation interceptor). Two per-request interceptor allocations removed.
as BooleanSuppliercoercion ran even when the ObservationRegistry is a no-op.
no
allowedMethods. A plain action went from 8 request-attribute operations to none. It wasalso being emitted twice into the same method.
ApplicationContext, and caches thenegative result of the whitelist field lookup, which previously rebuilt a
NoSuchFieldExceptionon every bind for untransformed classes.
instead of resolving them per call.
runs. Candidates are skipped, never reordered, so precedence is unchanged.
DefaultUrlMappingInfo, and repairs aSpringSecurityUtilsbranch that read an attribute only that dead code ever wrote.Behaviour changes
Documented in the 8.0 upgrade guide, sections 45 and 46:
request instanceof MultipartHttpServletRequestand casts to it no longer work. The filemethods are unaffected.
spring.mvc.apiversion.*) are now emitted for Grails-mapped requests.LocaleContextis restored rather than cleared at the end of a request.GrailsWebRequest.getCurrentRequest()is deprecated in favour ofgetRequest(). Tests thatstubbed it need updating;
getRequest()isfinalon Spring'sServletRequestAttributes, sothat was the only stubbable accessor.
Limitations
additionally evidenced by bytecode (proxy allocations gone, attribute operations gone), but the
redirect/render caches have no number.
resolution, which Fix 16145: Oversized multipart uploads cannot be handled by Grails application code #16146 provides. The two overlap; see the discussion there.
collectControllerMappingsremains the largest cost on the path (~360-1200 ns per request,against 2.5 ns for a cached URL match). Addressed separately.