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

Initial set of MergedAnnotations commits #22586

Closed
wants to merge 19 commits into from

Conversation

philwebb
Copy link
Member

This is the initial set of work in introducing a new MergedAnnotations API to Spring Framework. See #22560 for more background.

It seemed more sensible to send a single PR covering a number of issues, rather than creating distinct PRs for each subtask.

In order to reduce the likelihood of regressions, I've ran a complete build of both Spring Framework and Spring Boot which calls both the original and new code and compares the results. I've left these commits in this PR as I figure they might be a useful reference.

I've also used https://github.com/wilkinsona/spring-boot-benchmark-harness to check that there are no major performance implications with the new code. The following shows the Spring Boot Tomcat sample application running against a baseline from c0ddaae.

Main method:

|       | Baseline |  New  |
| ----- | -------: | ----: |
|       |    1.725 | 1.671 |
|       |    1.731 | 1.680 |
|       |    1.648 | 1.684 |
|       |    1.684 | 1.775 |
|       |    1.707 | 1.730 |
|       |    1.667 | 1.722 |
|       |    1.689 | 1.713 |
|       |    1.691 | 1.737 |
|       |    1.697 | 1.723 |
|       |    1.688 | 1.725 |
|  Mean |    1.693 | 1.716 |
| Range |    0.083 | 0.104 |
|  Diff =    0.023     

Special thanks should go to @wilkinsona who helped profile and identify several performance improvements.

@philwebb philwebb requested review from jhoeller and sbrannen March 13, 2019 05:41
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Mar 13, 2019
philwebb added 19 commits March 13, 2019 23:40
Add AssertJ as a test scope dependency for all sub-projects.

Closes spring-projectsgh-22561
Add a public variant of `getDeclaredMethods` that defensively copies the
cached methods array. This is often more faster and more convenient
for users than calling `doWithLocalMethods`. We still retain most of the
benefits of the cache, namely fewer security manager calls and not as
many `Method` instances being created.

Closes spring-projectsgh-22580
Add a new test class to help cover annotation introspection failure
handling. These tests were previously missing and are important to
ensure that annotation util code changes don't introduce regressions.

See spring-projectsgh-21697
Add new `MergedAnnotations` and `MergedAnnotation` interfaces that
attempt to provide a uniform way for dealing with merged annotations.

Specifically, the new API provides alternatives for the static methods
in `AnnotationUtils` and `AnnotatedElementUtils` and supports Spring's
comprehensive annotation attribute `@AliasFor` features. The interfaces
also open the possibility of the same API being exposed from the
`AnnotationMetadata` interface.

Additional utility classes for collecting, filtering and selecting
annotations have also been added.

Typical usage for the new API would be something like:

	MergedAnnotations.from(Example.class)
		.stream(MyAnnotation.class)
		.map(a -> a.getString("value"))
		.forEach(System.out::println);

Closes spring-projectsgh-21697
Create internal variants of the existing `AnnotationUtils` and
`AnnotatedElementUtils` classes and migrate the existing classes to
use them.

The internal variants will be used to check that the same results are
given as we migrate the utils methods to use the new `MergedAnnotations`
API.

See spring-projectsgh-22562
Migrate all possible `AnnotationUtils` and `AnnotatedElementUtils`
method to the `MergedAnnotation` API, verify results against the old
implementations.

All migrated methods now call both the new API and the old version and
ensure that the same results or exceptions are raised. A full build of
both Spring Framework and Spring Boot has been executed to ensure, as
much as possible, that the migration does not cause unexpected
regressions.

See spring-projectsgh-22562
Delete `InternalAnnotationUtils` and `InternalAnnotatedElementUtils` and
migrate exclusively to the new `MergedAnnotations` API.

Closes spring-projectsgh-22562
Update `ConfigurationClassParser` to skip `java.lang.annotation` types
which were often processed but would never provide useful results. Also
use a single shared immutable `SourceClass` instance to represent
`Object.class`.

Closes spring-projectsgh-22563
Update `ConcurrentReferenceHashMap` to make some methods more inline
friendly, and to manually inline a few others. These minor optimizations
don't make a great deal of difference for most applications, but seem
worthwhile since we use `ConcurrentReferenceHashMap` for many internal
caches.

Closes spring-projectsgh-22566
Add some additional empty array constants to `ReflectionUtils` to save
us creating new arrays for zero length results.

See spring-projectsgh-22567
Add an early exit to `StringUtils.cleanPath` to save array creating and
string concatenation. With a typical Spring application, the `cleanPath`
method can be called over 600 times, often with a path constructed by
a `ClassPathResource` that is likely to already be clean.

Closes spring-projectsgh-22568
Add a cache to `BridgeMethodResolver` to help with repeated calls to
resolve the same methods. Since bridge method resolution can be somewhat
expensive (especially when resolving generics), and the number of bridge
methods is quite small, a cache generally helps.

This commit also simplifies the code a little by calling `doWithMethods`
directly rather than relying on `ReflectionUtils.getAllDeclaredMethods`
to do so. The methods list is now always created, but we save the list
creation that `getAllDeclaredMethods` used to do.

Closes spring-projectsgh-22579
Refine the element filtering performed by `AnnotationsScanner` to also
cover `org.springframework.util` and most `com.sun` classes which turn
out to be referenced quite frequently and which we know contain no
useful annotations.

See spring-projectsgh-21697
@jhoeller jhoeller self-assigned this Mar 14, 2019
@jhoeller jhoeller added type: task A general task in: core Issues in core modules (aop, beans, core, context, expression) and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Mar 14, 2019
@jhoeller jhoeller added this to the 5.2 M1 milestone Mar 14, 2019
@jhoeller jhoeller closed this in 5e15338 Mar 23, 2019
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: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants