{"thread":{"id":"49695","subject":"[PATCH 3/3] commit-reach.h: add missing declarations (hdr-check)","startedAt":"2018-10-27T01:54:01Z","lastAt":"2018-10-29T23:59:49Z","messageCount":3,"participants":["Ramsay Jones","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"361664","messageId":"88102e3a-8a02-fca4-4daf-ab428008afc7@ramsayjones.plus.com","threadId":"49695","inReplyTo":null,"subject":"[PATCH 3/3] commit-reach.h: add missing declarations (hdr-check)","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-10-27T01:53:57Z","receivedAt":"2018-10-27T01:54:01Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nAdd the necessary #includes and forward declarations to allow the header\nfile to pass the 'hdr-check' target.\n\nNote that, since this header includes the commit-slab implementation\nheader file (indirectly via commit-slab.h), some of the commit-slab\ninline functions (e.g contains_cache_at_peek()) will not compile without\nthe complete type of 'struct commit'. Hence, we replace the forward\ndeclaration of 'struct commit' with the an #include of the 'commit.h'\nheader file.\n\nIt is possible, using the 'commit-slab-{decl,impl}.h' files, to avoid\nthis inclusion of the 'commit.h' header. Commit a9f1f1f9f8 (\"commit-slab.h:\ncode split\", 2018-05-19) separated the commit-slab interface from its\nimplementation, to allow for the definition of a public commit-slab data\nstructure. This enabled us to avoid including the commit-slab implementation\nin a header file, which could result in the replication of the commit-slab\nfunctions in each compilation unit in which it was included.\n\nIndeed, if you compile with optimizations disabled, then run this script:\n\n  $ cat -n dup-static.sh\n       1 #!/bin/sh\n       2\n       3 nm $1 | grep ' t ' | cut -d' ' -f3 | sort | uniq -c |\n       4 \tsort -rn | grep -v '      1'\n  $\n\n  $ ./dup-static.sh git | grep contains\n       24 init_contains_cache_with_stride\n       24 init_contains_cache\n       24 contains_cache_peek\n       24 contains_cache_at_peek\n       24 contains_cache_at\n       24 clear_contains_cache\n  $\n\nyou will find 24 copies of the commit-slab routines for the contains_cache.\nOf course, when you enable optimizations again, these duplicate static\nfunctions (mostly) disappear. Compiling with gcc at -O2, leaves two static\nfunctions, thus:\n\n  $ nm commit-reach.o | grep contains_cache\n  0000000000000870 t contains_cache_at_peek.isra.1.constprop.6\n  $ nm ref-filter.o | grep contains_cache\n  00000000000002b0 t clear_contains_cache.isra.14\n  $\n\nHowever, using a shared 'contains_cache' would result in all six of the\nabove functions as external public functions in the git binary. At present,\nonly three of these functions are actually called, so the trade-off\nseems to favour letting the compiler inline the commit-slab functions.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n---\n commit-reach.h | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/commit-reach.h b/commit-reach.h\nindex 7d313e2975..f41d8f6ba3 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -1,12 +1,13 @@\n #ifndef __COMMIT_REACH_H__\n #define __COMMIT_REACH_H__\n \n+#include \"commit.h\"\n #include \"commit-slab.h\"\n \n-struct commit;\n struct commit_list;\n-struct contains_cache;\n struct ref_filter;\n+struct object_id;\n+struct object_array;\n \n struct commit_list *get_merge_bases_many(struct commit *one,\n \t\t\t\t\t int n,\n-- \n2.19.0\n"},{"id":"361858","messageId":"xmqq1s89a72a.fsf@gitster-ct.c.googlers.com","threadId":"49695","inReplyTo":"88102e3a-8a02-fca4-4daf-ab428008afc7@ramsayjones.plus.com","subject":"Re: [PATCH 3/3] commit-reach.h: add missing declarations (hdr-check)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-29T01:13:33Z","receivedAt":"2018-10-29T01:13:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> ...\n>        24 clear_contains_cache\n>   $\n>\n> you will find 24 copies of the commit-slab routines for the contains_cache.\n> Of course, when you enable optimizations again, these duplicate static\n> functions (mostly) disappear. Compiling with gcc at -O2, leaves two static\n> functions, thus:\n>\n>   $ nm commit-reach.o | grep contains_cache\n>   0000000000000870 t contains_cache_at_peek.isra.1.constprop.6\n>   $ nm ref-filter.o | grep contains_cache\n>   00000000000002b0 t clear_contains_cache.isra.14\n>   $\n>\n> However, using a shared 'contains_cache' would result in all six of the\n> above functions as external public functions in the git binary.\n\nSorry, but you lost me here.  Where does the 6 in above 'all six'\ncome from?  I saw 24 (from unoptimized), and I saw 2 (from\noptimized), but...\n\nAh, OK, the slab system gives us a familiy of 6 helper functions to\ndeal with the \"contains_cache\" slab, and we call only 3 of them\n(i.e. the implementation of other three are left unused).  Makes\nsense.\n\n> At present,\n> only three of these functions are actually called, so the trade-off\n> seems to favour letting the compiler inline the commit-slab functions.\n\nOK.  I vaguely recall using a linker that can excise the\nimplementation an unused public function out of the resulting\nexecutable in the past, which may tip the balance in the opposite\ndirection, but the above reasonong certainly makes sense to me.\n\n> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>\n> ---\n>  commit-reach.h | 5 +++--\n>  1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/commit-reach.h b/commit-reach.h\n> index 7d313e2975..f41d8f6ba3 100644\n> --- a/commit-reach.h\n> +++ b/commit-reach.h\n> @@ -1,12 +1,13 @@\n>  #ifndef __COMMIT_REACH_H__\n>  #define __COMMIT_REACH_H__\n>  \n> +#include \"commit.h\"\n>  #include \"commit-slab.h\"\n>  \n> -struct commit;\n>  struct commit_list;\n> -struct contains_cache;\n>  struct ref_filter;\n> +struct object_id;\n> +struct object_array;\n>  \n>  struct commit_list *get_merge_bases_many(struct commit *one,\n>  \t\t\t\t\t int n,\n"},{"id":"361952","messageId":"57d29642-ba3b-d11a-2524-701c6f8c709e@ramsayjones.plus.com","threadId":"49695","inReplyTo":"xmqq1s89a72a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/3] commit-reach.h: add missing declarations (hdr-check)","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2018-10-29T23:59:45Z","receivedAt":"2018-10-29T23:59:49Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 29/10/2018 01:13, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>> ...\n>>        24 clear_contains_cache\n>>   $\n>>\n>> you will find 24 copies of the commit-slab routines for the contains_cache.\n>> Of course, when you enable optimizations again, these duplicate static\n>> functions (mostly) disappear. Compiling with gcc at -O2, leaves two static\n>> functions, thus:\n>>\n>>   $ nm commit-reach.o | grep contains_cache\n>>   0000000000000870 t contains_cache_at_peek.isra.1.constprop.6\n>>   $ nm ref-filter.o | grep contains_cache\n>>   00000000000002b0 t clear_contains_cache.isra.14\n>>   $\n>>\n>> However, using a shared 'contains_cache' would result in all six of the\n>> above functions as external public functions in the git binary.\n> \n> Sorry, but you lost me here.  Where does the 6 in above 'all six'\n> come from?  I saw 24 (from unoptimized), and I saw 2 (from\n> optimized), but...\n\nAs you already gathered, the 'six of the above functions' was the\nlist of 6 functions which were each duplicated 24 times (you only\nleft the last one un-snipped above) in the unoptimized git binary.\n\n> Ah, OK, the slab system gives us a familiy of 6 helper functions to\n> deal with the \"contains_cache\" slab, and we call only 3 of them\n> (i.e. the implementation of other three are left unused).  Makes\n> sense.\n\nYep, only clear_contains_cache(), contains_cache_at() and\ninit_contains_cache() are called.\n\n>> At present,\n>> only three of these functions are actually called, so the trade-off\n>> seems to favour letting the compiler inline the commit-slab functions.\n> \n> OK.  I vaguely recall using a linker that can excise the\n> implementation an unused public function out of the resulting\n> executable in the past, which may tip the balance in the opposite\n> direction, \n\nYes, and I thought I was using such a linker - I was surprised that\nseems not to be the case! ;-) [I know I have used such a linker, and\nI could have sworn it was on Linux ... ]\n\nAs I said in [1], the first version of this patch actually used a\nshared contains_cache (so as not to #include \"commit.h\"). I changed\nthat just before sending it out, because I felt the patch conflated\ntwo issues - I fully intended to send a \"let's use a shared contains\ncache instead\" follow up patch! (Again, see [1] for the initial version\nof that follow up patch).\n\nBut then Derrick said he preferred this version of the patch and I\ncouldn't really justify the follow up patch, other than to say \"you\nare making your compiler work harder than it needs to ...\" - not very\nconvincing! :-P\n\nFor example, applying the RFC/PATCH from [1] on top of this patch\nwe can see:\n\n  $ nm git | grep contains_cache\n  00000000000d21f0 T clear_contains_cache\n  00000000000d2400 T contains_cache_at\n  00000000000d2240 T contains_cache_at_peek\n  00000000000d2410 T contains_cache_peek\n  00000000000d21d0 T init_contains_cache\n  00000000000d2190 T init_contains_cache_with_stride\n  $ size git\n     text\t   data\t    bss\t    dec\t    hex\tfilename\n  2531234\t  70736\t 274832\t2876802\t 2be582\tgit\n  $ \n\nwhereas, without that patch on top (ie this patch):\n\n  $ nm git | grep contains_cache\n  0000000000161e30 t clear_contains_cache.isra.14\n  00000000000d2190 t contains_cache_at_peek.isra.1.constprop.6\n  $ size git\n     text\t   data\t    bss\t    dec\t    hex\tfilename\n  2530962\t  70736\t 274832\t2876530\t 2be472\tgit\n  $ \n\nwhich means the 'shared contains_cache' patch leads to a +272 bytes\nof bloat in text section. (from the 3 unused external functions).\n\n\n[1] https://public-inbox.org/git/2809251f-6eba-b0ac-68fe-b972809ccff7@ramsayjones.plus.com/\n\n>             but the above reasonong certainly makes sense to me.\n\nThanks!\n\nATB,\nRamsay Jones\n\n"}]}