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

[Andreas Eriksson]

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