Re: Remove diff machinery dependency from read-cache

Linus Torvalds [off-list ref] writes:

This trivial cleanup results in pretty stunning file size differences. 
The diff machinery really is mostly used by just the builtin programs, and 
you have things like these trivial before-and-after numbers:

  -rwxr-xr-x 1 torvalds torvalds 1727420 2010-01-21 10:53 git-hash-object
  -rwxrwxr-x 1 torvalds torvalds  940265 2010-01-21 11:16 git-hash-object

Now, I'm not saying that 940kB is good either, but that's mostly all the 
debug information - you can see the real code with 'size':

   text	   data	    bss	    dec	    hex	filename
 418675	   3920	 127408	 550003	  86473	git-hash-object (before)
 230650	   2288	 111728	 344666	  5425a	git-hash-object (after)

ie we have a nice 24% size reduction from this trivial cleanup.

The patch itself to move add_files_to_cache() to builtin-add.c (or to its
own file) makes sense from the code placement POV, but if the goal is to
shrink the on-disk footprint, isn't an alternative approach be to make
hash-object built-in?  You can lose the whole 1.7M from the filesystem
footprint that way, no?

On Thu, 21 Jan 2010, Junio C Hamano wrote:

The patch itself to move add_files_to_cache() to builtin-add.c (or to its
own file) makes sense from the code placement POV, but if the goal is to
shrink the on-disk footprint, isn't an alternative approach be to make
hash-object built-in?  You can lose the whole 1.7M from the filesystem
footprint that way, no?

Sure. Except, as I mentioned, it's not just git-hash-object. It's _all_ of 
them.

The total space savings wasn't 1.7M, it was 12M.

		Linus

On Thu, 21 Jan 2010, Linus Torvalds wrote:

Sure. Except, as I mentioned, it's not just git-hash-object. It's _all_ of 
them.

The total space savings wasn't 1.7M, it was 12M.

There are some other interesting cases. For example, look at this:

	[torvalds@nehalem git]$ size show-index.o git-show-index
	   text	   data	    bss	    dec	    hex	filename
	   1310	      0	   1024	   2334	    91e	show-index.o
	 222706	   2296	 112720	 337722	  5273a	git-show-index

ie a trivial program like 'show-index' has ballooned to 220kB. Let's look 
at why:

	[torvalds@nehalem git]$ nm show-index.o | grep ' U '
	                 U die
	                 U fread
	                 U free
	                 U printf
	                 U sha1_to_hex
	                 U stdin
	                 U usage
	                 U xmalloc

ok, if you ignore standard library things (which will be from a shared 
library anyway), it really only wants totally trivial things: die, 
xmalloc, and sha1_to_hex. Those should be a few hundred bytes, not a few 
hundred _kilo_bytes.

So what happens?

 - sha1_to_hex brings in all of sha1_file.c, even though it doesn't need 
   any of it. Ok, that's easily fixed: split up the hex helpers into a 
   file of its own ("hex.c")

 - "die()" brings in usage.c, which is actually designed correctly, so it 
   is all fine. No extra pain there. Sure, we'll get some other trivial 
   stuff from there, but we're talking maybe a kilobyte of code.

 - "xmalloc()" brings in the trivial wrappers.

   OOPS.

Those wrappers bring in zlib (through git_inflate*), which is not a huge 
issue, we coult just move the git_inflate*() wrappers to its own file. 
Trivial. But the wrappers also bring in:

 - xmalloc/xrealloc/xstrdup:
                 U release_pack_memory

which in turn brings in _all_ of the rest of the git libraries. End 
result: a trivial git helper program that _should_ be a couple of 
kilobytes in size ends up being 200+kB of text, and 900kB with debug 
information.

Absolutely _none_ of which is in the least useful.

Oh well.

