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

[coleen.phillimore at oracle.com]

On 5/29/18 2:20 AM, Per Liden wrote:
> Hi Kim,
>
> On 05/29/2018 08:09 AM, Kim Barrett wrote:
>>> On May 28, 2018, at 8:16 AM, Per Liden <per.liden at oracle.com> 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.
>>>
>>> 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
>>
>> Looks good.
>
> Thanks for reviewing!
>
>>
>> I was going to ask why the GrowableArray needs to be heap allocated?
>> Why not just
>>
>>    GrowableArray<oop> aos_objects(INITIAL_ARRAY_SIZE, true);
>>    ... s/aos_objects/&aos_objects/ ...
>>    // delete aos_objects no longer needed
>>
>> After some digging, probably because of this, from growableArray.hpp:
>>
>> 119    assert(!on_C_heap() || allocated_on_C_heap(), "growable array 
>> must be on C heap if elements are");
>
> I had the same reaction and initially made it stack allocated, but 
> noticed that GrowableArray doesn't allow that :(
>
>>
>> Conflating the allocation of the GrowableArray object with the
>> allocation of the underlying array.  That seems wrong, but not a
>> problem to be solved as part of this change.
>>
>
> I agree. I suspect someone wanted to protect against potential 
> problems with having different life cycles of the GrowableArray and 
> the backing array. But this seems overly strict.

Yes, this is exactly the reason, and we did have bugs.  That's why we 
have this check.

Coleen

>
> /Per