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

[Coleen Phillimore]

Mikael, Thanks for looking at this in detail.

On 4/15/2013 4:23 AM, Mikael Gerdin wrote:
> 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.

I forgot to copy it over.  Please reload.
>
>
> 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.

Yes, this is true and a good point.  I feel like it's safer actually 
since all the memory shouldn't go away before the destructor is run.   
Symbols aren't reclaimed that much so this change is not likely to cause 
a measurable increase in footprint between GC's (famous last words).

Coleen

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