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

[Mikael Gerdin]

Coleen

On 04/17/2013 05:57 PM, Coleen Phillimore wrote:
>
> 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).

Well, I see what you mean regarding the destructor but it seems kind of 
strange to always shift the symbol unloading to the next GC as well.

I'm not going to insist on you changing this.

/Mikael

>
> Coleen
>
>>
>> /Mikael
>>
>>> but delaying
>>>>
>>>> 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
>>>>>>
>>>>>
>>>>
>>>
>