Check the path condition first when request mappings are matched without a direct path match

Aperta
  • Qualcosa non va?
#37,623 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
4/5
Tempo stimato
3-5 giorni
Idoneità per principianti
55/100
Tipo di issue
Funzionalità
Chiarezza
Specificata chiaramente
Stato di attività
Attiva
Stack tecnologico
java, spring
Ambito
backend, performance

Direzione di ricerca

Read RequestMappingInfo.getMatchingCondition in the MVC and WebFlux source files named in the diff, then review MappingRegistry in both AbstractHandlerMethodMapping files. Run :spring-webmvc:check and :spring-webflux:test; the reported integration failures should match unpatched main. Done means path conditions run first, registry iteration uses registration order, and matching handlers and test results remain unchanged.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

status: waiting-for-triage

When a request has no direct path match (any URL with a URI variable), AbstractHandlerMethodMapping.lookupHandlerMethod evaluates RequestMappingInfo.getMatchingCondition for every registration. That method checks the methods, params, headers, consumes, produces and version conditions before the path. With many mappings, almost every candidate pays for those checks and is only then rejected by the path. The method checks alone call request.getMethod() 3–4 times per mapping (method condition plus CorsUtils.isPreFlightRequest in the method, consumes and produces conditions), through the request wrappers Spring Security adds.

Production impact. Our Spring MVC app (Boot 4.2.0-RC1, Framework 7.1.0-RC1, Kotlin) has about 2,500 handler mappings, half with URI variables. A 3-hour JFR recording from production (settings=profile, 13,207 execution samples) shows:

  • AbstractHandlerMethodMapping.lookupHandlerMethod: 24.7% of all CPU samples, 64% of the samples under DispatcherServlet. Nearly all of it is in addMatchingMappings over all registrations.
  • Top self frames: HttpServletRequestWrapper.getMethod 9.5% (from RequestMethodsRequestCondition and CorsUtils.isPreFlightRequest), HashMap$HashIterator.nextNode 3.6% (iterating the registry), and PathPattern.hasLength / TreeSet.first from PathPatternsRequestCondition.
  • 38,783 requests in the window (median 3.3 ms), so the lookup accounts for very roughly 0.8 ms per request.

Proposed change (MVC and WebFlux, 4 files):

  1. In RequestMappingInfo.getMatchingCondition, check the path condition(s) first. The other conditions have no side effects that depend on the order (ProducesRequestCondition only caches the parsed Accept header, and that attribute is cleared in handleMatch/handleNoMatch). The result of a match is unchanged; mappings with a non-matching path are simply rejected before the other conditions run. The custom condition stays last.
  2. Make MappingRegistry.registry a LinkedHashMap. The fallback iterates all registrations, and a HashMap keySet iterator walks the empty buckets of the table. Iteration order also becomes registration order instead of hash order.

Benchmark. Our app's 439 controllers were loaded from the boot jar, and their mappings registered through RequestMappingHandlerMapping.getMappingForMethod + registerMapping (2,547 mappings). Each lookup goes through getHandlerInternal on a MockHttpServletRequest wrapped 3 times. Median µs per lookup, JDK 27, Apple M5 Max:

Request 7.1.0-RC1 path first + linked registry
GET /rolle/{personReference:p2[0-9a-z]{7}}/{slug} (Host condition; 76% of our traffic) 112 41 38 (−66%)
GET /api/invoices/{id}/payment-links/{linkId} 123 57 49 (−60%)
PUT /api/invoices/{invoiceId}/voucher 72 57 48 (−33%)
no matching mapping 157 86 74 (−53%)
direct path match (unaffected) 0.6 0.6 0.6

Every request resolved to the same handler method in all three variants.

Tests. On main (ed4e0f896b) with the patch, :spring-webmvc:check passes (2,905 tests, checkstyle). :spring-webflux:test gives the same results as unpatched main: on my machine, three integration tests fail identically with and without the patch (DispatcherHandlerIntegrationTests, CoroutinesIntegrationTests and RequestMappingIntegrationTests fail with "failed to respond" for Reactor Netty/Jetty).

