RFR: 8230398: Remove NULL checks before FREE_C_HEAP_ARRAY

[Kim Barrett]

> On Sep 2, 2019, at 6:09 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Leo,
> 
> On 2/09/2019 11:06 pm, Leo Korinth 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.
> 
> I don't see such a NULL check except in debug builds:
> 
> #define FREE_C_HEAP_ARRAY(type, old) \
>  FreeHeap((char*)(old))
> 
> void FreeHeap(void* p) {
>  os::free(p);
> }
> 
> void  os::free(void *memblock) {
>  NOT_PRODUCT(inc_stat_counter(&num_frees, 1));
> #ifdef ASSERT
>  if (memblock == NULL) return;
>  if ((intptr_t)memblock == (intptr_t)MallocCatchPtr) {
>    log_warning(malloc, free)("os::free caught " PTR_FORMAT, p2i(memblock));
>    breakpoint();
>  }
>  void* membase = MemTracker::record_free(memblock);
>  verify_memory(membase);
> 
>  GuardedMemory guarded(membase);
>  size_t size = guarded.get_user_size();
>  inc_stat_counter(&free_bytes, size);
>  membase = guarded.release_for_freeing();
>  ::free(membase);
> #else
>  void* membase = MemTracker::record_free(memblock);
>  ::free(membase);
> #endif
> }

os::free is like ::free; if the argument is NULL then no operation is
performed. Any other behavior seems like it would be surprising. And I
suspect there is code that assumes that. This change is just cleaning
up code that has a redundant check.

In the !ASSERT case one has to do a bit more call chain following to
find the NULL check:

MemTracker::record_free(void* memblock)
  => MallocTracker::record_free(void* memblock)
  => if (MemTracker::tracking_level() == NMT_off ||
         memblock == NULL) {      // <<<< NULL check is here
       return memblock;
     }

The returned value is passed to ::free.

It might be more obvious if os::free had an up-front NULL check, and
MallocTracker could assume (assert) the memblock is non-NULL.  But
such a NULL check would be redundant when NMT_off.


> 
> Also this would now count NULL frees (which seems a bug in itself).

Depends on whether it is counting the calls to free, or counting the number of allocated
blocks that were freed.

>> 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?
> 
> It looks like dead code. People often write clean symmetrical allocation and freeing code, but the VM doesn't actually free a lot of stuff at termination.

It’s a file-scoped function; some compilers (like gcc with appropriate options) would warn
about it being defined but unused.

Hm, but the indentation didn’t get updated when the surrounding if was removed.

> 
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8230398
>> Webrev:
>> http://cr.openjdk.java.net/~lkorinth/8230398
>> Testing:
>> mach5 tier1-3
>> Thanks,
>> Leo