RFR: 8230398: Remove NULL checks before FREE_C_HEAP_ARRAY

[Kim Barrett]

> On Sep 2, 2019, at 8:37 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> 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.

Except it's not an optimization.  The value will usually (nearly(?)
always) be non-NULL, making the NULL pre-check is a waste of time and
code.  Indeed, in many cases a NULL value is simply not possible; the
value is the result of a NEW_C_HEAP_ARRAY call that would have
terminated the application rather than returning NULL.

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

As usual, there isn’t any documentation of the intended behavior; one must
read the code.  That’s a separate problem.

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

Looks harmless to me, especially since it doesn’t affect product code.
The only configuration that is being changed is the “optimize” build that various
folks periodically suggest killing off.