Web lists-archives.com

Re: [PATCH 3/3] commit-reach.h: add missing declarations (hdr-check)

On 29/10/2018 01:13, Junio C Hamano wrote:
> Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx> writes:
>> ...
>>        24 clear_contains_cache
>>   $
>> you will find 24 copies of the commit-slab routines for the contains_cache.
>> Of course, when you enable optimizations again, these duplicate static
>> functions (mostly) disappear. Compiling with gcc at -O2, leaves two static
>> functions, thus:
>>   $ nm commit-reach.o | grep contains_cache
>>   0000000000000870 t contains_cache_at_peek.isra.1.constprop.6
>>   $ nm ref-filter.o | grep contains_cache
>>   00000000000002b0 t clear_contains_cache.isra.14
>>   $
>> However, using a shared 'contains_cache' would result in all six of the
>> above functions as external public functions in the git binary.
> Sorry, but you lost me here.  Where does the 6 in above 'all six'
> come from?  I saw 24 (from unoptimized), and I saw 2 (from
> optimized), but...

As you already gathered, the 'six of the above functions' was the
list of 6 functions which were each duplicated 24 times (you only
left the last one un-snipped above) in the unoptimized git binary.

> Ah, OK, the slab system gives us a familiy of 6 helper functions to
> deal with the "contains_cache" slab, and we call only 3 of them
> (i.e. the implementation of other three are left unused).  Makes
> sense.

Yep, only clear_contains_cache(), contains_cache_at() and
init_contains_cache() are called.

>> At present,
>> only three of these functions are actually called, so the trade-off
>> seems to favour letting the compiler inline the commit-slab functions.
> OK.  I vaguely recall using a linker that can excise the
> implementation an unused public function out of the resulting
> executable in the past, which may tip the balance in the opposite
> direction, 

Yes, and I thought I was using such a linker - I was surprised that
seems not to be the case! ;-) [I know I have used such a linker, and
I could have sworn it was on Linux ... ]

As I said in [1], the first version of this patch actually used a
shared contains_cache (so as not to #include "commit.h"). I changed
that just before sending it out, because I felt the patch conflated
two issues - I fully intended to send a "let's use a shared contains
cache instead" follow up patch! (Again, see [1] for the initial version
of that follow up patch).

But then Derrick said he preferred this version of the patch and I
couldn't really justify the follow up patch, other than to say "you
are making your compiler work harder than it needs to ..." - not very
convincing! :-P

For example, applying the RFC/PATCH from [1] on top of this patch
we can see:

  $ nm git | grep contains_cache
  00000000000d21f0 T clear_contains_cache
  00000000000d2400 T contains_cache_at
  00000000000d2240 T contains_cache_at_peek
  00000000000d2410 T contains_cache_peek
  00000000000d21d0 T init_contains_cache
  00000000000d2190 T init_contains_cache_with_stride
  $ size git
     text	   data	    bss	    dec	    hex	filename
  2531234	  70736	 274832	2876802	 2be582	git

whereas, without that patch on top (ie this patch):

  $ nm git | grep contains_cache
  0000000000161e30 t clear_contains_cache.isra.14
  00000000000d2190 t contains_cache_at_peek.isra.1.constprop.6
  $ size git
     text	   data	    bss	    dec	    hex	filename
  2530962	  70736	 274832	2876530	 2be472	git

which means the 'shared contains_cache' patch leads to a +272 bytes
of bloat in text section. (from the 3 unused external functions).

[1] https://public-inbox.org/git/2809251f-6eba-b0ac-68fe-b972809ccff7@xxxxxxxxxxxxxxxxxxxx/

>             but the above reasonong certainly makes sense to me.


Ramsay Jones