RFR: 8247470: Fix CHeap GrowableArray NMT accounting

[coleen.phillimore at oracle.com]

Looks good.  Now there are no ambiguous constructors?

I was looking to see if the GrowableArray code in os_linux.cpp was in 
windows/bsd, but it's not but os_windows includes growableArray.hpp.  I 
wonder if you could try taking it out?  It seems related to this 
change.  If not, it's not important.

thanks,
Coleen

On 6/15/20 2:05 PM, Stefan Karlsson wrote:
> 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
>