RFR: 8230398: Remove NULL checks before FREE_C_HEAP_ARRAY

[Leo Korinth]

On 10/09/2019 23:44, David Holmes wrote:
> Hi Leo,
> 
> Looks fine.
> 
> Thanks,
> David

Thanks for the review! /Leo

> On 10/09/2019 11:39 pm, Leo Korinth wrote:
>>
>>
>> On 02/09/2019 22:38, Kim Barrett wrote:
>>>> On Sep 2, 2019, at 9:06 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
>>>>
>>>> Hi!
>>>>
>>>> This fixup removes NULL checks before FREE_C_HEAP_ARRAY. This is 
>>>> unnecessary as FREE_C_HEAP_ARRAY does also do this NULL check. This 
>>>> is a follow up to 8227168: Cleanup usage of NEW_C_HEAP_ARRAY.
>>>>
>>>> deallocate_counters() in src/hotspot/os/windows/os_perf_windows.cpp 
>>>> does (in addition to NULL the pointer) also set a counter to zero. 
>>>> Although my change "looks" correct, I tried follow the usage of 
>>>> deallocate_counters(), and could not find a call site. Should I 
>>>> create a bug on this, or am I missing something here?
>>>
>>> Your modification to deallocate_counters looks fine.  I don't think
>>> you are missing anything, I think it's not called.  That does look
>>> like it might be a (pre-existing) bug.  Maybe they are immortal?  But
>>> that isn't obvious from a quick skim.
>>>
>>>> Bug:
>>>> https://bugs.openjdk.java.net/browse/JDK-8230398
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~lkorinth/8230398
>>>>
>>>> Testing:
>>>> mach5 tier1-3
>>>>
>>>> Thanks,
>>>> Leo
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> [pre-existing]
>>> There are some destructors that are unnecessarily nulling out members
>>> after deleting their values, such as here:
>>>
>>> src/hotspot/share/classfile/classLoader.cpp
>>>   387   FREE_C_HEAP_ARRAY(const char, _name);
>>>   388   _name = NULL;
>>>
>>> src/hotspot/share/gc/g1/sparsePRT.cpp
>>> 106 RSHashTable::~RSHashTable() {
>>>
>>> src/hotspot/share/gc/shared/cardTableRS.cpp
>>> 626 CardTableRS::~CardTableRS() {
>>>
>>> src/hotspot/share/runtime/thread.cpp
>>> 1330 NamedThread::~NamedThread() {
>>>
>>> Your call whether you want to do anything about these with this change.
>>>
>>> ------------------------------------------------------------------------------ 
>>>
>>>
>>> Other than that, looks good.  I don't need another webrev if you
>>> decide to remove the above assignments to NULL near code you were
>>> already changing.  A more extensive pass for such should perhaps be a
>>> separate change.
>>>
>> I will remove NULL assignments of fields in destructors and keep them 
>> otherwise.
>>
>> I will also add a comment (// handles NULL pointers) to FreeHeap and 
>> os::free to not only describe behaviour, but also to establish a 
>> contract.
>>
>> Also fixed indentation miss in os_perf_windows.cpp
>>
>> Incremental:
>> http://cr.openjdk.java.net/~lkorinth/8230398/00_01
>>
>> Full:
>> http://cr.openjdk.java.net/~lkorinth/8230398/01
>>
>>
>> running tests...