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

Thu Apr 2 18:56:47 UTC 2020

Hi David,

Thank you for checking thoroughly.   I now grep on src/hotspot and clean 
all of them.

Updated delta webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.04-delta/

On 4/1/20 11:21 PM, David Holmes wrote:
> 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? 

yes, supposed to.   All should be fixed in webrev.04-delta.

> 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),
>

Are you reading webrev.03?    This has been fixed in webrev.04.

I found Klass::is_hidden_weak that should have been renamed too.

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

I hope consistently used in the source.

> Otherwise patch below seems fine.
>

Revised the patch.

Thanks
Mandy
> 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
>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20200402/a29c3528/attachment.htm>