RFR: JDK-8054987: (reflect) Add sharing of annotations between instances of Executable
Hi all, Cleaning out the patch queue, I found this small patch that adds sharing of conceptually immutable annotation maps between instances of Executable representing the same executable. In short, Method/Constructor contain one bit of mutable state, if they have been set accessible or not. Core Reflection keeps a 'root' instance of an executable and copies it whenever you call get(Declared){Method|Constructor}. When using an Executable to look at annotations each copy you query for annotations, for exampel m1.getAnnotations(), gets its own LinkedHashMap containing the annotations. The map is then flattned and handed out to the client as a newly allocated array. This patch queries the root method for its map thus sharing annotation instances. It still hands out a unique array for each invocation but the Map and the instances are shared. Webrev: http://cr.openjdk.java.net/~jfranck/8054987/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054987 cheers /Joel
Hi Joel, Does your changeset alter the support (or non-support) of redefining an annotation? -Joe On 08/13/2014 05:23 AM, Joel Borggren-Franck wrote:
Hi all,
Cleaning out the patch queue, I found this small patch that adds sharing of conceptually immutable annotation maps between instances of Executable representing the same executable.
In short, Method/Constructor contain one bit of mutable state, if they have been set accessible or not. Core Reflection keeps a 'root' instance of an executable and copies it whenever you call get(Declared){Method|Constructor}. When using an Executable to look at annotations each copy you query for annotations, for exampel m1.getAnnotations(), gets its own LinkedHashMap containing the annotations. The map is then flattned and handed out to the client as a newly allocated array. This patch queries the root method for its map thus sharing annotation instances. It still hands out a unique array for each invocation but the Map and the instances are shared.
Webrev: http://cr.openjdk.java.net/~jfranck/8054987/webrev.00/ Bug: https://bugs.openjdk.java.net/browse/JDK-8054987
cheers /Joel
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
On 08/13/2014 11:47 PM, 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, Okay; I approve your patch then. Cheers, -Joe
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
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
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.... 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
Hi Peter, Joe, On 2014-08-14, 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:
Thanks for taking a look at this. You are right about Fields, I added caching for Fields too: http://cr.openjdk.java.net/~jfranck/8054987/webrev.01/ I don't want to rewrite the synchronization in this patch, I'll file a followup RFE for the synchronization and a REF/bug for the publication. cheers /Joel
Looks good Joel; sorry for the delayed review. Thanks, -Joe On 8/18/2014 1:23 AM, Joel Borggren-Franck wrote:
Hi Peter, Joe,
On 2014-08-14, 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: Thanks for taking a look at this. You are right about Fields, I added caching for Fields too:
http://cr.openjdk.java.net/~jfranck/8054987/webrev.01/
I don't want to rewrite the synchronization in this patch, I'll file a followup RFE for the synchronization and a REF/bug for the publication.
cheers /Joel
participants (3)
-
Joe Darcy
-
Joel Borggren-Franck
-
Peter Levart