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

[David Holmes]

Hi Alex,

On 5/11/2012 11:36 PM, Alexander Knöller wrote:
> 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() ?

Sorry I thought Peter's proposed patch was an implementation of your 
suggestion. Can you provide the code for your suggestion as it is a bit 
hard to evaluate exactly what you mean from a textual description.

> Are additional local Variables similar "bad habit" for memory usage (although they only affect the stack)?

Locals (potentially) affect dynamic footprint whereas fields affect 
static footprint - it was static footprint that was of concern here. 
Though of course we can't just trade static footprint for dynamic footprint.

David

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