RFR: JDK-8054987: (reflect) Add sharing of annotations between instances of Executable

[Peter Levart]

Hi again Joel,

Looking at the Method and Constructor code, I can't escape the feeling 
that these classes could use a better way to publish their state. While 
root instances of Methods and Constructors are always handled correctly 
and never published using data races, the non-root instances that are 
handed to the user could be published by the user in a non-safe way so 
that various fields in the Method or Constructor could be observed by 
some other thread as half-initialized. Why couldn't the majority of 
fields in Method and Constructor (Field too) be final including the 
'root' field? For example:

http://cr.openjdk.java.net/~plevart/jdk9-dev/MemberSharedAnnotations/webrev.01/

Changing constructors would not break anything I think, since VM does 
not use them to construct root instances...

I know that this is off topic, but might it be considered for a separate 
enhancement?


Regards, Peter

On 08/14/2014 07:10 PM, Peter Levart wrote:
> Hi Joel,
>
> If you make Executable.declaredAnnotations field volatile, 
> synchronization is only necessary on the 'root' instance:
>
>
>     private transient volatile Map<Class<? extends Annotation>, 
> Annotation> declaredAnnotations;
>
>     private Map<Class<? extends Annotation>, Annotation> 
> declaredAnnotations() {
>         Map<Class<? extends Annotation>, Annotation> anns = 
> declaredAnnotations;
>         if (anns == null) {
>             Executable root = getRoot();
>             if (root != null) {
>                 declaredAnnotations = anns = root.declaredAnnotations();
>             } else {
>                 synchronized (this) {
>                     declaredAnnotations = anns = 
> AnnotationParser.parseAnnotations(
>                         getAnnotationBytes(),
> sun.misc.SharedSecrets.getJavaLangAccess().
> getConstantPool(getDeclaringClass()),
>                         getDeclaringClass());
>                 }
>             }
>         }
>         return anns;
>     }
>
>
> Regards, Peter
>
> On 08/14/2014 06:54 PM, Peter Levart wrote:
>> On 08/14/2014 08:47 AM, Joel Borggren-Franck wrote:
>>> On 2014-08-13, Joe Darcy wrote:
>>>> Hi Joel,
>>>>
>>>> Does your changeset alter the support (or non-support) of redefining
>>>> an annotation?
>>>>
>>> Hi Joe,
>>>
>>> It does not interact with the current non-support and I am convinced it
>>> wont hinder us in improving the situation.
>>>
>>> cheers
>>> /Joel
>> Hi Joel,
>>
>> Good to see this patch. It improves the efficiency of annotations 
>> caching on methods/constructors. What about fields? They too are 
>> AccessibleObject(s), contain annotations and are copied from root 
>> instances when handed over to the user...
>>
>> The difference of behaviour in the presence of class redefinition 
>> could be observed though, but I think this is not a problem. For 
>> example:
>>
>> Class c = ...;
>>
>> Method m1 = c.getDeclaredMethod("m");
>> Method m2 = c.getDeclaredMethod("m");
>>
>> assert m1 != m2; // but they share the same root;
>>
>> Annotation[] anns1 = m1.getDeclaredAnnotations();
>>
>> // now class 'c' is redefined and annotations changes on method m()...
>>
>> Annotation[] anns2 = m2.getDeclaredAnnotations();
>>
>> // previously anns1 / anns2 would countain annotations pre / post 
>> class redefinition, but with this patch, they would both contain the 
>> pre class redefinition annotations.
>>
>>
>> But as I see it this is not a big problem to hold the patch back.
>>
>>
>> Regards, Peter
>>
>