JDK 8 (initial) RFR for JDK-8005294 : Consider default methods for additions to AnnotatedElement

[Joe Darcy]

Hi Peter,

The concerns you raise below are some of the trade-offs I was thinking 
of as I was working on the current implementation :-)

 From a specification perspective, I'd like the @implSpec for this 
method to allow different combinations of calling 
getDeclaredAnnotation(Class) and getDeclaredAnnotations.

There are several concerns which influence the performance of different 
implementation options for getDeclaredAnnotationsByType:

* How often (and by how much) is getDeclaredAnnotation(Class) faster 
than iterating over getDeclaredAnnotations?
* Related question: how many annotations does getDeclaredAnnotations return?
* How often is an annotation both directly present and indirectly 
present on the same element?

Depending on the assumptions one makes, an implementation that is 
primarily based on getDeclaredAnnotation(Class) (one to two calls with 
getDeclaredAnnotations as a backstop) can have better expected 
performance than a method that always does a single call to 
getDeclaredAnnotations. On the other hand, the 
getDeclaredAnnotations-based approach will have a better worst-case 
behavior (one call to another AnnotedElement method vs three calls).

I don't think the performance of this default method should be that 
important since in my estimation most AnnotatedElement implementations 
are in JDK and will have more optimized implementations based on 
lower-level data structures. We often try to improve the expected case 
performance vs the worst case so I would lean toward a 
getDeclaredAnnotation(Class)-based implementation rather than a soley 
getDeclaredAnnotations-based one.

Thanks for the comments,

-Joe

On 10/25/2013 08:56 AM, Peter Levart wrote:
> Hi Joe,
>
> The code below is more optimal only in AnnotatedElement 
> implementations that don't override getDeclaredAnnotation(Class) with 
> more optimal implementation based on a HashMap lookup. Your 
> implementation avoids calling getDeclaredAnnotations() in common cases 
> (where only directly present or only in-directly present annotations 
> exist), but as this is a default method, it must be assumed that it 
> will be invoked on AnnotatedElement implementations that don't 
> override it and those presumably also don't override  default 
> getDeclaredAnnotation(Class) which is based on 
> getDeclaredAnnotations() and interation over returned elements. In 
> this situation, your code does 2 (or in uncommon case 3) calls to 
> getDeclaredAnnotations() with iteration over elements, while the below 
> code only does 1...
>
> On 10/25/2013 05:03 PM, Peter Levart wrote:
>> Hi Joe,
>>
>> I propose some "premature" optimization for 
>> AnnotatedElement.getDeclaredAnnotationsByType(). This implementation 
>> calls either getDeclaredAnnotatios() once or 
>> getDeclaredAnnotation(Class) once, depending on whether the 
>> annotation type is repeatable or not respectively (this check also 
>> requires a getDeclaredAnnotation() call, but I propose to extend 
>> AnnotationType in the future to provide with cached containerClass 
>> directly). Also, it is about 7x faster to use 
>> annotationClass.isInstance(ann) to check for the type of annotation 
>> instead of ann.annotationType() == annotationClass which goes through 
>> annotation Proxy, etc...
>>
>> Here it is:
>>
>>     default <T extends Annotation> T[] 
>> getDeclaredAnnotationsByType(Class<T> annotationClass) {
>>         Objects.requireNonNull(annotationClass);
>>         Repeatable repeatable = 
>> annotationClass.getDeclaredAnnotation(Repeatable.class);
>>
>>         // non-repeatable: just delegate to getDeclaredAnnotation and 
>> return wrapped result
>>         if (repeatable == null) {
>>             T ann = getDeclaredAnnotation(annotationClass);
>>             @SuppressWarnings("unchecked")
>>             T[] result = (T[]) Array.newInstance(annotationClass, ann 
>> == null ? 0 : 1);
>>             if (ann != null) result[0] = ann;
>>             return result;
>>         }
>>
>>         // repeatable: iterate all declared annotations, find 
>> directly and indirectly present
>>         // and also determine the order (directlyPresentFirst)...
>>         Class<? extends Annotation> containerClass = repeatable.value();
>>         T directlyPresent = null;
>>         T[] indirectlyPresent = null;
>>         boolean directlyPresentFirst = false;
>>         for (Annotation ann : getDeclaredAnnotations()) {
>>             if (directlyPresent == null && 
>> annotationClass.isInstance(ann)) {
>>                 directlyPresent = annotationClass.cast(ann);
>>                 if (indirectlyPresent != null) {
>>                     break;
>>                 }
>>             } else if (indirectlyPresent == null && 
>> containerClass.isInstance(ann)) {
>>                 indirectlyPresent = AnnotationSupport.getValueArray(ann);
>>                 if (directlyPresent != null) {
>>                     directlyPresentFirst = true;
>>                     break;
>>                 }
>>             }
>>         }
>>
>>         // concatenate directlyPresent and indirectlyPresent annotations
>>         @SuppressWarnings("unchecked")
>>         T[] result = (directlyPresent == null && indirectlyPresent != 
>> null)
>>                      ? indirectlyPresent
>>                      : (T[]) Array.newInstance(annotationClass,
>>                          (directlyPresent == null ? 0 : 1) +
>>                          (indirectlyPresent == null ? 0 : 
>> indirectlyPresent.length));
>>
>>         if (directlyPresent != null) {
>>             if (indirectlyPresent == null || indirectlyPresent.length 
>> == 0) {
>>                 result[0] = directlyPresent;
>>             } else {
>>                 result[directlyPresentFirst ? 0 : 
>> indirectlyPresent.length] = directlyPresent;
>>                 System.arraycopy(indirectlyPresent, 0, result,
>>                     directlyPresentFirst ? 1 : 0, 
>> indirectlyPresent.length);
>>             }
>>         }
>>
>>         return result;
>>     }
>>
>>
>>
>> Regards, Peter
>>
>> On 10/25/2013 10:40 AM, Joe Darcy wrote:
>>> Hi Joel and Peter,
>>>
>>> On 10/24/2013 07:10 AM, Peter Levart wrote:
>>>> Hi Joe,
>>>>
>>>> I see two problems with the implementation in 
>>>> *AnnotatedElementSupport*. The first is the treatment of 
>>>> declared-only annotations where the code returns either directly 
>>>> present or in-directly present repeatable annotations, but not 
>>>> both. So in the following example:
>>>>
>>>> @Ann(1)
>>>> @AnnCont({@Ann(2), @Ann(3)})
>>>>
>>>> it will only return [@Ann(1)], but I think it should return all of 
>>>> them [@Ann(1), @Ann(2), @Ann(3)] - does the spec. define that?
>>>>
>>>
>>> [snip]
>>>
>>> From your feedback (and a closer reading of the specifciation), I've 
>>> reworked the specifications and implemenations of the default 
>>> methods for get[Declared]AnnotationsByType:
>>>
>>> http://cr.openjdk.java.net/~darcy/8005294.2/
>>>
>>> Tests still need to be written, but this implementation should be 
>>> much closed to what is needed.
>>>
>>> Thanks,
>>>
>>> -Joe
>>
>