Review Request: 8238358: Implementation of JEP 371: Hidden Classes
David Holmes
david.holmes at oracle.com
Fri Apr 3 01:36:03 UTC 2020
Hi Mandy,
Update seems fine.
Thanks,
David
On 3/04/2020 4:56 am, 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/
>
> 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 serviceability-dev
mailing list