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