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

未关闭
  • 哪里不对？
#37,623 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看

维护者通常 1 天内回复

还没有人认领这个 Issue。

评估

难度
4/5
预计耗时
3-5 天
新手友好度
55/100
Issue 类型
功能
描述清晰度
描述清楚
活跃度
活跃
技术栈
java, spring
领域
backend, performance

调研方向

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.

由索引模型根据 Issue 内容生成。

描述

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;
主要语言
Java
星标
60.3k
派生
38.7k
平均合并
1 天 1 小时
30 天内合并 PR
33

环境准备

  • 没有 Dockerfile 或 Docker Compose 文件
  • 没有 Pull Request 模板
  • 阅读贡献指南

从这里开始

  1. 先读完整个 Issue，再读项目的贡献指南。
  2. 在 Issue 下留言说明你要接手 —— 这能避免两个人做同样的事。
  3. Fork 仓库，在一个分支上完成修改。
  4. 提交 Pull Request，并在描述里引用这个 Issue 编号。

spring-projects/spring-framework 的其他 Issue

查看 spring-projects/spring-framework 的全部 Issue

相似的 Issue

更多 Java Issue

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。