Skip to content

Handler methods rediscover parameter names on every invocation, which is costly for Kotlin #37625

Open

Description

@gregjotau

InvocableHandlerMethod.getMethodArgumentValues calls parameter.initParameterNameDiscovery(this.parameterNameDiscoverer) for each parameter on every invocation. The MethodParameter instances belong to the HandlerMethod and are shared across invocations. initParameterNameDiscovery re-arms discovery, so the next getParameterName() discovers the name again even though it was already cached in parameterName.

Since 6.1 (8c6287e, "Expose parameter/field name for non-JavaBeans type conversion"), TypeConverterSupport.convertIfNecessary(value, requiredType, MethodParameter) calls methodParam.getParameterName(). AbstractNamedValueMethodArgumentResolver converts every @PathVariable, @RequestParam, @RequestHeader etc. through it. So each such argument now repeats parameter name discovery on every request.

For Kotlin classes, DefaultParameterNameDiscoverer tries KotlinReflectionParameterNameDiscoverer first. That calls ReflectJvmMapping.getKotlinFunction(method), which goes through the functions of the whole KClass, so the cost grows with the size of the controller. The same pattern exists in the WebFlux and messaging InvocableHandlerMethod variants.

Production impact. Our Spring MVC app (Boot 4.2.0-RC1, Framework 7.1.0-RC1, Kotlin 2.5) has about 440 Kotlin controllers. In a 3-hour production JFR recording (13,207 execution samples, 38,783 requests), KotlinReflectionParameterNameDiscoverer.getParameterNames accounts for 0.5% of all CPU samples. About 0.3% comes from per-request argument conversion (TypeConverterSupport.convertIfNecessary → MethodParameter.getParameterName); the rest is at startup. Most of it is in KClassImpl$Data.getAllMembers and the stream in KotlinReflectionParameterNameDiscoverer. All of it repeats work whose result is already stored in the MethodParameter.

Proposed change. initParameterNameDiscovery keeps a name that has already been discovered (spring-core, 1 line plus Javadoc and a test):

public void initParameterNameDiscovery(@Nullable ParameterNameDiscoverer parameterNameDiscoverer) {
	if (this.parameterName == null) {
		this.parameterNameDiscoverer = parameterNameDiscoverer;
	}
}

Passing null already could not clear a discovered name, because getParameterName() returns the cached name once the discoverer is unset. The only behaviour change is that a different non-null discoverer no longer replaces a name that was already found. A parameter whose name could not be discovered still retries as before. An alternative is to initialize discovery once per HandlerMethod in the four InvocableHandlerMethod classes, but the discoverer is set per handler adapter, so the change in MethodParameter seemed the smallest one that covers all of them.

Benchmark. This resolves the arguments of real controller methods from our boot jar the way RequestMappingHandlerAdapter does: a new ServletInvocableHandlerMethod per call, getMethodArgumentValues, the adapter's default argument resolvers and ServletRequestDataBinderFactory, and DefaultParameterNameDiscoverer. It reports the median ns per invocation, on JDK 27 and an Apple M5 Max:

Handler method 7.1.0-RC1 patched
personRole(@PathVariable String, @PathVariable String, HttpServletResponse), in a class with 66 functions 2,712 1,291 (−52%)
leads(HttpServletRequest, 23 × @RequestParam), in a class with 73 functions 26,410 18,825 (−29%)

Repeated runs were within 5%, and the resolved arguments were identical.

Tests. On main (ed4e0f8) with the patch, using JDK 25:

  • :spring-core:check passes.
  • :spring-beans:test, :spring-web:test, :spring-webmvc:test, :spring-messaging:test and :spring-webflux:test pass: 18,246 tests across the six modules, no failures.
  • The new MethodParameterTests.parameterNameIsDiscoveredOnceAcrossInitializations fails without the change: the discoverer is called 3 times instead of once.

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/61a95e162d42c65d12396722c26babab/raw/spring-method-parameter-name-once.patch | git apply
Diff
diff --git a/spring-core/src/main/java/org/springframework/core/MethodParameter.java b/spring-core/src/main/java/org/springframework/core/MethodParameter.java
index 2ea648b2..6b0f8d05 100644
--- a/spring-core/src/main/java/org/springframework/core/MethodParameter.java
+++ b/spring-core/src/main/java/org/springframework/core/MethodParameter.java
@@ -686,10 +686,15 @@ public class MethodParameter {
 	 * <p>Note: As of 7.0.3, a default parameter name discoverer is available.
 	 * This init method can be used to override the default discoverer or to
 	 * suppress discovery (passing {@code null}).
+	 * <p>A parameter name that has already been discovered is retained.
 	 * @see DefaultParameterNameDiscoverer#getSharedInstance()
 	 */
 	public void initParameterNameDiscovery(@Nullable ParameterNameDiscoverer parameterNameDiscoverer) {
-		this.parameterNameDiscoverer = parameterNameDiscoverer;
+		// Handler methods share their parameters and initialize discovery on every invocation:
+		// discovering the name again each time is costly, in particular for Kotlin functions.
+		if (this.parameterName == null) {
+			this.parameterNameDiscoverer = parameterNameDiscoverer;
+		}
 	}
 
 	/**
diff --git a/spring-core/src/test/java/org/springframework/core/MethodParameterTests.java b/spring-core/src/test/java/org/springframework/core/MethodParameterTests.java
index e3a95b39..b563d891 100644
--- a/spring-core/src/test/java/org/springframework/core/MethodParameterTests.java
+++ b/spring-core/src/test/java/org/springframework/core/MethodParameterTests.java
@@ -24,7 +24,9 @@ import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
 import java.util.ArrayList;
 import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicInteger;
 
+import org.jspecify.annotations.Nullable;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 
@@ -258,6 +260,27 @@ class MethodParameterTests {
 		assertThat(longParameter.getParameterName()).isEqualTo("lng");
 	}
 
+	@Test
+	void parameterNameIsDiscoveredOnceAcrossInitializations() {
+		AtomicInteger discoveries = new AtomicInteger();
+		ParameterNameDiscoverer discoverer = new ParameterNameDiscoverer() {
+			@Override
+			public @Nullable String @Nullable [] getParameterNames(Method method) {
+				discoveries.incrementAndGet();
+				return new String[] {"str", "lng"};
+			}
+			@Override
+			public @Nullable String @Nullable [] getParameterNames(Constructor<?> ctor) {
+				return null;
+			}
+		};
+		for (int i = 0; i < 3; i++) {
+			stringParameter.initParameterNameDiscovery(discoverer);
+			assertThat(stringParameter.getParameterName()).isEqualTo("str");
+		}
+		assertThat(discoveries).hasValue(1);
+	}
+
 	@Test
 	void jspecifyNullableParameter() {
 		assertThat(jspecifyNullableParameter.isOptional()).isTrue();
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions