[+core-libs-dev, Alexey]
On Tue, Nov 4, 2014 at 2:13 AM, Paul Sandoz <paul.san...@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 <marti...@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.
