Hi David. What about my proposal for a simple double-checked-locking for the redefinitions fields and the use of local variables for "annotations" in getAnnotations() and initAnnotationsIfNecessary() ? Are additional local Variables similar "bad habit" for memory usage (although they only affect the stack)?
Alex Am 05.11.2012 um 06:23 schrieb David Holmes: > Hi Peter, > > Moving the annotations fields into a helper object would tie in with the > Class-instance size reduction effort that was investigated as part of "JEP > 149: Reduce Core-Library Memory Usage": > > http://openjdk.java.net/jeps/149 > > The investigations there to date only looked at relocating the reflection > related fields, though the JEP mentions the annotations as well. > > Any such effort requires extensive benchmarking and performance analysis > before being accepted though. > > David > ----- > > On 5/11/2012 9:09 AM, Peter Levart wrote: >> Hi, >> >> I propose the following patch to java.lang.Class in order to overcome >> the synchronization bottleneck when accessing annotations from multiple >> threads. The patch packs the 'annotations' and 'declaredAnnotations' >> Maps together with an int redefinedCount into a structure: >> >> static class Annotations { >> final Map<Class<? extends Annotation>, Annotation> annotations; >> final Map<Class<? extends Annotation>, Annotation> declaredAnnotations; >> final int redefinedCount; >> >> which is referenced by a single volatile Class.annotations field. >> Instead of initAnnotationsIfNecessary() there's a >> getAnnotationsPrivate() method that uses simple double-checked locking >> to lazily initialize and return this structure. The slow-path part is >> still synchronized to ensure that >> Class.setAnnotationType/getAnnotationType call backs from AnnotationType >> are only done while holding a lock. >> >> Regards, Peter >> >> Here's the patch to jdk8 source: >> >> diff -r 7ac292e57b5a src/share/classes/java/lang/Class.java >> --- a/src/share/classes/java/lang/Class.java Thu Nov 01 14:12:21 2012 -0700 >> +++ b/src/share/classes/java/lang/Class.java Sun Nov 04 23:53:04 2012 +0100 >> @@ -2237,10 +2237,8 @@ >> declaredFields = publicFields = declaredPublicFields = null; >> declaredMethods = publicMethods = declaredPublicMethods = null; >> declaredConstructors = publicConstructors = null; >> - annotations = declaredAnnotations = null; >> >> - // Use of "volatile" (and synchronization by caller in the case >> - // of annotations) ensures that no thread sees the update to >> + // Use of "volatile" ensures that no thread sees the update to >> // lastRedefinedCount before seeing the caches cleared. >> // We do not guard against brief windows during which multiple >> // threads might redundantly work to fill an empty cache. >> @@ -3049,8 +3047,7 @@ >> if (annotationClass == null) >> throw new NullPointerException(); >> >> - initAnnotationsIfNecessary(); >> - return (A) annotations.get(annotationClass); >> + return (A) getAnnotationsPrivate().annotations.get(annotationClass); >> } >> >> /** >> @@ -3070,40 +3067,52 @@ >> * @since 1.5 >> */ >> public Annotation[] getAnnotations() { >> - initAnnotationsIfNecessary(); >> - return AnnotationParser.toArray(annotations); >> + return AnnotationParser.toArray(getAnnotationsPrivate().annotations); >> } >> >> /** >> * @since 1.5 >> */ >> public Annotation[] getDeclaredAnnotations() { >> - initAnnotationsIfNecessary(); >> - return AnnotationParser.toArray(declaredAnnotations); >> + return >> AnnotationParser.toArray(getAnnotationsPrivate().declaredAnnotations); >> } >> >> // Annotations cache >> - private transient Map<Class<? extends Annotation>, Annotation> >> annotations; >> - private transient Map<Class<? extends Annotation>, Annotation> >> declaredAnnotations; >> + private transient volatile Annotations annotations; >> >> - private synchronized void initAnnotationsIfNecessary() { >> - clearCachesOnClassRedefinition(); >> - if (annotations != null) >> - return; >> - declaredAnnotations = AnnotationParser.parseAnnotations( >> - getRawAnnotations(), getConstantPool(), this); >> - Class<?> superClass = getSuperclass(); >> - if (superClass == null) { >> - annotations = declaredAnnotations; >> - } else { >> - annotations = new HashMap<>(); >> - superClass.initAnnotationsIfNecessary(); >> - for (Map.Entry<Class<? extends Annotation>, Annotation> e : >> superClass.annotations.entrySet()) { >> - Class<? extends Annotation> annotationClass = e.getKey(); >> - if (AnnotationType.getInstance(annotationClass).isInherited()) >> - annotations.put(annotationClass, e.getValue()); >> + private Annotations getAnnotationsPrivate() { >> + // double checked locking >> + int redefinedCount = classRedefinedCount; >> + Annotations anns = annotations; >> + if (anns != null && anns.redefinedCount == redefinedCount) { >> + return anns; >> + } >> + >> + synchronized (this) { >> + redefinedCount = classRedefinedCount; >> + anns = annotations; >> + if (anns != null && anns.redefinedCount == redefinedCount) { >> + return anns; >> } >> - annotations.putAll(declaredAnnotations); >> + >> + Map<Class<? extends Annotation>, Annotation> declAnnMap = >> AnnotationParser.parseAnnotations( >> + getRawAnnotations(), getConstantPool(), this); >> + Map<Class<? extends Annotation>, Annotation> annMap; >> + Class<?> superClass = getSuperclass(); >> + if (superClass == null) { >> + annMap = declAnnMap; >> + } else { >> + annMap = new HashMap<>(); >> + for (Map.Entry<Class<? extends Annotation>, Annotation> e : >> superClass.getAnnotationsPrivate().annotations.entrySet()) { >> + Class<? extends Annotation> annotationClass = e.getKey(); >> + if (AnnotationType.getInstance(annotationClass).isInherited()) >> + annMap.put(annotationClass, e.getValue()); >> + } >> + annMap.putAll(declAnnMap); >> + } >> + >> + annotations = anns = new Annotations(annMap, declAnnMap, redefinedCount); >> + return anns; >> } >> } >> >> @@ -3119,6 +3128,18 @@ >> return annotationType; >> } >> >> + static final class Annotations { >> + final Map<Class<? extends Annotation>, Annotation> annotations; >> + final Map<Class<? extends Annotation>, Annotation> declaredAnnotations; >> + final int redefinedCount; >> + >> + Annotations(Map<Class<? extends Annotation>, Annotation> annotations, >> Map<Class<? extends Annotation>, Annotation> declaredAnnotations, int >> redefinedCount) { >> + this.annotations = annotations; >> + this.declaredAnnotations = declaredAnnotations; >> + this.redefinedCount = redefinedCount; >> + } >> + } >> + >> /* Backing store of user-defined values pertaining to this class. >> * Maintained by the ClassValue class. >> */ >>
