RFR: 8247470: Fix CHeap GrowableArray NMT accounting

[Stefan Karlsson]

Hi all,

Updated webrevs:

https://cr.openjdk.java.net/~stefank/8247470/webrev.02.delta/
https://cr.openjdk.java.net/~stefank/8247470/webrev.02/

Testing showed that the previous patch was incomplete. I changed the 
signature and expected the compiler to show all places I needed to 
update. The intention was that the compiler should see that true/false 
wasn't a MEMFLAGS and complain. Unfortunately, there was a problem with 
the following pattern:

   new (ResourceObj::C_HEAP, mtCode)GrowableArray<const char*>(0, true);

which matches the following constructor:

// Resource allocation
GrowableArray(Thread* thread, int initial_size) : 
GenericGrowableArray(initial_size, 0, mtNone) {

and yields thread == NULL (0) and initial_size == 1 (true).

There were only two places that used the Thread* parameter. This special 
constructor seems to have been a performance optimization at some point. 
But we've made Thread::current() much cheaper, and neither of the 
affected call sites seems to be performance sensitive. I opted to 
simplify the code and removed that constructor.

The systemDictionaryShared.cpp was needed after I rebased the patch.

Thanks,
StefanK


On 2020-06-12 11:33, 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