RFR: 8227168: Cleanup usage of NEW_C_HEAP_ARRAY

[coleen.phillimore at oracle.com]

On 9/3/19 12:12 PM, Leo Korinth 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 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 like the removal of the unnecessary casts.  But I don't agree.  I 
don't think NEW_C_HEAP_ARRAY should be used to allocate and 
FREE_C_HEAP_OBJ should be used to deallocate.  That's worse.

Actually, I think neither NEW_C_HEAP_OBJ or FREE_C_HEAP_OBJ should 
exist.  They should just use new Type(), where Type is inherited from 
CHeapObj<mtAppropriate>

Thanks,
Coleen

>
> Incremental:
> http://cr.openjdk.java.net/~lkorinth/8227168/00_01/
>
> Full:
> http://cr.openjdk.java.net/~lkorinth/8227168/01/
>
> rerunning tests...
>
> Thanks, Leo