RFR (S): 8011803: release_C_heap_structures is never called for anonymous classes.

[Stefan Karlsson]

On 04/11/2013 10:52 PM, Coleen Phillimore wrote:
> Summary: Call this function from CLD::unload()  now for anonymous 
> classes.
>
> Anonymous classes aren't in the system dictionary so 
> release_C_heap_structures isn't called for them if they are unloaded.  
> I fixed this for jdk8 because they are in the class loader data graph 
> but for jdk7, I don't know how to fix this. They are unloaded by being 
> garbage collected after the references are dropped.
>
> Tested with ute tests vm.mvlm.testlist nsk.monitoring.testlist and 
> jdk/test/java/lang/invoke tests.
>
> open webrev at http://cr.openjdk.java.net/~coleenp/8011803/
> bug link at http://bugs.sun.com/view_bug.do?bug_id=8011803

I've two comments.

1) It's unfortunate that we now call unload_class() at two different 
places in the code. It makes it harder to understand the deallocation of 
metadata. It would be better to keep the calls to unload_class() for 
ordinary InstanceKlasses and anonymous classes together.

2) This change actually breaks the Tracing class unloading event that 
Markus is working on. That code walk the 
ClassLoaderDataGraph::_unloading list in 
ClassLoaderDataGraph::do_unloading and use the information in the 
classes of the CLDs that are about to be unloaded. With this patch they 
probably crash because some pointers in the anonymous classes will be 
trashed.

I have a suggestion on how to solve both points above. It's probably 
easier to show with a pseudo patch:

$ hg diff
diff --git a/src/share/vm/classfile/classLoaderData.cpp 
b/src/share/vm/classfile/classLoaderData.cpp
--- a/src/share/vm/classfile/classLoaderData.cpp
+++ b/src/share/vm/classfile/classLoaderData.cpp
@@ -671,7 +671,6 @@
      }
      seen_dead_loader = true;
      ClassLoaderData* dead = data;
-    dead->unload();
      data = data->next();
      // Remove from loader list.
      if (prev != NULL) {
@@ -683,9 +682,24 @@
      dead->set_next(_unloading);
      _unloading = dead;
    }
+
+  report_class_unloading_info(_unloading);
+
+  ClassLoaderDataGraph::unload_classes();
+
    return seen_dead_loader;
  }

+void ClassLoaderDataGraph::unload_classes() {
+  ClassLoaderData* list = _unloading;
+  ClassLoaderData* next = list;
+  while (next != NULL) {
+    for (Klass* k = next->_klasses; k != NULL; k = k->next_link()) {
+      k->unload_class();
+    }
+  }
+}
+
  void ClassLoaderDataGraph::purge() {
    ClassLoaderData* list = _unloading;
    _unloading = NULL;
diff --git a/src/share/vm/classfile/dictionary.cpp 
b/src/share/vm/classfile/dictionary.cpp
--- a/src/share/vm/classfile/dictionary.cpp
+++ b/src/share/vm/classfile/dictionary.cpp
@@ -157,9 +157,6 @@
              // to be unloaded.
              // Notify the debugger and clean up the class.
              class_was_unloaded = true;
-
-            // perform unloading actions on this class
-            ik->unload_class();
            }
            // Also remove this system dictionary entry.
            purge_entry = true;

As a bonus, the dictionary()->do_unloading() will then only do a simple 
purge of the SystemDictionary.

What do you think?

thanks,
StefanK

>
> Thanks,
> Coleen