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

[Aleksey Shipilev]

On 04.11.2014 20:53, Martin Buchholz wrote:
> On Tue, Nov 4, 2014 at 2:13 AM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
>> On Nov 3, 2014, at 11:18 PM, Martin Buchholz <martinrb at google.com> wrote:
>>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/reflection-final-fields/
>>> https://bugs.openjdk.java.net/browse/JDK-8062771

Looks good. I always like more finals.

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

Ouch. I remember reviewing that change, but it never occurred to me the
original code was broken.


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

Yes. Although I am not sure how we push the changes to jdk7 these days.


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

If you do this:

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

...you don't need to do this for correctness (but may do for performance
-- avoidable volatile read for ARM/PPC):

>      // 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
>      }
> 

Also, I wonder if we need a singleton ClassRepository instance, or we
can tolerate two different ClassRepository-ies returned under concurrent
init.

Thanks,
-Aleksey.