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

[Per Liden]

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?

/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