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

[Mandy Chung]

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.

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
>