Review Request: 8238358: Implementation of JEP 371: Hidden Classes
David Holmes
david.holmes at oracle.com
Thu Apr 2 06:21:10 UTC 2020
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
>>
>
More information about the serviceability-dev
mailing list