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

[Mikael Gerdin]

Coleen,

On 2013-04-12 23:36, Coleen Phillimore wrote:
> 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.

It looks like http://cr.openjdk.java.net/~coleenp/8011803_2/ is the 
wrong webrev since it's inconsistent with your mail.
But I have some comments about moving the calls to 
release_c_heap_structures.

By moving the calls to {IK,CP}::release_c_heap_structures to 
~ClassLoaderData they will be called concurrently from the CMS thread 
through its call to ClassLoaderDataGraph::purge.

Previously they were called at a safepoint since 
Dictionary::do_unloading is only called at safepoint.
A cursory inspection suggests that this is safe.

However.
This will cause the symbol refcounts to be decremented at a later stage 
than before, in fact by the time we come to ~ClassLoaderData we've 
already called SymbolTable::unkink so this will artificially extend the 
lifetime of unreferenced symbols by one full gc.

/Mikael

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