Reduce per-request work across the request path - #16149
Open
codeconsole wants to merge 18 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.
✅ All tests passed ✅🏷️ Commit: aaefad8 Learn more about TestLens at testlens.app. |
codeconsole
requested review from
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. |
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.