We could fix it a few ways

 - ignore it. Most git programs will get the pack handling functions 
   anyway, since they want to get object reading.

 - as mentioned, just build in _everything_ so that we only ever have one 
   binary

 - get rid of release_pack_memory() entirely. We have better ways to limit 
   pack memory use these days, but they do require configuration (we do 
   have a default packed_git_limit, though, so even without any explicit 
   configuration it's not insane).

 - don't have explicit knowledge about 'release_pack_memory' in xmalloc, 
   but instead have the packing functions register a "xmalloc 
   pressure_reliever function". So then programs that have pack handling 
   will register the fixup function, and programs that don't will never 
   even know.

Hmm? We have about 20 external programs that may hide issues like this.

		Linus

On Thu, 21 Jan 2010, Linus Torvalds wrote:

We could fix it a few ways

 - ignore it. Most git programs will get the pack handling functions 
   anyway, since they want to get object reading.

In fact, we should probably remove git-show-index. It may have some 
historical significance as a pack-file index debugger, but it has no 
actual redeeming features now, considering that the binary is a megabyte 
of useless crud with debugging info.

However, we do actually use it in t/t5302-pack-index.sh. So in the 
meantime, how about this hacky patch to simply just avoid xmalloc, and 
separating out the trivial hex functions into "hex.o".

This results in

  [torvalds@nehalem git]$ size git-show-index 
       text    data     bss     dec     hex filename
     222818    2276  112688  337782   52776 git-show-index (before)
       5696     624    1264    7584    1da0 git-show-index (after)

which is a whole lot better, no?

(Or make it a built-in, if we actually think we want to carry it along in 
the long run)

		Linus

---
 Makefile     |    1 +
 sha1_file.c  |   66 ----------------------------------------------------------
 show-index.c |    2 +-
 3 files changed, 2 insertions(+), 67 deletions(-)

diff --git a/Makefile b/Makefile
index ad890ec..a041b69 100644
--- a/Makefile
+++ b/Makefile

@@ -559,6 +559,7 @@ LIB_OBJS += graph.o
 LIB_OBJS += grep.o
 LIB_OBJS += hash.o
 LIB_OBJS += help.o
+LIB_OBJS += hex.o
 LIB_OBJS += ident.o
 LIB_OBJS += levenshtein.o
 LIB_OBJS += list-objects.o

diff --git a/sha1_file.c b/sha1_file.c
index 7086760..12478a3 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -35,54 +35,6 @@ static size_t sz_fmt(size_t s) { return s; }
 
 const unsigned char null_sha1[20];
 
-const signed char hexval_table[256] = {
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 00-07 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 08-0f */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 10-17 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 18-1f */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 20-27 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 28-2f */
-	  0,  1,  2,  3,  4,  5,  6,  7,		/* 30-37 */
-	  8,  9, -1, -1, -1, -1, -1, -1,		/* 38-3f */
-	 -1, 10, 11, 12, 13, 14, 15, -1,		/* 40-47 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 48-4f */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 50-57 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 58-5f */
-	 -1, 10, 11, 12, 13, 14, 15, -1,		/* 60-67 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 68-67 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 70-77 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 78-7f */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 80-87 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 88-8f */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 90-97 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* 98-9f */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* a0-a7 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* a8-af */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* b0-b7 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* b8-bf */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* c0-c7 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* c8-cf */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* d0-d7 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* d8-df */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* e0-e7 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* e8-ef */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f0-f7 */
-	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f8-ff */
-};
-
-int get_sha1_hex(const char *hex, unsigned char *sha1)
-{
-	int i;
-	for (i = 0; i < 20; i++) {
-		unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
-		if (val & ~0xff)
-			return -1;
-		*sha1++ = val;
-		hex += 2;
-	}
-	return 0;
-}
-
 static inline int offset_1st_component(const char *path)
 {
 	if (has_dos_drive_prefix(path))

@@ -133,24 +85,6 @@ int safe_create_leading_directories_const(const char *path)
 	return result;
 }
 
-char *sha1_to_hex(const unsigned char *sha1)
-{
-	static int bufno;
-	static char hexbuffer[4][50];
-	static const char hex[] = "0123456789abcdef";
-	char *buffer = hexbuffer[3 & ++bufno], *buf = buffer;
-	int i;
-
-	for (i = 0; i < 20; i++) {
-		unsigned int val = *sha1++;
-		*buf++ = hex[val >> 4];
-		*buf++ = hex[val & 0xf];
-	}
-	*buf = '\0';
-
-	return buffer;
-}
-
 static void fill_sha1_path(char *pathbuf, const unsigned char *sha1)
 {
 	int i;

diff --git a/show-index.c b/show-index.c
index 63f9da5..4c0ac13 100644
--- a/show-index.c
+++ b/show-index.c

@@ -48,7 +48,7 @@ int main(int argc, char **argv)
 			unsigned char sha1[20];
 			uint32_t crc;
 			uint32_t off;
-		} *entries = xmalloc(nr * sizeof(entries[0]));
+		} *entries = malloc(nr * sizeof(entries[0]));
 		for (i = 0; i < nr; i++)
 			if (fread(entries[i].sha1, 20, 1, stdin) != 1)
 				die("unable to read sha1 %u/%u", i, nr);