bottleneck by java.lang.Class.getAnnotations() - proposed patch

[Alexander Knöller]

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.
>> */
>>