RFR: 8203885: ConcurrentLocksDump::dump_at_safepoint() should not allocate array in resource area

[David Holmes]

On 29/05/2018 1:38 AM, Per Liden wrote:
> Hi David,
> 
> On 05/28/2018 03:20 PM, David Holmes wrote:
>> On 28/05/2018 11:05 PM, Per Liden wrote:
>>> On 05/28/2018 02:47 PM, David Holmes wrote:
>>>> Hi Per,
>>>>
>>>> On 28/05/2018 10:16 PM, Per Liden wrote:
>>>>> ConcurrentLocksDump::dump_at_safepoint() creates a GrowableArray, 
>>>>> which gets allocated in a resource area. This array is than passed 
>>>>> down a call chain, where it can't control that another ResourceMark 
>>>>> isn't created. In the leaf of this call chain, a closure 
>>>>> (FindInstanceClosure) is executed, which appends to the array, 
>>>>> which means it might need to be resized. This doesn't work if a new 
>>>>> ResourceMark has been created, since the array resize will happen 
>>>>> in a nested ResourceArea context. As a result, the append operation 
>>>>> fails in GenericGrowableArray::check_nesting().
>>>>>
>>>>> This has so far gone unnoticed because 
>>>>> CollectedHeap::object_iterate() in existing collectors typically 
>>>>> don't create new ResourceMarks. This is not true for ZGC (and 
>>>>> potentially other concurrent collectors), which needs to walk 
>>>>> thread stacks, which in turn requires a ResourceMark.
>>>>>
>>>>> The proposed fix is to make this array C Heap allocated.
>>>>
>>>> That seems fine in itself but I'm not clear what the OOM behaviour 
>>>> of either the old or the new code is here??
>>>
>>> Thanks for looking at this David.
>>>
>>> On OOM, both the old and the new code will eventually end up in 
>>> vm_exit_out_of_memory().
>>
>> Okay - that's not good. Probably non trivial to fix as the dump has to 
>> allow for failure - and can't directly propagate an exception. I'll 
>> file a bug.
> 
> Check! Since my patch doesn't make this better or worse, I assume you're 
> ok with me going ahead with my change?

Absolutely.

David


> /Per
> 
>>
>> Thanks,
>> David
>>
>>> /Per
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8203885
>>>>> Webrev: http://cr.openjdk.java.net/~pliden/8203885/webrev.0
>>>>>
>>>>> Testing: hs-tier{1,3}
>>>>>
>>>>> /Per