RFR: 8135322: ConstantPool::release_C_heap_structures not run in some circumstances
Coleen Phillimore
coleen.phillimore at oracle.com
Thu May 12 15:50:07 UTC 2016
This looks great. Thank you for finding this subtle problem.
Coleen
On 5/12/16 8:54 AM, Andreas Eriksson wrote:
> Hi,
>
> Here is a new webrev where free_deallocate_list() is called in
> ClassLoaderData::unload() instead.
> http://cr.openjdk.java.net/~aeriksso/8135322/webrev.02/
>
> Thanks,
> Andreas
>
>
> On 2016-04-12 14:11, Andreas Eriksson wrote:
>> Hi,
>>
>> Here is a new webrev where I do the filtering of the _deallocate_list.
>> http://cr.openjdk.java.net/~aeriksso/8135322/webrev.01/
>>
>> Another possible version is to call free_deallocate_list in
>> ClassLoaderDataGraph::do_unloading.
>> http://cr.openjdk.java.net/~aeriksso/8135322/webrev.do_unloading/
>>
>> Both of these versions pass the test which crashed.
>> What do you think? Any preference for one of the versions?
>>
>> Thanks,
>> Andreas
>>
>> On 2016-04-01 18:22, Andreas Eriksson wrote:
>>> Hi,
>>>
>>> When running some additional testing I had a test crash.
>>> Looks like sometimes data we need to run a full deallocate_contents
>>> call when we are in ~ClassLoaderData is already freed (seems to
>>> depend on what GC we are running, and what we have in the
>>> _deallocate_list).
>>>
>>> Doing the free_deallocate_list call in
>>> ClassLoaderDataGraph::do_unloading instead will most likely work,
>>> since it's run at an earlier point in the GC.
>>> Or I could do what I mentioned in the initial email, only run
>>> release_C_heap_structures() on constant pools in the list that do
>>> not have a corresponding InstanceKlass.
>>>
>>> I'm leaning towards, doing the free_deallocate_list call in
>>> ClassLoaderDataGraph::do_unloading, when we detect that the
>>> ClassLoaderData is dead.
>>> I'll send out a new review once I've had some time to test that fix.
>>>
>>> Thanks,
>>> Andreas
>>>
>>> On 2016-04-01 02:15, serguei.spitsyn at oracle.com wrote:
>>>> Andreas,
>>>>
>>>> I agree with Coleen, it is a good find and a good and safe fix.
>>>>
>>>> Thanks!
>>>> Serguei
>>>>
>>>>
>>>> On 3/30/16 16:06, Coleen Phillimore wrote:
>>>>>
>>>>> Andreas,
>>>>>
>>>>> Good find! This seems like a safe fix. It's unfortunate to have
>>>>> to walk the deallocate list, but I think it's short enough and
>>>>> less work in comparison to having to call free_C_heap_structures
>>>>> on all the other classes in the dead class loader data. And
>>>>> deallocate_contents removes the scratch_class from the CLD
>>>>> _klasses list so won't walk it twice.
>>>>>
>>>>> This is really good.
>>>>>
>>>>> Coleen
>>>>>
>>>>> On 3/30/16 8:12 AM, Andreas Eriksson wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this fix for 8135322:
>>>>>> ConstantPool::release_C_heap_structures not run in some
>>>>>> circumstances.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8135322
>>>>>> Webrev: http://cr.openjdk.java.net/~aeriksso/8135322/webrev.00
>>>>>>
>>>>>> The way this occurs is:
>>>>>>
>>>>>> 1. A ConstantPool, /merge_cp/, without a corresponding
>>>>>> InstanceKlass is
>>>>>> created during class redefinition.
>>>>>> 2. /Merge_cp/ is added to ClassLoaderData::_deallocate_list, but
>>>>>> is not
>>>>>> cleaned up by ClassLoaderDataGraph::do_unloading because the
>>>>>> holding
>>>>>> ClassLoaderData is already dead.
>>>>>> 3. ClassLoaderData::~ClassLoaderData is called, which usually runs
>>>>>> InstanceKlass::release_C_heap_structures, which in turn will call
>>>>>> ConstantPool::release_C_heap_structures. But since there is no
>>>>>> corresponding InstanceKlass for /merge_cp/ it will never
>>>>>> release its
>>>>>> C heap structures.
>>>>>> 4. For JDK 8 this means we leak a Monitor (ConstantPool::_lock). For
>>>>>> JDK 9 the Monitor has been removed, but we will miss doing a
>>>>>> call to
>>>>>> ConstantPool::unreference_symbols, which is probably not good
>>>>>> either.
>>>>>>
>>>>>>
>>>>>> This change adds a call to free everything in the
>>>>>> _deallocate_list first thing in ClassLoaderData::~ClassLoaderData.
>>>>>>
>>>>>> Note:
>>>>>> Running deallocate_contents() on everything in the list might be
>>>>>> doing unnecessary work:
>>>>>> What we really would like to do is run
>>>>>> release_C_heap_structures() on constant pools in the list that do
>>>>>> not have a corresponding InstanceKlass. Not sure it's worth the
>>>>>> effort to do the filtering, since I believe the _deallocate_list
>>>>>> is usually not that long.
>>>>>>
>>>>>> Note 2:
>>>>>> After fixing in JDK 9 I'll backport this to JDK 8, where it has a
>>>>>> more immediately visible effect.
>>>>>>
>>>>>> Regards,
>>>>>> Andreas
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list