RFR: 8247470: Fix CHeap GrowableArray NMT accounting

[Stefan Karlsson]

On 2020-06-15 20:16, coleen.phillimore at oracle.com wrote:
>
> Looks good.  Now there are no ambiguous constructors?

Maybe this one could have been problematic:
GrowableArray(Arena* arena, int initial_size, int initial_len, const E& 
filler)

If some code previously did, say:
GrowableArray(0, 0, 0, false)

and tried to match the old:
GrowableArray(int initial_size, int initial_len, const E& filler, bool 
C_heap = false, MEMFLAGS memflags = mtNone)

it could have called the arena version.

I temporarily added a dummy parameter like this:
GrowableArray(Arena* arena, GADummy dummy, int initial_size, int 
initial_len, const E& filler)

and fixed all call-sites where arena GrowableArrays are used. It 
compiles on Linux x64. I'll verify that it builds on the other platforms 
as well.

>
> 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.

I'll take it out and let the full test of this run overnight.

Thanks,
StefanK
>
> 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
>>
>