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

[serguei.spitsyn at oracle.com]

+1

On 5/12/16 08:50, Coleen Phillimore wrote:
>
> 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
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>