RFR: 8135322: ConstantPool::release_C_heap_structures not run in some circumstances

[Andreas Eriksson]

On 2016-03-31 05:02, serguei.spitsyn at oracle.com wrote:
> On 3/30/16 19:52, serguei.spitsyn at oracle.com wrote:
>> Hi Andreas,
>>
>>
>> One simple question so far:
>>   Should the _deallocate_list be set to NULL after deletion at L428:
>> 424 ClassLoaderData::~ClassLoaderData() {
>> 425 // Delete free list
>> 426 if (_deallocate_list != NULL) {
>> 427 free_deallocate_list();
>> 428 delete _deallocate_list;
>> 429 }
>> 430
>
> Please, skip my question.
> It is too paranoid to do so. :)

Alright, thanks for looking at this!

- Andreas

>
>
> Thanks,
> Serguei
>
>
>>
>>
>> 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
>>>>
>>>
>>
>