RFR: 8230398: Remove NULL checks before FREE_C_HEAP_ARRAY

[David Holmes]

On 3/09/2019 9:41 am, Kim Barrett wrote:
>> 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.

I understand this is all functionally correct. But I don't see that it 
is necessarily a bad thing to do these "redundant" NULL checks up front 
as an optimisation rather than relying on the called code to eventually 
notice there is nothing to do. And it's not obvious that its okay to use 
FREE_C_HEAP_ARRAY with a NULL value, so you can understand why people 
add the check.

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

Whichever it is this change results in a change to that behaviour. It's 
probably harmless, but its a change.

Cheers,
David

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