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

[Daniel D. Daugherty]

On 5/29/18 7:48 AM, coleen.phillimore at oracle.com wrote:
>
>
> 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.

If I remember the history correctly, you could have a GrowableArray
object that was stack allocated and elements that were C-heap allocated.
When the stack allocated GrowableArray object was freed, we lost track
of the C-heap allocated elements...

Yes, we could have changed the stack allocated GrowableArray object to
free the C-heap allocated elements, but then the life-cycle started to
get confusing... As in: why was my C-heap allocated data freed without
my permission? Oh... I didn't C-heap allocate my GrowableArray object
so my C-heap allocated data was freed... stuff like that...

Dan


Dan


>
> Coleen
>
>>
>> /Per
>