RFR: 8247470: Fix CHeap GrowableArray NMT accounting

[coleen.phillimore at oracle.com]

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.

I really like this change.

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