Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reintroduce synthesized annotation attribute value caching #24970

Closed
sbrannen opened this issue Apr 25, 2020 · 2 comments
Closed

Reintroduce synthesized annotation attribute value caching #24970

sbrannen opened this issue Apr 25, 2020 · 2 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression
Milestone

Comments

@sbrannen
Copy link
Member

Prior to the introduction of the MergedAnnotation in Spring Framework 5.2, our SynthesizedAnnotationInvocationHandler utilized a cache for annotation attribute values; whereas, the new SynthesizedMergedAnnotationInvocationHandler has no such caching in place.

Issues such as #24961 indicate a regression in performance caused by the lack of such an attribute value cache. For example, the required attribute in @RequestParam is looked up using the internal meta-model in the MergedAnnotation API twice per request for each @RequestParam in a given controller handler method.

Reintroducing the attribute value cache would avoid the unnecessary performance overhead associated with multiple lookups of the same attribute not only for use cases like RequestParam.required but also for equals() and toString() invocations on a synthesized annotation. Note that invocations of hashCode() also incur additional overhead for attribute value lookups but only once, since the resulting hash code is cached after the first computation.

@sbrannen sbrannen added type: regression A bug that is also a regression in: core Issues in core modules (aop, beans, core, context, expression) labels Apr 25, 2020
@sbrannen sbrannen added this to the 5.2.6 milestone Apr 25, 2020
@sbrannen sbrannen self-assigned this Apr 25, 2020
@sbrannen
Copy link
Member Author

Empirical Evidence

This is by no means a proper benchmark, but given the following modified test method from MergedAnnotationsTests, the approximate average runtime is:

  • 700 ms without caching
  • 400 ms with caching
@Test
void synthesizeWithTransitiveImplicitAliasesForAliasPair() throws Exception {
	TransitiveImplicitAliasesForAliasPairTestConfiguration config =
			TransitiveImplicitAliasesForAliasPairTestConfigurationClass.class.getAnnotation(
					TransitiveImplicitAliasesForAliasPairTestConfiguration.class);
	TransitiveImplicitAliasesForAliasPairTestConfiguration synthesized = MergedAnnotation.from(
			config).synthesize();
	assertThat(synthesized).isInstanceOf(SynthesizedAnnotation.class);

	IntStream.rangeClosed(1, 1_000_000).forEach(count -> {
		assertThat(synthesized.xml()).isEqualTo("test.xml");
		assertThat(synthesized.xml()).isEqualTo("test.xml");
		assertThat(synthesized.groovy()).isEqualTo("test.xml");
		assertThat(synthesized.groovy()).isEqualTo("test.xml");
		synthesized.toString();
		synthesized.hashCode();
		synthesized.equals(config);
	});
}

Although the caching introduces the use of a map to store the computed attribute values, we feel that the increase in performance is significant enough to justify the memory trade-off.

@sbrannen
Copy link
Member Author

Reopening to fully address toString() for synthesized annotations as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

1 participant