RFR: 8227168: Cleanup usage of NEW_C_HEAP_ARRAY
Leo Korinth
leo.korinth at oracle.com
Tue Sep 3 16:12:45 UTC 2019
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.
Incremental:
http://cr.openjdk.java.net/~lkorinth/8227168/00_01/
Full:
http://cr.openjdk.java.net/~lkorinth/8227168/01/
rerunning tests...
Thanks, Leo
More information about the hotspot-dev
mailing list