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

オープン
  • 何か問題がありますか？
#37,623 コメント 0 件 リアクション 0 件 担当者 0 名 GitHub で見る

メンテナーはふだん 1 日以内に返信

まだ誰も着手していません。

評価

難易度
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日 10時間
マージ済み PR（30日）
33

環境構築

はじめの一歩

  1. issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
  2. 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
  3. リポジトリをフォークし、ブランチを切って変更します。
  4. issue 番号を参照したプルリクエストを送ります。

spring-projects/spring-framework のほかの issue

spring-projects/spring-framework の issue をすべて見る

似ている issue

Java の issue をもっと見る

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。