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

[Coleen Phillimore]

Hi,

I have updated the webrev to split the serviceability actions and the 
deleting C heap data for classes in ClassLoaderData.  I left the 
serviceability actions in dictionary::do_unloading() to avoid rewalking 
the ClassLoaderData _klasses.   For release_C_heap_structures(), all the 
_klasses are walked at the end.   This should work for Markus G's new 
serviceability changes with minor modifications.

open webrev at http://cr.openjdk.java.net/~coleenp/8011803_2/

thanks,
Coleen

On 4/12/2013 8:38 AM, Coleen Phillimore wrote:
> 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
>>
>