Re: [PATCH 5/5] Add single-threaded path to _int_malloc

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 12/10/2017 16:51, DJ Delorie wrote:
> 
> Wilco Dijkstra <Wilco.Dijkstra@arm.com> writes:
> 
>> +      if (SINGLE_THREAD_P)
> 
> Would a suitable __glibc_unlikely make sense here?  I can't convince
> myself that either of these two cases would be rare enough to move to
> cold sections, but I posit the question anyway...

SINGLE_THREAD_P macro on Linux already uses __glibc_likely prioritizing
single thread execution.

> 
>> +	      void *p = chunk2mem (victim);
>> +	      alloc_perturb (p, bytes);
>> +	      return p;
>> +	    }
> 
> This part does not need to be duplicated.  It can be moved outside the
> if (SINGLE_THREAD_P) clause completely.  It adds one jmp if the compiler
> doesn't duplicate it behind the scenes, but that's no reason to have
> duplicate code.
> 
> The overall duplication of code looks... undesirable... but given
> there's a macro in the middle we're changing, I don't see a good way to
> factor it out, unless the SINGLE_THREAD_P test is so cheap we can
> instead rewrite the REMOVE_FB() macro to use it instead of duplicating
> code.  Could/did you consider that?  If the benchmarks show no
> significant difference, I'd prefer cleaner code over an insignificant
> speedup.  Might even call SINGLE_THREAD_P once and store the result in a
> register, to let the compiler optimize it better.
> 
> Remember, "fast at any cost" is not the goal.  "Fast while still
> maintainable" is ;-)

SINGLE_THREAD_P is a memory load either from a global variable or from 
an offset based on TCB (which is faster for the architecture). I think
we can try checking if we can use it on REMOVE_FB first.