On 04.11.2014 20:53, Martin Buchholz wrote:
> On Tue, Nov 4, 2014 at 2:13 AM, Paul Sandoz <paul.san...@oracle.com> wrote:
>> On Nov 3, 2014, at 11:18 PM, Martin Buchholz <marti...@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.
