RFR: 8230398: Remove NULL checks before FREE_C_HEAP_ARRAY

[David Holmes]

Okay - no point belabouring this. The code is functionally fine.

The os::free/malloc counters seem to be only used for reporting when 
memory verification fails and they're maintained in a buggy way already.

Thanks,
David

On 3/09/2019 1:05 pm, Kim Barrett wrote:
>> 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.
> 
>