RFR (S): 8011803: release_C_heap_structures is never called for anonymous classes.
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Apr 22 13:46:27 PDT 2013
On 04/22/2013 02:10 PM, Stefan Karlsson wrote:
> 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.
Yes, I am ok with this. I still think it's safer than having classes on
the _unloaded list pointing to null symbols for names.
Thanks,
Coleen
>
> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
More information about the hotspot-dev
mailing list