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

[Coleen Phillimore]

On 4/12/2013 4:48 AM, Stefan Karlsson wrote:
> 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.

Hi Stefan,  Thank you for looking at this.   This is the feedback I was 
looking for.

I went back and forth about moving the regular class unloading actions 
for everything in unload (or ~ClassLoaderData), but I didn't want to 
introduce a _klasses walk for all the class loaders.   We're already 
walking them in the system dictionary purge.

For my change I'm walking these classes if UseCompressedKlassPointers, 
and Markus is walking them for serviceability (hopefully under some 
flag) so maybe I should just do it.

>
> 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 thought about that, since I've been talking to Markus about this.   I 
wanted to consolidate the serviceability calls in one place and calling 
InstanceKlass::unload_class() that notifies serviceability in the 
~ClassLoaderData destructor might be too late.  It might not be 
though.   I think the walk in the destructor might be better for 
release_C_heap_structures though.

> 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();
> +    }
> +  }
> +}
> +

That's too much extra walking.   But maybe we can split the 
serviceability stuff into ClassLoaderData::unload() and do the 
release_C_heap_structures() into the ClassLoaderData destructor. That 
seems better.   Maybe Markus could put his callback into CLD::unload().

Thanks!  I will change this and resend.
Coleen

>  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
>