Possible follow-ups. With the path checked first, about a third of the remaining lookup time is ServletRequestPathUtils.getParsedRequestPath(request), which reads the parsed path from a request attribute (through the wrappers) once per mapping. Passing the parsed path down, or indexing registrations by their first literal path segment (similar to the trie idea in #24901), would remove most of the rest. Both are larger changes, so I left them out.

Apply. The patch is a plain diff against main that you can take into a PR in whatever form suits you. From the repository root:

curl -fsSL https://gist.githubusercontent.com/gregjotau/22f7137b392b771d5c74182cbfc641f2/raw/spring-mvc-lookup-path-first.patch | git apply
Diff 
diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java
index b2e9acf8ca..bc93aa3138 100644
--- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java
+++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/AbstractHandlerMethodMapping.java
@@ -22,7 +22,7 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -462,7 +462,8 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 	 */
 	class MappingRegistry {
 
-		private final Map<T, MappingRegistration<T>> registry = new HashMap<>();
+		// Linked for faster iteration: lookups without a direct path match iterate over all registrations.
+		private final Map<T, MappingRegistration<T>> registry = new LinkedHashMap<>();
 
 		private final MultiValueMap<String, T> pathLookup = new LinkedMultiValueMap<>();
 
diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java
index 900c9edd47..bff5a1cf0a 100644
--- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java
+++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/RequestMappingInfo.java
@@ -242,6 +242,12 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 	 */
 	@Override
 	public @Nullable RequestMappingInfo getMatchingCondition(ServerWebExchange exchange) {
+		// Check the path first: when a request has no direct path match, every mapping is checked,
+		// and the path rejects nearly all of them more cheaply than the other conditions.
+		PatternsRequestCondition patterns = this.patternsCondition.getMatchingCondition(exchange);
+		if (patterns == null) {
+			return null;
+		}
 		RequestMethodsRequestCondition methods = this.methodsCondition.getMatchingCondition(exchange);
 		if (methods == null) {
 			return null;
@@ -267,10 +273,6 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 		if (version == null) {
 			return null;
 		}
-		PatternsRequestCondition patterns = this.patternsCondition.getMatchingCondition(exchange);
-		if (patterns == null) {
-			return null;
-		}
 		RequestConditionHolder custom = this.customConditionHolder.getMatchingCondition(exchange);
 		if (custom == null) {
 			return null;
diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java
index 0261368eca..cd42d51664 100644
--- a/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java
+++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/handler/AbstractHandlerMethodMapping.java
@@ -22,7 +22,7 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -542,7 +542,8 @@ public abstract class AbstractHandlerMethodMapping<T> extends AbstractHandlerMap
 	 */
 	class MappingRegistry {
 
-		private final Map<T, MappingRegistration<T>> registry = new HashMap<>();
+		// Linked for faster iteration: lookups without a direct path match iterate over all registrations.
+		private final Map<T, MappingRegistration<T>> registry = new LinkedHashMap<>();
 
 		private final MultiValueMap<String, T> pathLookup = new LinkedMultiValueMap<>();
 
diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java
index 80779574da..c30a674f46 100644
--- a/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java
+++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/RequestMappingInfo.java
@@ -419,6 +419,22 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 	@SuppressWarnings("removal")
 	@Override
 	public @Nullable RequestMappingInfo getMatchingCondition(HttpServletRequest request) {
+		// Check the path first: when a request has no direct path match, every mapping is checked,
+		// and the path rejects nearly all of them more cheaply than the other conditions.
+		PathPatternsRequestCondition pathPatterns = null;
+		if (this.pathPatternsCondition != null) {
+			pathPatterns = this.pathPatternsCondition.getMatchingCondition(request);
+			if (pathPatterns == null) {
+				return null;
+			}
+		}
+		PatternsRequestCondition patterns = null;
+		if (this.patternsCondition != null) {
+			patterns = this.patternsCondition.getMatchingCondition(request);
+			if (patterns == null) {
+				return null;
+			}
+		}
 		RequestMethodsRequestCondition methods = this.methodsCondition.getMatchingCondition(request);
 		if (methods == null) {
 			return null;
@@ -443,20 +459,6 @@ public final class RequestMappingInfo implements RequestCondition<RequestMapping
 		if (version == null) {
 			return null;
 		}
-		PathPatternsRequestCondition pathPatterns = null;
-		if (this.pathPatternsCondition != null) {
-			pathPatterns = this.pathPatternsCondition.getMatchingCondition(request);
-			if (pathPatterns == null) {
-				return null;
-			}
-		}
-		PatternsRequestCondition patterns = null;
-		if (this.patternsCondition != null) {
-			patterns = this.patternsCondition.getMatchingCondition(request);
-			if (patterns == null) {
-				return null;
-			}
-		}
 		RequestConditionHolder custom = this.customConditionHolder.getMatchingCondition(request);
 		if (custom == null) {
 			return null;
Lingua principale
Java
Stelle
60.3k
Fork
38.7k
Merge medio
1g 10h
PR unite (30g)
33

Preparare l'ambiente

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di spring-projects/spring-framework

Tutte le issue di spring-projects/spring-framework

Issue simili

Altre issue su Java

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.