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


Reply via email to