Request for review: 8004698: Implement Core Reflection for Type Annotations

[Peter Levart]

On 01/22/2013 01:47 PM, Joel Borggrén-Franck wrote:
> Hi Peter,
>
> Thanks for your comments, see inline,
>
> On 01/19/2013 06:11 PM, Peter Levart wrote:
>
>> I see, there is a dilema how to cache type annotations. To satisfy
>> single-annotation-returning methods and repeating annotation logic, a
>> HashMap would be a logical choice, but it has much bigger footprint than
>> simple arrays of annotations...
>>
>
> I don't prioritize footprint for classes that have runtime visible 
> type annotations. Those classes should be very few, and as a user you 
> are making an explicit choice of adding metadata when you use runtime 
> visible (type) annotations.
>
> So here is my list of priorities (and by un-annotated I mean without 
> runtime visible annotations):
>
> - Unannotated classes/fields/methods should have as small as possible 
> extra footprint, this is most important.

Hello Joel,

I imagine there will be additional places where references to 
Map<Class<? extends Annotation>, Annotation> will be added to hold 
cached annotations. To satisfy your priority #1 I would consistently 
make sure that when there are no annotations to put into the map, a 
singleton Collections.emptyMap() reference is put in place. This is 
currently not so for regular annotations (should be corrected). I know 
of runtime tools that "scan" classes for particular annotations, just to 
find out that majority of them are without annotations. If empty 
instances of HashMap(16) pop-up as a result of such scan, lots of memory 
is wasted.

>
> - Classes/fields/methods without type annotations should have 
> negligible footprint over classes/fields/methods with only regular 
> annotations.

You meant "Classes/fields/methods *with* type annotations should have 
negligible footprint over classes/fields/methods with only regular 
annotations", right?

Regards, Peter

>
>> I experimented with an idea of using a special purpose immutable Map
>> implementation:
>>
>> https://github.com/plevart/jdk8-tl/blob/anno-map/jdk/src/share/classes/sun/reflect/annotation/UniqueIndex.java 
>>
>>
>> and:
>>
>> https://github.com/plevart/jdk8-tl/blob/anno-map/jdk/src/share/classes/sun/reflect/annotation/AnnotationMap.java 
>>
>>
>> This is just a preview. I still have to create some tests to see how it
>> compares to HashMap. Being an immutable wrapper for a single array of
>> annotations it has a nice feature that a reference to it can be 
>> passed to
>> other threads via a data race with no danger of data inconsistency, 
>> which
>> simplifies caching mechanisms and helps scalability.
>> Might even be a space-saving replacement for existing HashMaps of
>> annotations. Depending on performance, of course.
>> What do you think? Is it worth pursuing this further?
>>
>
> I'd be very hesitant to introduce a new hashmap like type. Especially 
> since IMHO footprint when we actually have annotations isn't as 
> critical as when we are annotation free.
>
> My initial approach is probably to dust of your patches for an 
> annotations cache on Class and adopt it for type annotations, and use 
> on Field/Method/Constructor/(TypeVar).
>
> Thank you again for your work on this!
>
> cheers
> /Joel
>
>> Regards, Peter
>>
>> On Jan 17, 2013 5:24 PM, "Joel Borggrén-Franck" <joel.franck at oracle.com
>> <mailto:joel.franck at oracle.com>> wrote:
>>
>>     Hi,
>>
>>     Here is a webrev for core reflection support for type annotations:
>>
>>     http://cr.openjdk.java.net/~jfranck/8004698/webrev.00/
>>
>>     This code is based on the tl/jdk repository, but in order to run the
>>     tests you need a javac with type annotations support and also a 
>> recent
>>     copy of the VM that includes this change set:
>> http://hg.openjdk.java.net/hsx/hotspot-main/hotspot/rev/35431a769282
>>     (This also means this code can't be pushed until all supporting 
>> pieces
>>     are in place.)
>>
>>     Type annotations are still a bit of a moving target so there will be
>>     follow up work on this. There is no caching of annotations in this
>>     version and I am not sure how we want to do the space/time trade 
>> of for
>>     type annotations.
>>
>>     cheers
>>     /Joel
>>