Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[David Holmes]

Hi Mandy,

On 1/04/2020 1:01 pm, Mandy Chung wrote:
> Thanks to the review feedbacks.
> 
> Updated webrev:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04/

I've had a good look through all the hotspot related changes. All looks 
good.

A few minor comments:

src/hotspot/share/ci/ciField.cpp

    // Trust VM hidden and unsafe anonymous classes.

should say

    // Trust hidden and VM unsafe anonymous classes.


   // the private API (jdk.internal.misc.Unsafe)

s/the/a/  or else change the () to commas or perhaps even better:

   // the private jdk.internal.misc.Unsafe API,

---

src/hotspot/share/ci/ciInstanceKlass.cpp

   // VM weak hidden and unsafe anonymous classes

should say

   // weak hidden and VM unsafe anonymous classes

---

src/hotspot/share/classfile/classLoaderData.hpp

!  // the CLDs lifecycle.  For example, a non-strong hidden class or an
...
!  // Used for weak hidden classes, unsafe anonymous classes and the

There is an inconsistency in terminology, so far most code comments 
refer to "weak hidden" but now you are using "non-strong hidden".

! for an weak hidden

s/an/a/   multiple locations

---

src/hotspot/share/interpreter/linkResolver.cpp

Can you fix one of my comments please (in two places) :)

+     // For private access check if there was a problem with nest host

would read better as:

+     // For private access see if there was a problem with nest host

---

Thanks,
David
------

> Delta between webrev.03 and webrev.04:
> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03-delta/
> 
> Summary of changes is:
> 1. Update javac to retain the previous behavior when compiling to target 
> release <= 14 where lambda proxy class is not a nestmate
> 2. Rename Class::isHiddenClass to Class::isHidden
> 3. Address Joe's feedback on the CSR to document the behavior of the 
> relevant methods in java.lang.Class for hidden classes
> 4. Add test case for unloadable class with nest host error
> 5. Add test cases for hidden classes with different kinds of class or 
> interface
> 6. Update dcmd to drop "weak hidden class" and refer it as "hidden class"
> 7. Small changes in response to Remi, Coleen, Paul's review comments
> 8. Fix copyright headers
> 9. Fix @modules in tests
> 
> Most of the changes above have also been reviewed as separate patches.
> 
> Thanks
> Mandy
> 
> On 3/26/20 4:57 PM, Mandy Chung wrote:
>> Please review the implementation of JEP 371: Hidden Classes. The main 
>> changes are in core-libs and hotspot runtime area.  Small changes are 
>> made in javac, VM compiler (intrinsification of Class::isHiddenClass), 
>> JFR, JDI, and jcmd.  CSR [1]has been reviewed and is in the finalized 
>> state (see specdiff and javadoc below for reference).
>>
>> Webrev:
>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03 
>>
>>
>> javadoc/specdiff
>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/ 
>>
>>
>> JVMS 5.4.4 change:
>> http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf 
>>
>>
>> CSR:
>> https://bugs.openjdk.java.net/browse/JDK-8238359
>>
>> Thanks
>> Mandy
>> [1] https://bugs.openjdk.java.net/browse/JDK-8238359
>> [2] https://bugs.openjdk.java.net/browse/JDK-8240338
>> [3] https://bugs.openjdk.java.net/browse/JDK-8230502
>