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

[Peter Levart]

Hi all,

I have prepared a better patch. It addresses the goals of JEP-149 more
seriously. I also have some benchmarks. Stay tuned...

Regards, Peter
On Nov 6, 2012 2:29 AM, "David Holmes" <david.holmes at oracle.com> wrote:

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