Optimize annotation metadata resolution performance #1717
Conversation
| * @param member The member | ||
| * @return THe {@link OptionalInt} value | ||
| */ | ||
| default @Nonnull OptionalInt intValue(@Nonnull Class<? extends Annotation> annotation, @Nonnull String member) { |
There was a problem hiding this comment.
I think the @Nonnull annotation may be redundant here. Kotlin treats Optional as non null without the annotation. I haven't tested OptionalInt, though I assume it works the same
There was a problem hiding this comment.
Thanks will remove them
| * @return The string values if it is present | ||
| */ | ||
| default @Nonnull String[] stringValues(@Nonnull Class<? extends Annotation> annotation, @Nonnull String member) { | ||
| return StringUtils.EMPTY_STRING_ARRAY; |
There was a problem hiding this comment.
Why doesn't this implementation default to getting the values the old way?
There was a problem hiding this comment.
Could do, but the old ways performance is not good. This method is overridden anyway be the implementation. I put this in there for backwards compatibility so if someone had implemented this interface (unlikely) it would still compile.
| .build() | ||
|
|
||
| expect: | ||
| av.intValue().asInt == 10 |
There was a problem hiding this comment.
I think this behavior is somewhat surprising. I can imagine someone using this and only receiving the first value and assuming that was all that was set in the annotation. Perhaps we should throw an exception in this case
There was a problem hiding this comment.
Annotation values are bit different, it is common to use arrays to represent "no value" in annotation definitions even if you only care about the first value. eg. Class[] foo() default {} or int[] bar() default {}
This is just a convenience method so that libraries can specify if they only care about a single int value or many. What is probably missing from the impl is all the variants for array types (like intValues() etc.)
| @@ -71,8 +71,7 @@ public void process(BeanDefinition<?> beanDefinition, ExecutableMethod<?, ?> met | |||
| boolean sensitive = configuration | |||
| .isSensitive() | |||
| .orElseGet(() -> beanDefinition | |||
| .getValue(Endpoint.class, "defaultSensitive", Boolean.class) | |||
| .orElse(Endpoint.SENSITIVE)); | |||
| .isTrue(Endpoint.class, "defaultSensitive")); | |||
There was a problem hiding this comment.
This method is automatically retrieving the default value if the value isn't specified?
There was a problem hiding this comment.
No it isn't, but in this case the default is false hence why it made sense for this use case, and the performance is better.
|
My only concern about this going into 1.1.x is the |
|
I will revert the |
Turns out
annotationMetadata.getValue(SomeAnnotation.class, "foo", String.class)is expensive in its current form. I don't want to break the behaviour of that method and type conversion is sometimes useful so this adds new methods likestringValue,intValue,longValueetc. which are optimized forms that are much much faster. Generally one should favour these methods unless you really want type conversion.