RFR: 8227168: Cleanup usage of NEW_C_HEAP_ARRAY

[David Holmes]

I'll concur with Kim on this. Restore the comment in allocation.hpp 
despite the inaccuracy - or else fix the blatant errors but don't worry 
about elaborating on missing macros.

Thanks,
David

On 4/09/2019 4:33 am, Kim Barrett wrote:
>> On Sep 3, 2019, at 12:12 PM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>
>> On 03/09/2019 01:46, Kim Barrett wrote:
>>>> On Sep 2, 2019, at 5:59 PM, David Holmes <david.holmes at oracle.com> wrote:
>>>> On 2/09/2019 10:23 pm, Leo Korinth wrote:
>>>>> Hi!
>>>>> After I got caught doing an unnecessary check on the return value of NEW_C_HEAP_ARRAY (a mistake that I copied) I thought it would be good to do a cleanup in the sources so that others would not fall into this trap. This is the result.
>>>>> I have removed some places where the VM will be shut down after NEW_C_HEAP_ARRAY returns NULL (it never does return NULL, it does instead exit). I have also removed lots of unnecessary casts, that might hide bugs.
>>>>
>>>> Those changes all seem fine, as are the perfMemory changes.
>>>>
>>>> My only issue is with the places where you can changed allocation of a NEW_C_HEAP_ARRAY of size 1 to a NEW_C_HEAP_OBJ. They are still freed with FREE_C_HEAP_ARRAY which looks odd even though correct (in that it's all os::free under the covers anyway). I'm not sure this change is worth the potential confusion it may cause.
>>> I noticed that too, and then forgot about it.  I agree that’s not right.  I think those places
>>> should be changed to use FREE_C_HEAP_OBJ for the deallocation.
>>
>> Thank you for finding this. I actually considered implementing the macro FREE_C_HEAP_OBJ as I could not find it. Now I can not understand how I could /miss/ it, though I guess I was looking for a macro with the same signature as FREE_C_HEAP_ARRAY (one taking the type as argument) :-(
>>
>> As a consequence I also deleted some outdated help text:
>>
>> - //   NEW_RESOURCE_ARRAY(type, size)
>> - //   NEW_RESOURCE_OBJ(type)
>> - //   NEW_C_HEAP_ARRAY(type, size)
>> - //   NEW_C_HEAP_OBJ(type, memflags)
>> - //   FREE_C_HEAP_ARRAY(type, old)
>> - //   FREE_C_HEAP_OBJ(objname, type, memflags)
>> - //   char* AllocateHeap(size_t size, const char* name);
>> - //   void  FreeHeap(void* p);
> 
> I think just deleting the list of macros and functions isn't right.
> Doing so leaves the preceeding two paragraphs dangling, e.g. "The
> following macros and function ...".
> 
> That whole section of the comment could use some TLC.  That doesn't
> need to be part of this change.
> 
>> I have tried to use FREE_C_HEAP_OBJ correctly, and I did revert one usage of NEW_C_HEAP_OBJ to NEW_C_HEAP_ARRAY as it seemed to be able to also be an array type when allocated in another place.
> 
> I think create_multi_counter_query is only paired with the appropriate
> overload of destroy_counter_query, and could use NEW/FREE_C_HEAP_OBJ.
> 
> The array case only seems to arise with the set member of
> MultiCounterQuerySet, which uses NEW/FREE_C_HEAP_ARRAY (in
> destroy_multi_counter_query, which is strictly a helper for some
> overloads of destroy_counter_query where an array is involved.
> 
> But I can see why you might take the simple approach here.  I wonder
> why the query types are POD with all these file-scoped create/destroy
> helper functions, rather than having constructors and destructors.  I
> suspect that would make these questions around the lifetime management
> much simpler.
> 
>>
>> Incremental:
>> http://cr.openjdk.java.net/~lkorinth/8227168/00_01/
>>
>> Full:
>> http://cr.openjdk.java.net/~lkorinth/8227168/01/
>>
>> rerunning tests...
>>
>> Thanks, Leo
> 
> Other than the partial comment deletion, this looks good to me.
> 
> I don’t need a new webrev if you are just going to reinstate that partial comment.
>