RFR: 8227168: Cleanup usage of NEW_C_HEAP_ARRAY

[Leo Korinth]

On 04/09/2019 05:35, David Holmes wrote:
> 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


how about:

//NEW_RESOURCE_ARRAY*
//REALLOC_RESOURCE_ARRAY*
//FREE_RESOURCE_ARRAY*
//FREE_FAST
//NEW_RESOURCE_OBJ*
//NEW_C_HEAP_ARRAY*
//REALLOC_C_HEAP_ARRAY*
//FREE_C_HEAP_ARRAY*
//NEW_C_HEAP_OBJ*
//FREE_C_HEAP_OBJ
//
//char* AllocateHeap(size_t size, MEMFLAGS flags, const NativeCallStack& 
stack, AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM);
//char* AllocateHeap(size_t size, MEMFLAGS flags, AllocFailType 
alloc_failmode = AllocFailStrategy::EXIT_OOM);
//char* ReallocateHeap(char *old, size_t size, MEMFLAGS flag, 
AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM);

It is (at least) better than what is present, I could also list them all:

//NEW_RESOURCE_ARRAY(type, size)
//NEW_RESOURCE_ARRAY_RETURN_NULL(type, size)
//NEW_RESOURCE_ARRAY_IN_THREAD(thread, type, size)
//NEW_RESOURCE_ARRAY_IN_THREAD_RETURN_NULL(thread, type, size)
//REALLOC_RESOURCE_ARRAY(type, old, old_size, new_size)
//REALLOC_RESOURCE_ARRAY_RETURN_NULL(type, old, old_size, new_size)
//FREE_RESOURCE_ARRAY(type, old, size)
//FREE_FAST(old)
//NEW_RESOURCE_OBJ(type)
//NEW_RESOURCE_OBJ_RETURN_NULL(type)
//NEW_C_HEAP_ARRAY3(type, size, memflags, pc, allocfail)
//NEW_C_HEAP_ARRAY2(type, size, memflags, pc)
//NEW_C_HEAP_ARRAY(type, size, memflags)
//NEW_C_HEAP_ARRAY2_RETURN_NULL(type, size, memflags, pc)
//NEW_C_HEAP_ARRAY_RETURN_NULL(type, size, memflags)
//REALLOC_C_HEAP_ARRAY(type, old, size, memflags)
//REALLOC_C_HEAP_ARRAY_RETURN_NULL(type, old, size, memflags)
//FREE_C_HEAP_ARRAY(type, old)
//NEW_C_HEAP_OBJ(type, memflags)
//NEW_C_HEAP_OBJ_RETURN_NULL(type, memflags)
//FREE_C_HEAP_OBJ(objname)
//
//char* AllocateHeap(size_t size, MEMFLAGS flags, const NativeCallStack& 
stack, AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM);
//char* AllocateHeap(size_t size, MEMFLAGS flags, AllocFailType 
alloc_failmode = AllocFailStrategy::EXIT_OOM);
//char* ReallocateHeap(char *old, size_t size, MEMFLAGS flag, 
AllocFailType alloc_failmode = AllocFailStrategy::EXIT_OOM);

I propose the first shorter (asterisked) version.

Thanks,
Leo

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