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

[Andreas Eriksson]

Thanks Coleen and Serguei!

- Andreas


On 2016-05-12 19:06, serguei.spitsyn at oracle.com wrote:
> +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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>