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
