RFR: 8230398: Remove NULL checks before FREE_C_HEAP_ARRAY

[David Holmes]

Hi Leo,

Looks fine.

Thanks,
David

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