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

[Stefan Karlsson]

On 2013-04-17 17:57, 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).
If you are OK with the Symbols being reclaimed later, and that we now 
release the C heap structures concurrently with the mutator threads 
(CMS), then this looks good to me.

thanks,
StefanK

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