RFR: 8247470: Fix CHeap GrowableArray NMT accounting

[Stefan Karlsson]

Tier1 testing is complete, a large part of tier2 and tier3 is complete 
but some platforms are blocked up. I think this is enough testing of 
this, and I'm going to push this now.

StefanK

On 2020-06-15 22:37, Stefan Karlsson wrote:
> 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
>>>
>>
>