RFR: 8227168: Cleanup usage of NEW_C_HEAP_ARRAY

[Kim Barrett]

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