JDK 8 RFR for JDK-7185456 : (ann) Optimize Annotation handling in java/sun.reflect.* code for small number of annotationsC

[Peter Levart]

On 03/26/2013 11:43 PM, Joe Darcy wrote:
> Hello,
>
> Please review this refactoring of how annotations objects are created:
>
>     JDK-7185456 : (ann) Optimize Annotation handling in 
> java/sun.reflect.* code for small number of annotationsC
>     http://cr.openjdk.java.net/~darcy/7185456.0/
>
> In brief, an annotation object is backed by a hash map with one entry 
> per method defined on the annotation type. Currently the default 
> HashMap constructor which uses a default capacity of 16 is used. Since 
> most annotation type define many fewer methods, some space is wasted. 
> The patch (inline below) "right sizes" the HashMap to match the number 
> of methods in the annotation type.

This optimization is good, although these are HashMaps that hold default 
values, methods and types of members - not member values of actual 
annotation objects (only one instance of each such map exists per 
annotation interface). The Map holding values is created in 
AnnotationParser (line 230) (it's actually LinkedHashMap, presumably to 
maintain order for toString method). This is already pre-sized correctly 
as it's constructor takes defaults which are later overridden in a 
parsing loop which can re-size the map as it fits. This re-sizing could 
be eliminated though. I also don't know whether having LinkedHashMap 
instead of plain HashMap is necessary, since it is initialized with 
defaults from plain HashMap (which is hashCode % capacity ordered) and 
only some of defaults are overridden in parsing loop in general. For 
example, having this annotation:

@interface Ann {
     String one();
     String two() default "2";
}

and usage:

@Ann(one = "1")

...toString will print: @Ann(two = "2", one = "1")

So replacing LinkedHashMap with plain HashMap in parser would be another 
optimization opportunity.

This is just speculation: It might be possible to replace it with 
IdentityHashMap which is even more compact. Since keys are always 
obtained from Method.getName() and are therefore pre-interned, so 
interning them before putting them into the map in parser would present 
no-overhead. Lookup (in AnnotationInvocationHandler) is always taking 
the Method.getName so this is also taken care of already.

Regards, Peter

>
> Thanks,
>
> -Joe
>
> --- old/src/share/classes/sun/reflect/annotation/AnnotationType.java 
> 2013-03-26 15:37:22.000000000 -0700
> +++ new/src/share/classes/sun/reflect/annotation/AnnotationType.java 
> 2013-03-26 15:37:22.000000000 -0700
> @@ -45,19 +45,18 @@
>       * types.  This matches the return value that must be used for a
>       * dynamic proxy, allowing for a simple isInstance test.
>       */
> -    private final Map<String, Class<?>> memberTypes = new 
> HashMap<String,Class<?>>();
> +    private final Map<String, Class<?>> memberTypes;
>
>      /**
>       * Member name -> default value mapping.
>       */
> -    private final Map<String, Object> memberDefaults =
> -        new HashMap<String, Object>();
> +    private final Map<String, Object> memberDefaults;
>
>      /**
>       * Member name -> Method object mapping. This (and its assoicated
>       * accessor) are used only to generate 
> AnnotationTypeMismatchExceptions.
>       */
> -    private final Map<String, Method> members = new HashMap<String, 
> Method>();
> +    private final Map<String, Method> members;
>
>      /**
>       * The retention policy for this annotation type.
> @@ -105,6 +104,9 @@
>                  }
>              });
>
> +        memberTypes = new HashMap<String,Class<?>>(methods.length+1, 
> 1.0f);
> +        memberDefaults = new HashMap<String, Object>(0);
> +        members = new HashMap<String, Method>(methods.length+1, 1.0f);
>
>          for (Method method :  methods) {
>              if (method.getParameterTypes().length != 0)