bottleneck by java.lang.Class.getAnnotations() - a better patch

[David Holmes]

Hi Peter,

The movement of the reflection caches to a helper object is exactly what 
I had previously proposed here (some differences in the details of course):

http://cr.openjdk.java.net/~dholmes/JEP-149/webrev/

and discussed here:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2012-April/009749.html

but this did not touch the annotations fields.

David

On 6/11/2012 11:12 PM, Peter Levart wrote:
> On 11/05/2012 06:23 AM, David Holmes wrote:
>> 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 11/05/2012 10:25 AM, Alan Bateman wrote:
>> Here's a good starting place on the interaction of runtime visible
>> attributes and RedefineClasses/RetransformClasses:
>>
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=5002251
>>
>> -Alan.
>
> Hi all,
>
> Presented here is a patch mainly against java.lang.Class and also
> against java.lang.reflect.[Field,Method,Constructor,Executable] classes.
>
> Currently java.lang.Class uses the following fields to maintain caches
> of reflection data that are invalidated as a result of class or
> superclass redefinition/re-transformation:
>
> private volatile transient SoftReference<Field[]> declaredFields;
> private volatile transient SoftReference<Field[]> publicFields;
> private volatile transient SoftReference<Method[]> declaredMethods;
> private volatile transient SoftReference<Method[]> publicMethods;
> private volatile transient SoftReference<Constructor<T>[]>
> declaredConstructors;
> private volatile transient SoftReference<Constructor<T>[]>
> publicConstructors;
> private volatile transient SoftReference<Field[]> declaredPublicFields;
> private volatile transient SoftReference<Method[]> declaredPublicMethods;
>
> // Value of classRedefinedCount when we last cleared the cached values
> // that are sensitive to class redefinition.
> private volatile transient int lastRedefinedCount = 0;
>
> // Annotations cache
> private transient Map<Class<? extends Annotation>, Annotation> annotations;
> private transient Map<Class<? extends Annotation>, Annotation>
> declaredAnnotations;
>
> If I understand Alan's references correctly, current VM can redefine the
> class in a way that changes method bodies. Also new methods can be
> added. And the set of annotations can also be altered. And future
> improvements could allow even more.
>
> Because annotations are cached on Field/Method/Constructor instances,
> all the above fields must be invalidated when the class or superclass is
> redefined.
>
> It can also be observed that Field/Method/Constructor caches are
> maintained using SoftReferences but annotations are hard references. I
> don't know if this is intentional. I believe that annotations could also
> be SoftReferenced, so that in the event of memory pressure they get
> cleared. Many applications retrieve annotations only in the early stages
> of their life-cycle and then either cache them themselves or forget
> about them.
>
> So I designed the patch to equalize this. If this is undesirable, the
> patch could be modified to make a distinction again.
>
> The patch replaces the above-mentioned java.lang.Class fields with a
> single field:
>
> private volatile transient SoftReference<VolatileData<T>> volatileData;
>
> ...which is a SoftReference to the following structure:
>
> // volatile data that might get invalid when JVM TI RedefineClasses() is
> called
> static class VolatileData<T> {
> volatile Field[] declaredFields;
> volatile Field[] publicFields;
> volatile Method[] declaredMethods;
> volatile Method[] publicMethods;
> volatile Constructor<T>[] declaredConstructors;
> volatile Constructor<T>[] publicConstructors;
> // Intermediate results for getFields and getMethods
> volatile Field[] declaredPublicFields;
> volatile Method[] declaredPublicMethods;
> // Annotations
> volatile Map<Class<? extends Annotation>, Annotation> annotations;
> volatile Map<Class<? extends Annotation>, Annotation> declaredAnnotations;
> // Value of classRedefinedCount when we created this VolatileData instance
> final int redefinedCount;
>
> So let's look at static overhead differences using 64 bit addressing
> (useful load - arrays, Maps not counted since the patched code uses the
> same amount of same types of each).
>
> * Fresh java.lang.Class instance:
>
> current JDK8 code:
>
> 10 OOPs + 1 int = 10*8+4 = 84 bytes in 1 instance
>
> vs. patched code :
>
> 1 OOP = 8 bytes in 1 instance
>
> * Fully loaded java.lang.Class (Fields, Methods, Constructors, annotations):
>
> current JDK8 code:
>
> 10 OOPs + 1 int = 84 bytes
> 8 SoftReference instances = 8*(header + 4 OOPs + 1 long) = 8*(24+32+8) =
> 8*64 = 512 bytes
> total: 84+512 = 596 bytes in 9 instances
>
> vs. patched code :
>
> 1 OOP = 8 bytes
> 1 SoftReference = 64 bytes
> 1 VolatileData = header + 10 OOPs + 1 int = 24+84 = 108 bytes
> total: 8+64+108 = 180 bytes in 3 instances
>
> So we have 84 vs. 8 (empty) or 596 vs. 180 (fully loaded) byte overheads and
> 1 vs. 1 (empty) or 9 vs. 3 (fully loaded) instance overheads
>
> Other than that, the patch also removes synchronized blocks for lazy
> initialization of annotations in Class, Field, Method and Constructor
> and replaces them with volatile fields. In case of Class.volatileData,
> this field is initialized using a CAS so there is no race which could
> install an already stale instance over more recent. Although such race
> would quickly be corrected at next call to any retrieval method, because
> redefinedCount is now an integral part of the cached structure not an
> individual volatile field.
>
> There is also a change in how annotations are cached in Field, Method
> and Constructor. Originally they are cached in each copy of the
> Field/Method/Constructor that is returned to the outside world at each
> invocation of Class.getFields() etc. Such caching is not very effective
> if the annotations are only retrieved once per instance. The patch
> changes this and delegates caching to the "root" instance which is held
> inside Class so caching becomes more effective in certain usage
> patterns. There's now a possible hot-spot on the "root" instance but
> that seems not to be a bottleneck since the fast-path does not involve
> blocking synchronization (just volatile read). The effects of this
> change are clearly visible in one of the benchmarks.
>
> I have tried to create 3 micro benchmarks which exercise concurrent load
> on 3 Class instances.
>
> Here's the benchmark code:
>
> https://raw.github.com/plevart/jdk8-hacks/master/src/test/ReflectionTest.java
>
> And here are the results when run on an Intel i7 CPU (4 cores, 2
> threads/core) Linux machine using -Xmx4G VM option:
>
> https://raw.github.com/plevart/jdk8-hacks/master/benchmark_results.txt
>
>
> The huge difference of Test1 results is a direct consequence of patched
> code delegating caching of annotations in Field/Method/Constructor to
> the "root" instance.
>
> Test2 results show no noticeable difference between original and patched
> code. This, I believe, is the most common usage of the API, so another
> level of indirection does not appear to present any noticeable
> performance overhead.
>
> The Test3 on the other hand shows the synchronization overhead of
> current jdk8 code in comparison with non-blocking synchronization in
> patched code.
>
> JEP 149 also mentions testing with SPECjbb2005 and SPECjvm98, but that
> exceeds my possibilities.
>
> The patch against jdk8/jdk8/jdk hg repository is here:
>
> https://raw.github.com/plevart/jdk8-hacks/master/volatile_class_data_caching.patch
>
> You can also browse the changed sources:
>
> https://github.com/plevart/jdk8-hacks
>
> For completeness I also paste the patch below.
>
> Regards, Peter
>
>
> 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 Tue Nov 06 14:11:43 2012 +0100
> @@ -2212,39 +2212,72 @@
>
> // Caches for certain reflective results
> private static boolean useCaches = true;
> - private volatile transient SoftReference<Field[]> declaredFields;
> - private volatile transient SoftReference<Field[]> publicFields;
> - private volatile transient SoftReference<Method[]> declaredMethods;
> - private volatile transient SoftReference<Method[]> publicMethods;
> - private volatile transient SoftReference<Constructor<T>[]>
> declaredConstructors;
> - private volatile transient SoftReference<Constructor<T>[]>
> publicConstructors;
> - // Intermediate results for getFields and getMethods
> - private volatile transient SoftReference<Field[]> declaredPublicFields;
> - private volatile transient SoftReference<Method[]> declaredPublicMethods;
> +
> + // volatile data that might get invalid when JVM TI RedefineClasses()
> is called
> + static class VolatileData<T> {
> + volatile Field[] declaredFields;
> + volatile Field[] publicFields;
> + volatile Method[] declaredMethods;
> + volatile Method[] publicMethods;
> + volatile Constructor<T>[] declaredConstructors;
> + volatile Constructor<T>[] publicConstructors;
> + // Intermediate results for getFields and getMethods
> + volatile Field[] declaredPublicFields;
> + volatile Method[] declaredPublicMethods;
> + // Annotations
> + volatile Map<Class<? extends Annotation>, Annotation> annotations;
> + volatile Map<Class<? extends Annotation>, Annotation> declaredAnnotations;
> + // Value of classRedefinedCount when we created this VolatileData instance
> + final int redefinedCount;
> +
> + VolatileData(int redefinedCount) {
> + this.redefinedCount = redefinedCount;
> + }
> +
> + // initialize Unsafe machinery here, since we need to call Class.class
> instance method and would like to avoid
> + // calling it in the static initializer of the Class class...
> + private static final Unsafe unsafe;
> + // offset of Class.volatileData instance field
> + private static final long volatileDataOffset;
> +
> + static {
> + unsafe = Unsafe.getUnsafe();
> + // bypass caches
> + Field volatileDataField =
> searchFields(Class.class.getDeclaredFields0(false), "volatileData");
> + if (volatileDataField == null) throw new Error("No volatileData field
> found in java.lang.Class");
> + volatileDataOffset = unsafe.objectFieldOffset(volatileDataField);
> + }
> +
> + static <T> boolean compareAndSwap(Class<?> clazz,
> SoftReference<VolatileData<T>> oldData, SoftReference<VolatileData<T>>
> newData) {
> + return unsafe.compareAndSwapObject(clazz, volatileDataOffset, oldData,
> newData);
> + }
> + }
> +
> + private volatile transient SoftReference<VolatileData<T>> volatileData;
>
> // Incremented by the VM on each call to JVM TI RedefineClasses()
> // that redefines this class or a superclass.
> private volatile transient int classRedefinedCount = 0;
>
> - // Value of classRedefinedCount when we last cleared the cached values
> - // that are sensitive to class redefinition.
> - private volatile transient int lastRedefinedCount = 0;
> + // Lazily create and cache VolatileData
> + private VolatileData<T> volatileData() {
> + if (!useCaches) return null;
>
> - // Clears cached values that might possibly have been obsoleted by
> - // a class redefinition.
> - private void clearCachesOnClassRedefinition() {
> - if (lastRedefinedCount != classRedefinedCount) {
> - 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
> - // 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.
> - lastRedefinedCount = classRedefinedCount;
> + while (true)
> + {
> + SoftReference<VolatileData<T>> volatileData = this.volatileData;
> + int classRedefinedCount = this.classRedefinedCount;
> + VolatileData<T> vd;
> + if (volatileData != null && (vd = volatileData.get()) != null &&
> vd.redefinedCount == classRedefinedCount) {
> + return vd;
> + }
> + // no SoftReference or cleared SoftReference or stale VolatileData
> + vd = new VolatileData<T>(classRedefinedCount);
> + // try to CAS it...
> + if (VolatileData.compareAndSwap(this, volatileData, new
> SoftReference<VolatileData<T>>(vd))) {
> + return vd;
> + }
> + // else retry
> }
> }
>
> @@ -2288,26 +2321,18 @@
> private Field[] privateGetDeclaredFields(boolean publicOnly) {
> checkInitted();
> Field[] res = null;
> - if (useCaches) {
> - clearCachesOnClassRedefinition();
> - if (publicOnly) {
> - if (declaredPublicFields != null) {
> - res = declaredPublicFields.get();
> - }
> - } else {
> - if (declaredFields != null) {
> - res = declaredFields.get();
> - }
> - }
> + VolatileData<T> vd = volatileData();
> + if (vd != null) {
> + res = publicOnly ? vd.declaredPublicFields : vd.declaredFields;
> if (res != null) return res;
> }
> // No cached value available; request value from VM
> res = Reflection.filterFields(this, getDeclaredFields0(publicOnly));
> - if (useCaches) {
> + if (vd != null) {
> if (publicOnly) {
> - declaredPublicFields = new SoftReference<>(res);
> + vd.declaredPublicFields = res;
> } else {
> - declaredFields = new SoftReference<>(res);
> + vd.declaredFields = res;
> }
> }
> return res;
> @@ -2319,11 +2344,9 @@
> private Field[] privateGetPublicFields(Set<Class<?>> traversedInterfaces) {
> checkInitted();
> Field[] res = null;
> - if (useCaches) {
> - clearCachesOnClassRedefinition();
> - if (publicFields != null) {
> - res = publicFields.get();
> - }
> + VolatileData<T> vd = volatileData();
> + if (vd != null) {
> + res = vd.publicFields;
> if (res != null) return res;
> }
>
> @@ -2356,8 +2379,8 @@
>
> res = new Field[fields.size()];
> fields.toArray(res);
> - if (useCaches) {
> - publicFields = new SoftReference<>(res);
> + if (vd != null) {
> + vd.publicFields = res;
> }
> return res;
> }
> @@ -2381,17 +2404,9 @@
> private Constructor<T>[] privateGetDeclaredConstructors(boolean
> publicOnly) {
> checkInitted();
> Constructor<T>[] res = null;
> - if (useCaches) {
> - clearCachesOnClassRedefinition();
> - if (publicOnly) {
> - if (publicConstructors != null) {
> - res = publicConstructors.get();
> - }
> - } else {
> - if (declaredConstructors != null) {
> - res = declaredConstructors.get();
> - }
> - }
> + VolatileData<T> vd = volatileData();
> + if (vd != null) {
> + res = publicOnly ? vd.publicConstructors : vd.declaredConstructors;
> if (res != null) return res;
> }
> // No cached value available; request value from VM
> @@ -2402,11 +2417,11 @@
> } else {
> res = getDeclaredConstructors0(publicOnly);
> }
> - if (useCaches) {
> + if (vd != null) {
> if (publicOnly) {
> - publicConstructors = new SoftReference<>(res);
> + vd.publicConstructors = res;
> } else {
> - declaredConstructors = new SoftReference<>(res);
> + vd.declaredConstructors = res;
> }
> }
> return res;
> @@ -2424,26 +2439,18 @@
> private Method[] privateGetDeclaredMethods(boolean publicOnly) {
> checkInitted();
> Method[] res = null;
> - if (useCaches) {
> - clearCachesOnClassRedefinition();
> - if (publicOnly) {
> - if (declaredPublicMethods != null) {
> - res = declaredPublicMethods.get();
> - }
> - } else {
> - if (declaredMethods != null) {
> - res = declaredMethods.get();
> - }
> - }
> + VolatileData<T> vd = volatileData();
> + if (vd != null) {
> + res = publicOnly ? vd.declaredPublicMethods : vd.declaredMethods;
> if (res != null) return res;
> }
> // No cached value available; request value from VM
> res = Reflection.filterMethods(this, getDeclaredMethods0(publicOnly));
> if (useCaches) {
> if (publicOnly) {
> - declaredPublicMethods = new SoftReference<>(res);
> + vd.declaredPublicMethods = res;
> } else {
> - declaredMethods = new SoftReference<>(res);
> + vd.declaredMethods = res;
> }
> }
> return res;
> @@ -2546,11 +2553,9 @@
> private Method[] privateGetPublicMethods() {
> checkInitted();
> Method[] res = null;
> - if (useCaches) {
> - clearCachesOnClassRedefinition();
> - if (publicMethods != null) {
> - res = publicMethods.get();
> - }
> + VolatileData<T> vd = volatileData();
> + if (vd != null) {
> + res = vd.publicMethods;
> if (res != null) return res;
> }
>
> @@ -2558,7 +2563,7 @@
> // Start by fetching public declared methods
> MethodArray methods = new MethodArray();
> {
> - Method[] tmp = privateGetDeclaredMethods(true);
> + Method[] tmp = privateGetDeclaredMethods(true);
> methods.addAll(tmp);
> }
> // Now recur over superclass and direct superinterfaces.
> @@ -2598,8 +2603,8 @@
> methods.addAllIfNotPresent(inheritedMethods);
> methods.compactAndTrim();
> res = methods.getArray();
> - if (useCaches) {
> - publicMethods = new SoftReference<>(res);
> + if (vd != null) {
> + vd.publicMethods = res;
> }
> return res;
> }
> @@ -2609,7 +2614,7 @@
> // Helpers for fetchers of one field, method, or constructor
> //
>
> - private Field searchFields(Field[] fields, String name) {
> + private static Field searchFields(Field[] fields, String name) {
> String internedName = name.intern();
> for (int i = 0; i < fields.length; i++) {
> if (fields[i].getName() == internedName) {
> @@ -3049,8 +3054,7 @@
> if (annotationClass == null)
> throw new NullPointerException();
>
> - initAnnotationsIfNecessary();
> - return (A) annotations.get(annotationClass);
> + return (A) privateGetAnnotations(false).get(annotationClass);
> }
>
> /**
> @@ -3070,41 +3074,45 @@
> * @since 1.5
> */
> public Annotation[] getAnnotations() {
> - initAnnotationsIfNecessary();
> - return AnnotationParser.toArray(annotations);
> + return AnnotationParser.toArray(privateGetAnnotations(false));
> }
>
> /**
> * @since 1.5
> */
> public Annotation[] getDeclaredAnnotations() {
> - initAnnotationsIfNecessary();
> - return AnnotationParser.toArray(declaredAnnotations);
> + return AnnotationParser.toArray(privateGetAnnotations(true));
> }
>
> - // Annotations cache
> - private transient Map<Class<? extends Annotation>, Annotation>
> annotations;
> - private transient Map<Class<? extends Annotation>, Annotation>
> declaredAnnotations;
>
> - private synchronized void initAnnotationsIfNecessary() {
> - clearCachesOnClassRedefinition();
> - if (annotations != null)
> - return;
> - declaredAnnotations = AnnotationParser.parseAnnotations(
> + private Map<Class<? extends Annotation>, Annotation>
> privateGetAnnotations(boolean declaredOnly) {
> + Map<Class<? extends Annotation>, Annotation> res;
> + VolatileData<T> vd = volatileData();
> + if (vd != null) {
> + res = declaredOnly ? vd.declaredAnnotations : vd.annotations;
> + if (res != null) return res;
> + }
> +
> + Map<Class<? extends Annotation>, Annotation> declaredAnnotations =
> AnnotationParser.parseAnnotations(
> getRawAnnotations(), getConstantPool(), this);
> + Map<Class<? extends Annotation>, Annotation> annotations;
> 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()) {
> + for (Map.Entry<Class<? extends Annotation>, Annotation> e :
> superClass.privateGetAnnotations(false).entrySet()) {
> Class<? extends Annotation> annotationClass = e.getKey();
> if (AnnotationType.getInstance(annotationClass).isInherited())
> annotations.put(annotationClass, e.getValue());
> }
> annotations.putAll(declaredAnnotations);
> }
> +
> + vd.annotations = annotations;
> + vd.declaredAnnotations = declaredAnnotations;
> +
> + return declaredOnly ? declaredAnnotations : annotations;
> }
>
> // Annotation types cache their internal (AnnotationType) form
> diff -r 7ac292e57b5a src/share/classes/java/lang/reflect/Constructor.java
> --- a/src/share/classes/java/lang/reflect/Constructor.java Thu Nov 01
> 14:12:21 2012 -0700
> +++ b/src/share/classes/java/lang/reflect/Constructor.java Tue Nov 06
> 14:11:43 2012 +0100
> @@ -482,7 +482,10 @@
> * @since 1.5
> */
> public Annotation[] getDeclaredAnnotations() {
> - return super.getDeclaredAnnotations();
> + if (root != null)
> + return root.getDeclaredAnnotations();
> + else
> + return super.getDeclaredAnnotations();
> }
>
> /**
> diff -r 7ac292e57b5a src/share/classes/java/lang/reflect/Executable.java
> --- a/src/share/classes/java/lang/reflect/Executable.java Thu Nov 01
> 14:12:21 2012 -0700
> +++ b/src/share/classes/java/lang/reflect/Executable.java Tue Nov 06
> 14:11:43 2012 +0100
> @@ -378,11 +378,12 @@
> return AnnotationParser.toArray(declaredAnnotations());
> }
>
> - private transient Map<Class<? extends Annotation>, Annotation>
> declaredAnnotations;
> + private volatile transient Map<Class<? extends Annotation>,
> Annotation> declaredAnnotations;
>
> - private synchronized Map<Class<? extends Annotation>, Annotation>
> declaredAnnotations() {
> + private Map<Class<? extends Annotation>, Annotation>
> declaredAnnotations() {
> + Map<Class<? extends Annotation>, Annotation> declaredAnnotations =
> this.declaredAnnotations;
> if (declaredAnnotations == null) {
> - declaredAnnotations = AnnotationParser.parseAnnotations(
> + this.declaredAnnotations = declaredAnnotations =
> AnnotationParser.parseAnnotations(
> getAnnotationBytes(),
> sun.misc.SharedSecrets.getJavaLangAccess().
> getConstantPool(getDeclaringClass()),
> diff -r 7ac292e57b5a src/share/classes/java/lang/reflect/Field.java
> --- a/src/share/classes/java/lang/reflect/Field.java Thu Nov 01 14:12:21
> 2012 -0700
> +++ b/src/share/classes/java/lang/reflect/Field.java Tue Nov 06 14:11:43
> 2012 +0100
> @@ -1027,11 +1027,15 @@
> return AnnotationParser.toArray(declaredAnnotations());
> }
>
> - private transient Map<Class<? extends Annotation>, Annotation>
> declaredAnnotations;
> + private volatile transient Map<Class<? extends Annotation>,
> Annotation> declaredAnnotations;
>
> - private synchronized Map<Class<? extends Annotation>, Annotation>
> declaredAnnotations() {
> + private Map<Class<? extends Annotation>, Annotation>
> declaredAnnotations() {
> + if (root != null)
> + return root.declaredAnnotations();
> +
> + Map<Class<? extends Annotation>, Annotation> declaredAnnotations =
> this.declaredAnnotations;
> if (declaredAnnotations == null) {
> - declaredAnnotations = AnnotationParser.parseAnnotations(
> + this.declaredAnnotations = declaredAnnotations =
> AnnotationParser.parseAnnotations(
> annotations, sun.misc.SharedSecrets.getJavaLangAccess().
> getConstantPool(getDeclaringClass()),
> getDeclaringClass());
> diff -r 7ac292e57b5a src/share/classes/java/lang/reflect/Method.java
> --- a/src/share/classes/java/lang/reflect/Method.java Thu Nov 01
> 14:12:21 2012 -0700
> +++ b/src/share/classes/java/lang/reflect/Method.java Tue Nov 06
> 14:11:43 2012 +0100
> @@ -583,7 +583,10 @@
> * @since 1.5
> */
> public Annotation[] getDeclaredAnnotations() {
> - return super.getDeclaredAnnotations();
> + if (root != null)
> + return root.getDeclaredAnnotations();
> + else
> + return super.getDeclaredAnnotations();
> }
>
> /**
>
>