Re: [PATCH] Annotate malloc conditions with glibc_likely.

[Adhemerval Zanella <azanella at linux dot vnet dot ibm dot com>]

I'm also not very found of this kind of annotations. In some performance evaluations 
for ppc I have found some branch prediction instructions were in fact hurting performance
for newer chips. You may argue the compiler should take it just as hint, however that
not usually the case. As Will notes, I prefer if you could provide a benchtest that
proves the modification show in fact better performance.


On 10-12-2013 07:54, Will Newton wrote:
> On 9 December 2013 20:44, OndÅej BÃlka <neleai@seznam.cz> wrote:
>> Hi,
>>
>> Another little optimization in malloc is annotate that fastbins are
>> likely case. These should be fast path and given that other path
>> slower it should pay off.
> In general I am not in favour of adding these types of annotations
> unless we can show they improve performance as they make code harder
> to read. They are also something of a blunt instrument - does "likely"
> mean will happen with 75% likelihood? 90%? 99%? I guess the results
> will vary with compiler and architecture as to whether or not the
> annotation helps or not.
>
>> I also moved a short mmmaped case before long normal one to make code
>> more readable.
> I think this should be a separate patch.
>
>> There is a discrepancy in freeing mmaped chunks, this is first done in
>> __libc_free and __libc_realloc. I wonder from where in __int_free mmaped
>> chunk comes from.
>>
>>
>>         * malloc/malloc.c (_int_malloc, _int_free): Annotate branches
>>         by __glibc_likely/__glibc_unlikely.
>>
>>
>> diff --git a/malloc/malloc.c b/malloc/malloc.c
>> index 393316e..c9d67f3 100644
>> --- a/malloc/malloc.c
>> +++ b/malloc/malloc.c
>> @@ -3221,7 +3221,8 @@ _int_malloc(mstate av, size_t bytes)
>>      can try it without checking, which saves some time on this fast path.
>>    */
>>
>> -  if ((unsigned long)(nb) <= (unsigned long)(get_max_fast ())) {
>> +  if (__glibc_likely((unsigned long)(nb) <= (unsigned long)(get_max_fast ()))
>> +     ) {
> The coding style looks a little odd (all the malloc code does, but
> never mind!) and the casts to unsigned long are superfluous if
> INTERNAL_SIZE_T becomes size_t.
>
>>      idx = fastbin_index(nb);
>>      mfastbinptr* fb = &fastbin (av, idx);
>>      mchunkptr pp = *fb;
>> @@ -3711,8 +3712,8 @@ _int_free(mstate av, mchunkptr p, int have_lock)
>>      and used quickly in malloc.
>>    */
>>
>> -  if ((unsigned long)(size) <= (unsigned long)(get_max_fast ())
>> -
>> +  if (__glibc_likely ((unsigned long)(size) <=
>> +                     (unsigned long)(get_max_fast ()))
> Again the casts can be removed if we are using size_t.
>
>>  #if TRIM_FASTBINS
>>        /*
>>         If TRIM_FASTBINS set, don't place chunks
>> @@ -3777,12 +3778,15 @@ _int_free(mstate av, mchunkptr p, int have_lock)
>>         goto errout;
>>        }
>>    }
>> +  else if (__glibc_unlikely(chunk_is_mmapped(p))) {
>> +    munmap_chunk (p);
>> +  }
>> +  else {
>>
>>    /*
>>      Consolidate other non-mmapped chunks as they arrive.
>>    */
>>
>> -  else if (!chunk_is_mmapped(p)) {
>>      if (! have_lock) {
>>  #if THREAD_STATS
>>        if(!mutex_trylock(&av->mutex))
>> @@ -3929,13 +3933,6 @@ _int_free(mstate av, mchunkptr p, int have_lock)
>>        (void)mutex_unlock(&av->mutex);
>>      }
>>    }
>> -  /*
>> -    If the chunk was allocated via mmap, release via munmap().
>> -  */
>> -
>> -  else {
>> -    munmap_chunk (p);
>> -  }
>>  }
>>
>>  /*
>
>