RFR: 8062771: Core reflection should use final fields whenever possible

[Martin Buchholz]

[+core-libs-dev, Alexey]


On Tue, Nov 4, 2014 at 2:13 AM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
> Hi Martin,
>
> Should this and the other review also be posted on core-libs?

 (sorry for forgetting the mailing list)

> On Nov 3, 2014, at 11:18 PM, Martin Buchholz <martinrb at google.com> wrote:
>
>> Hi Joe, Paul,
>>
>> I'd like you to do a code review.
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/reflection-final-fields/
>> https://bugs.openjdk.java.net/browse/JDK-8062771
>
> Kind of scary.
>
> I suspect the fix JDK-8016236 also fixed this bug because the field GenericInfo.genericInfo was made volatile, which stamped in a barrier ensuring the publication was safe and the AbstractRepository.factory was not null. If my suspicions are correct it might be worth adding that analysis to the test (plus it might be easy to verify in your environment).

I was misled by the synopsis of
JDK-8016236  Class.getGenericInterfaces performance improvement
because it's a correctness fix as well as a performance improvement.

I recommend y'all do a little correctness backport to jdk7 Class.java
as follows:

@@ -2450,7 +2450,7 @@
     private native String getGenericSignature();

     // Generic info repository; lazily initialized
-    private transient ClassRepository genericInfo;
+    private volatile transient ClassRepository genericInfo;

     // accessor for factory
     private GenericsFactory getFactory() {
@@ -2460,11 +2460,13 @@

     // accessor for generic info repository
     private ClassRepository getGenericInfo() {
+        ClassRepository genericInfo = this.genericInfo;
         // lazily initialize repository if necessary
         if (genericInfo == null) {
             // create and cache generic info repository
             genericInfo = ClassRepository.make(getGenericSignature(),
                                                getFactory());
+            this.genericInfo = genericInfo;
         }
         return genericInfo; //return cached repository
     }

> Using final fields where at all possible is a good cleanup.

Right.  (even though this change fixes no known bug ... in openjdk9)

> Is it possible to make EmptyClass be a static inner class of the test? IIUC if its enclosed within ThreadSafety it should still work with URLClassLoader.

Done.  Webrev regenerated.