RFR: 8247470: Fix CHeap GrowableArray NMT accounting

[Stefan Karlsson]

On 2020-06-15 14:09, coleen.phillimore at oracle.com wrote:
>
> https://cr.openjdk.java.net/~stefank/8247470/webrev.01/src/hotspot/share/prims/jvmtiCodeBlobEvents.cpp.udiff.html 
>
>
> I thought there was an mtJVMTI tag or that I filed a bug to add one 
> but I guess not.  mtServiceability is fine.  Thank you for fixing these.
>
> https://cr.openjdk.java.net/~stefank/8247470/webrev.01/src/hotspot/share/runtime/unhandledOops.cpp.udiff.html 
>
>
> Maybe mtThread  since the unhandled oops are added to the thread, so 
> we can account for them there.

Sure, I'll change it.

>
> I really like this change.

Thanks for reviewing!

StefanK

>
> Thank you!
> Coleen
>
>
> On 6/12/20 5:33 AM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to fix the NMT accounting of CHeap 
>> GrowableArray allocations.
>>
>> https://cr.openjdk.java.net/~stefank/8247470/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8247470
>>
>> The initial reason why I started fixing this was that I found code 
>> like this:
>>
>> src/hotspot/share/classfile/javaClasses.cpp:
>>     new (ResourceObj::C_HEAP, mtModule) GrowableArray<Klass*>(500, 
>> true);
>>
>> Before this patch, the GrowableArray constructor looked like this:
>>
>> GrowableArray(int initial_size, bool C_heap = false, MEMFLAGS F = 
>> mtInternal)
>> : GenericGrowableArray(initial_size, 0, C_heap, F) {
>> _data = (E*)raw_allocate(sizeof(E));
>>
>> and:
>>
>> GenericGrowableArray(int initial_size, int initial_len, bool c_heap, 
>> MEMFLAGS flags = mtNone) { _len = initial_len; _max = initial_size; 
>> _memflags = flags;
>>
>> and:
>> void* GenericGrowableArray::raw_allocate(int elementSize) {
>> ...
>> return (void*)AllocateHeap(byte_size, _memflags);
>>
>> This means that the GrowableArray instance was accounted to mtModule, 
>> but the backing memory for the array was accounted to mtInternal.
>>
>> My proposal for fixing this is to fuse the (c_heap, flags) duality 
>> into one parameter (flags). The call sites are then changed like this:
>>
>> For resource allocations:
>> GrowableArray(...) -> GrowableArray(...)
>> GrowableArray(... /* c_heap */ false) -> GrowableArray(...)
>> GrowableArray(... /* c_heap */ false, /* flags */ mtXX) -> 
>> GrowableArray(...)
>>
>> For CHeap allocations:
>> GrowableArray(... /* c_heap */ true, /* flags */ mtXX) -> 
>> GrowableArray(..., mtXX)
>> GrowableArray(... /* c_heap */ true) -> GrowableArray(..., mtInternal)
>>
>> That is, if you specify a "valid" mtXX tag it invokes the CHeap 
>> allocation path. If you don't specify a mtXX tag, then 'flags' get 
>> the mtNone default value. This is a marker for the code that no CHeap 
>> allocation should be done.
>>
>> While properly tagging all GrowableArray allocations, I went ahead 
>> and changed most GrowableArray mtInternal tags to the appropriate 
>> values. I also introduced a new mtServiceability tag, since a lot of 
>> the mtInternal tags were in areas dealing with serviceability code. I 
>> could split this out into a separate patch, if necessary.
>>
>> This hasn't been fully tested yet, because I'd like to get some early 
>> feedback.
>>
>> Thanks,
>> StefanK
>