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

[Coleen Phillimore]

On 4/12/2013 3:03 PM, Coleen Phillimore wrote:
>
> 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.

Reload if you looked at this already.  Stefan convinced me that walking 
the class list in the ClassLoaderData::unload() is not too expensive, so 
the serviceability actions have been removed from 
dictionary::do_unloading() and done in ClassLoaderData::unload() 
unconditionally.

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