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

Lois Foltan lois.foltan at oracle.com
Thu Apr 2 20:28:07 UTC 2020


On 4/2/2020 2:56 PM, Mandy Chung wrote:
> 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/ 
>

Patch looks good Mandy.
Lois

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



More information about the valhalla-dev mailing list