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.
Thanks!
ATB,
Ramsay Jones
