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

[David Holmes]

Hi Mandy,

On 2/04/2020 3:17 pm, Mandy Chung wrote:
> Hi David,
> 
> Thanks for the edits to the comments.   "weak hidden" were missed to be 
> changed to "non-strong hidden".  Here is the patch fixing the comments.

There are 33 cases of "weak hidden" in the patch I reviewed and also 
some "hidden weak". Should they all be "non-strong hidden" now? In some 
cases it also appears in variables and associated logic ie.:

src/hotspot/share/classfile/classLoaderHierarchyDCmd.cpp

+      _hidden_weak_classes(NULL), _num_hidden_weak_classes(0),

I'm not clear how far this terminology change needs to extend ??

Otherwise patch below seems fine.

Thanks,
David
-----


> diff --git a/src/hotspot/share/ci/ciField.cpp 
> b/src/hotspot/share/ci/ciField.cpp
> --- a/src/hotspot/share/ci/ciField.cpp
> +++ b/src/hotspot/share/ci/ciField.cpp
> @@ -223,8 +223,8 @@
>         holder->is_in_package("jdk/internal/foreign") || 
> holder->is_in_package("jdk/incubator/foreign") ||
>         holder->is_in_package("java/lang"))
>       return true;
> -  // Trust VM hidden and unsafe anonymous classes. They are created via 
> Lookup.defineClass or
> -  // the private API (jdk.internal.misc.Unsafe) and can't be 
> serialized, so there is no hacking
> +  // Trust hidden and VM unsafe anonymous classes. They are created via 
> Lookup.defineClass or
> +  // the private jdk.internal.misc.Unsafe API and can't be serialized, 
> so there is no hacking
>     // of finals going on with them.
>     if (holder->is_hidden() || holder->is_unsafe_anonymous())
>       return true;
> diff --git a/src/hotspot/share/ci/ciInstanceKlass.cpp 
> b/src/hotspot/share/ci/ciInstanceKlass.cpp
> --- a/src/hotspot/share/ci/ciInstanceKlass.cpp
> +++ b/src/hotspot/share/ci/ciInstanceKlass.cpp
> @@ -76,7 +76,7 @@
>     oop holder = ik->klass_holder();
>     if (ik->class_loader_data()->has_class_mirror_holder()) {
>       // Though ciInstanceKlass records class loader oop, it's not 
> enough to keep
> -    // VM weak hidden and unsafe anonymous classes alive (loader == 
> NULL). Klass holder should
> +    // non-strong hidden class and VM unsafe anonymous classes alive 
> (loader == NULL). Klass holder should
>       // be used instead. It is enough to record a ciObject, since 
> cached elements are never removed
>       // during ciObjectFactory lifetime. ciObjectFactory itself is 
> created for
>       // every compilation and lives for the whole duration of the 
> compilation.
> diff --git a/src/hotspot/share/classfile/classLoaderData.hpp 
> b/src/hotspot/share/classfile/classLoaderData.hpp
> --- a/src/hotspot/share/classfile/classLoaderData.hpp
> +++ b/src/hotspot/share/classfile/classLoaderData.hpp
> @@ -127,9 +127,10 @@
>     bool _accumulated_modified_oops; // Mod Union Equivalent (CMS support)
> 
>     int _keep_alive;         // if this CLD is kept alive.
> -                           // Used for weak hidden classes, unsafe 
> anonymous classes and the
> +                           // Used for non-strong hidden classes, 
> unsafe anonymous classes and the
>                              // boot class loader. _keep_alive does not 
> need to be volatile or
> -                           // atomic since there is one unique CLD per 
> weak hidden or unsafe anonymous class.
> +                           // atomic since there is one unique CLD per 
> non-strong hidden class
> +                           // or unsafe anonymous class.
> 
>     volatile int _claim; // non-zero if claimed, for example during GC 
> traces.
>                          // To avoid applying oop closure more than once.
> @@ -242,15 +243,15 @@
>     }
> 
>     // Returns true if this class loader data is for the system class 
> loader.
> -  // (Note that the class loader data may be for an weak hidden or 
> unsafe anonymous class)
> +  // (Note that the class loader data may be for a non-strong hidden 
> class or unsafe anonymous class)
>     bool is_system_class_loader_data() const;
> 
>     // Returns true if this class loader data is for the platform class 
> loader.
> -  // (Note that the class loader data may be for an weak hidden or 
> unsafe anonymous class)
> +  // (Note that the class loader data may be for a non-strong hidden 
> class or unsafe anonymous class)
>     bool is_platform_class_loader_data() const;
> 
>     // Returns true if this class loader data is for the boot class loader.
> -  // (Note that the class loader data may be for an weak hidden unsafe 
> anonymous class)
> +  // (Note that the class loader data may be for a non-strong hidden 
> class or unsafe anonymous class)
>     inline bool is_boot_class_loader_data() const;
> 
>     bool is_builtin_class_loader_data() const;
> @@ -271,7 +272,7 @@
>       return _unloading;
>     }
> 
> -  // Used to refcount an weak hidden or unsafe anonymous class's CLD in 
> order to
> +  // Used to refcount a non-strong hidden class's or unsafe anonymous 
> class's CLD in order to
>     // indicate their aliveness.
>     void inc_keep_alive();
>     void dec_keep_alive();
> 
> diff --git a/src/hotspot/share/interpreter/linkResolver.cpp 
> b/src/hotspot/share/interpreter/linkResolver.cpp
> --- a/src/hotspot/share/interpreter/linkResolver.cpp
> +++ b/src/hotspot/share/interpreter/linkResolver.cpp
> @@ -611,7 +611,7 @@
>                (same_module) ? "" : sel_klass->class_in_module_of_loader()
>                );
> 
> -    // For private access check if there was a problem with nest host
> +    // For private access see if there was a problem with nest host
>       // resolution, and if so report that as part of the message.
>       if (sel_method->is_private()) {
>         print_nest_host_error_on(&ss, ref_klass, sel_klass, THREAD);
> @@ -951,7 +951,7 @@
>                (same_module) ? "" : "; ",
>                (same_module) ? "" : sel_klass->class_in_module_of_loader()
>                );
> -    // For private access check if there was a problem with nest host
> +    // For private access see if there was a problem with nest host
>       // resolution, and if so report that as part of the message.
>       if (fd.is_private()) {
>         print_nest_host_error_on(&ss, ref_klass, sel_klass, THREAD);
> 
> Mandy
> 
> On 4/1/20 9:52 PM, David Holmes wrote:
>> 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
>>
>