RFR: 8227168: Cleanup usage of NEW_C_HEAP_ARRAY
david.holmes at oracle.com
Wed Sep 4 03:35:22 UTC 2019
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.
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:
>>>>> 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.
>> 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.
More information about the hotspot-dev