Shrink the git binary a bit by avoiding unnecessary inline functions

Shrink the git binary a bit by avoiding unnecessary inline functions

[Linus Torvalds]

So I was looking at the disgusting size of the git binary, and even with 
the debugging removed, and using -Os instead of -O2, the size of the text 
section was pretty high. In this day and age I guess almost a megabyte of 
text isn't really all that surprising, but it still doesn't exactly make 
me think "lean and mean".

With -Os, a surprising amount of text space is wasted on inline functions 
that end up just being replicated multiple times, and where performance 
really isn't a valid reason to inline them. In particular, the trivial 
wrapper functions like "xmalloc()" are used _everywhere_, and making them 
inline just duplicates the text (and the string we use to 'die()' on 
failure) unnecessarily.

So this just moves them into a "wrapper.c" file, getting rid of a tiny bit 
of unnecessary bloat. The following numbers are both with "CFLAGS=-Os":

Before:
	[torvalds@woody git]$ size git
	   text	   data	    bss	    dec	    hex	filename
	 700460	  15160	 292184	1007804	  f60bc	git

After:
	[torvalds@woody git]$ size git
	   text	   data	    bss	    dec	    hex	filename
	 670540	  15160	 292184	 977884	  eebdc	git

so it saves almost 30k of text-space (it actually saves more than that 
with the default -O2, but I don't think that's necessarily a very relevant 
number from a "try to shrink git" standpoint).

It might conceivably have a performance impact, but none of this should be 
_that_ performance critical. The real cost is not generally in the wrapper 
anyway, but in the code it wraps (ie the cost of "xread()" is all in the 
read itself, not in the trivial wrapping of it).

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

There are probably other things we should do, but this was the obvious 
low-hanging fruit.

NOTE! When looking at the actual size of the binary on-disk, all the size 
issues are dwarfed by the cost of teh debug information, so don't be 
fooled by doing an "ls -l" unless you strip it first, or remove the "-g". 

If you just do a "ls -l git" with the default CFLAGS options, you'll think 
that the git binary is 3+MB in size:

	$ ls -lh git
	-rwxrwxr-x 88 torvalds torvalds 3.4M 2008-06-22 12:14 git

but that's mostly debug and symbol info:

	$ strip git
	$ ls -lh git
	-rwxrwxr-x 88 torvalds torvalds 811K 2008-06-22 12:15 git

so it's not wuite *that* scary. With -Os (this patch applied in all 
cases), it shrinks to

	$ ls -lh git
	-rwxrwxr-x 88 torvalds torvalds 2.9M 2008-06-22 12:17 git

	$ strip git
	$ ls -lh git
	-rwxrwxr-x 88 torvalds torvalds 680K 2008-06-22 12:18 git

respectively.

 Makefile          |    1 +
 git-compat-util.h |  167 ++++-------------------------------------------------
 wrapper.c         |  160 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+), 155 deletions(-)

diff --git a/Makefile b/Makefile
index 6a31c9f..408ec33 100644
--- a/Makefile
+++ b/Makefile
@@ -467,6 +467,7 @@ LIB_OBJS += unpack-trees.o
 LIB_OBJS += usage.o
 LIB_OBJS += utf8.o
 LIB_OBJS += walker.o
+LIB_OBJS += wrapper.o
 LIB_OBJS += write_or_die.o
 LIB_OBJS += ws.o
 LIB_OBJS += wt-status.o
diff --git a/git-compat-util.h b/git-compat-util.h
index c04e8ba..6f94a81 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -240,161 +240,18 @@ static inline char *gitstrchrnul(const char *s, int c)
 
 extern void release_pack_memory(size_t, int);
 
-static inline char* xstrdup(const char *str)
-{
-	char *ret = strdup(str);
-	if (!ret) {
-		release_pack_memory(strlen(str) + 1, -1);
-		ret = strdup(str);
-		if (!ret)
-			die("Out of memory, strdup failed");
-	}
-	return ret;
-}
-
-static inline void *xmalloc(size_t size)
-{
-	void *ret = malloc(size);
-	if (!ret && !size)
-		ret = malloc(1);
-	if (!ret) {
-		release_pack_memory(size, -1);
-		ret = malloc(size);
-		if (!ret && !size)
-			ret = malloc(1);
-		if (!ret)
-			die("Out of memory, malloc failed");
-	}
-#ifdef XMALLOC_POISON
-	memset(ret, 0xA5, size);
-#endif
-	return ret;
-}
-
-/*
- * xmemdupz() allocates (len + 1) bytes of memory, duplicates "len" bytes of
- * "data" to the allocated memory, zero terminates the allocated memory,
- * and returns a pointer to the allocated memory. If the allocation fails,
- * the program dies.
- */
-static inline void *xmemdupz(const void *data, size_t len)
-{
-	char *p = xmalloc(len + 1);
-	memcpy(p, data, len);
-	p[len] = '\0';
-	return p;
-}
-
-static inline char *xstrndup(const char *str, size_t len)
-{
-	char *p = memchr(str, '\0', len);
-	return xmemdupz(str, p ? p - str : len);
-}
-
-static inline void *xrealloc(void *ptr, size_t size)
-{
-	void *ret = realloc(ptr, size);
-	if (!ret && !size)
-		ret = realloc(ptr, 1);
-	if (!ret) {
-		release_pack_memory(size, -1);
-		ret = realloc(ptr, size);
-		if (!ret && !size)
-			ret = realloc(ptr, 1);
-		if (!ret)
-			die("Out of memory, realloc failed");
-	}
-	return ret;
-}
-
-static inline void *xcalloc(size_t nmemb, size_t size)
-{
-	void *ret = calloc(nmemb, size);
-	if (!ret && (!nmemb || !size))
-		ret = calloc(1, 1);
-	if (!ret) {
-		release_pack_memory(nmemb * size, -1);
-		ret = calloc(nmemb, size);
-		if (!ret && (!nmemb || !size))
-			ret = calloc(1, 1);
-		if (!ret)
-			die("Out of memory, calloc failed");
-	}
-	return ret;
-}
-
-static inline void *xmmap(void *start, size_t length,
-	int prot, int flags, int fd, off_t offset)
-{
-	void *ret = mmap(start, length, prot, flags, fd, offset);
-	if (ret == MAP_FAILED) {
-		if (!length)
-			return NULL;
-		release_pack_memory(length, fd);
-		ret = mmap(start, length, prot, flags, fd, offset);
-		if (ret == MAP_FAILED)
-			die("Out of memory? mmap failed: %s", strerror(errno));
-	}
-	return ret;
-}
-
-/*
- * xread() is the same a read(), but it automatically restarts read()
- * operations with a recoverable error (EAGAIN and EINTR). xread()
- * DOES NOT GUARANTEE that "len" bytes is read even if the data is available.
- */
-static inline ssize_t xread(int fd, void *buf, size_t len)
-{
-	ssize_t nr;
-	while (1) {
-		nr = read(fd, buf, len);
-		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
-			continue;
-		return nr;
-	}
-}
-
-/*
- * xwrite() is the same a write(), but it automatically restarts write()
- * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
- * GUARANTEE that "len" bytes is written even if the operation is successful.
- */
-static inline ssize_t xwrite(int fd, const void *buf, size_t len)
-{
-	ssize_t nr;
-	while (1) {
-		nr = write(fd, buf, len);
-		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
-			continue;
-		return nr;
-	}
-}
-
-static inline int xdup(int fd)
-{
-	int ret = dup(fd);
-	if (ret < 0)
-		die("dup failed: %s", strerror(errno));
-	return ret;
-}
-
-static inline FILE *xfdopen(int fd, const char *mode)
-{
-	FILE *stream = fdopen(fd, mode);
-	if (stream == NULL)
-		die("Out of memory? fdopen failed: %s", strerror(errno));
-	return stream;
-}
-
-static inline int xmkstemp(char *template)
-{
-	int fd;
-
-	fd = mkstemp(template);
-	if (fd < 0)
-		die("Unable to create temporary file: %s", strerror(errno));
-	return fd;
-}
+extern char *xstrdup(const char *str);
+extern void *xmalloc(size_t size);
+extern void *xmemdupz(const void *data, size_t len);
+extern char *xstrndup(const char *str, size_t len);
+extern void *xrealloc(void *ptr, size_t size);
+extern void *xcalloc(size_t nmemb, size_t size);
+extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
+extern ssize_t xread(int fd, void *buf, size_t len);
+extern ssize_t xwrite(int fd, const void *buf, size_t len);
+extern int xdup(int fd);
+extern FILE *xfdopen(int fd, const char *mode);
+extern int xmkstemp(char *template);
 
 static inline size_t xsize_t(off_t len)
 {
diff --git a/wrapper.c b/wrapper.c
new file mode 100644
index 0000000..4e04f76
--- /dev/null
+++ b/wrapper.c
@@ -0,0 +1,160 @@
+/*
+ * Various trivial helper wrappers around standard functions
+ */
+#include "cache.h"
+
+char *xstrdup(const char *str)
+{
+	char *ret = strdup(str);
+	if (!ret) {
+		release_pack_memory(strlen(str) + 1, -1);
+		ret = strdup(str);
+		if (!ret)
+			die("Out of memory, strdup failed");
+	}
+	return ret;
+}
+
+void *xmalloc(size_t size)
+{
+	void *ret = malloc(size);
+	if (!ret && !size)
+		ret = malloc(1);
+	if (!ret) {
+		release_pack_memory(size, -1);
+		ret = malloc(size);
+		if (!ret && !size)
+			ret = malloc(1);
+		if (!ret)
+			die("Out of memory, malloc failed");
+	}
+#ifdef XMALLOC_POISON
+	memset(ret, 0xA5, size);
+#endif
+	return ret;
+}
+
+/*
+ * xmemdupz() allocates (len + 1) bytes of memory, duplicates "len" bytes of
+ * "data" to the allocated memory, zero terminates the allocated memory,
+ * and returns a pointer to the allocated memory. If the allocation fails,
+ * the program dies.
+ */
+void *xmemdupz(const void *data, size_t len)
+{
+	char *p = xmalloc(len + 1);
+	memcpy(p, data, len);
+	p[len] = '\0';
+	return p;
+}
+
+char *xstrndup(const char *str, size_t len)
+{
+	char *p = memchr(str, '\0', len);
+	return xmemdupz(str, p ? p - str : len);
+}
+
+void *xrealloc(void *ptr, size_t size)
+{
+	void *ret = realloc(ptr, size);
+	if (!ret && !size)
+		ret = realloc(ptr, 1);
+	if (!ret) {
+		release_pack_memory(size, -1);
+		ret = realloc(ptr, size);
+		if (!ret && !size)
+			ret = realloc(ptr, 1);
+		if (!ret)
+			die("Out of memory, realloc failed");
+	}
+	return ret;
+}
+
+void *xcalloc(size_t nmemb, size_t size)
+{
+	void *ret = calloc(nmemb, size);
+	if (!ret && (!nmemb || !size))
+		ret = calloc(1, 1);
+	if (!ret) {
+		release_pack_memory(nmemb * size, -1);
+		ret = calloc(nmemb, size);
+		if (!ret && (!nmemb || !size))
+			ret = calloc(1, 1);
+		if (!ret)
+			die("Out of memory, calloc failed");
+	}
+	return ret;
+}
+
+void *xmmap(void *start, size_t length,
+	int prot, int flags, int fd, off_t offset)
+{
+	void *ret = mmap(start, length, prot, flags, fd, offset);
+	if (ret == MAP_FAILED) {
+		if (!length)
+			return NULL;
+		release_pack_memory(length, fd);
+		ret = mmap(start, length, prot, flags, fd, offset);
+		if (ret == MAP_FAILED)
+			die("Out of memory? mmap failed: %s", strerror(errno));
+	}
+	return ret;
+}
+
+/*
+ * xread() is the same a read(), but it automatically restarts read()
+ * operations with a recoverable error (EAGAIN and EINTR). xread()
+ * DOES NOT GUARANTEE that "len" bytes is read even if the data is available.
+ */
+ssize_t xread(int fd, void *buf, size_t len)
+{
+	ssize_t nr;
+	while (1) {
+		nr = read(fd, buf, len);
+		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
+			continue;
+		return nr;
+	}
+}
+
+/*
+ * xwrite() is the same a write(), but it automatically restarts write()
+ * operations with a recoverable error (EAGAIN and EINTR). xwrite() DOES NOT
+ * GUARANTEE that "len" bytes is written even if the operation is successful.
+ */
+ssize_t xwrite(int fd, const void *buf, size_t len)
+{
+	ssize_t nr;
+	while (1) {
+		nr = write(fd, buf, len);
+		if ((nr < 0) && (errno == EAGAIN || errno == EINTR))
+			continue;
+		return nr;
+	}
+}
+
+int xdup(int fd)
+{
+	int ret = dup(fd);
+	if (ret < 0)
+		die("dup failed: %s", strerror(errno));
+	return ret;
+}
+
+FILE *xfdopen(int fd, const char *mode)
+{
+	FILE *stream = fdopen(fd, mode);
+	if (stream == NULL)
+		die("Out of memory? fdopen failed: %s", strerror(errno));
+	return stream;
+}
+
+int xmkstemp(char *template)
+{
+	int fd;
+
+	fd = mkstemp(template);
+	if (fd < 0)
+		die("Unable to create temporary file: %s", strerror(errno));
+	return fd;
+}

Re: Shrink the git binary a bit by avoiding unnecessary inline functions

[Christian MICHON]

On Sun, Jun 22, 2008 at 9:19 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So I was looking at the disgusting size of the git binary, and even with
> the debugging removed, and using -Os instead of -O2, the size of the text
> section was pretty high. In this day and age I guess almost a megabyte of
> text isn't really all that surprising, but it still doesn't exactly make
> me think "lean and mean".

using gcc-3.4.6 and uclibc-0.9.29 (not exactly everyone's
configuration of course...)
I get different numbers with CFLAGS=-Os and NO_CURL, NO_ICONV on plain
git-1.5.6:

sh-3.2# ls -lh git
-rwxr-xr-x    3 root     root       699.7k Jun 22 23:26 git

sh-3.2# size git
   text    data     bss     dec     hex filename
 616544   10960  272272  899776   dbac0 git

after I use your patch, it goes to:

sh-3.2# ls -lh git
-rwxr-xr-x    1 root     root       652.6k Jun 22 23:30 git

sh-3.2# size git
   text    data     bss     dec     hex filename
 568124   10960  272272  851356   cfd9c git

So your patch obviously works here too but I get quite smaller figures too.

curl and iconv are not available on my distro detaolb, maybe it's a
big difference too...

Could your figures come from recent gcc/glibc versions ?

-- 
Christian
--
http://detaolb.sourceforge.net/, a linux distribution for Qemu with Git inside !

Re: Shrink the git binary a bit by avoiding unnecessary inline functions

[Matthieu Moy]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> -static inline char* xstrdup(const char *str)
> -{
> -	char *ret = strdup(str);
> -	if (!ret) {
> -		release_pack_memory(strlen(str) + 1, -1);
> -		ret = strdup(str);
> -		if (!ret)
> -			die("Out of memory, strdup failed");
> -	}
> -	return ret;
> -}

Wouldn't it be better to split that kind of function into two parts:

- inline the short common case
- put the special case apart

like

static inline char* xstrdup(const char *str)
{
	char *ret = strdup(str);
	if (!ret)
		return xstrdup_failed(str);
	else
		return ret;
}

(with xstrdup_failed not being inline.)

?

-- 
Matthieu

Re: Shrink the git binary a bit by avoiding unnecessary inline functions

[Linus Torvalds]

On Mon, 23 Jun 2008, Matthieu Moy wrote:
> 
> Wouldn't it be better to split that kind of function into two parts:
> 
> - inline the short common case
> - put the special case apart

It's almost certainly not worth it. These things really aren't big 
optimization wins, and the *big* wins from doing inlines are if it allows 
the call-site to do more optimizations (eg conditionals or calculations 
that just go away if inlined with constant arguments, for example), or if 
the lack of any function call at all allows the caller to do better 
register allocation around it.

Since these inlines contain a real call regardless, the second case never 
happens.

The first case can happen with things like "strlen()" being optimized away 
for constant-sized arguments, but for the one user where that was 
(xstrdup()) it really wasn't an issue. And while a constant size argument 
to "xmalloc()" is not unlikely, and would allow the compiler to optimize 
the

	if (!ret && !size)
		ret = malloc(1);

away statically as an inline, it's really not a big win.

		Linus

Re: Shrink the git binary a bit by avoiding unnecessary inline functions

[Christian MICHON]

On Sun, Jun 22, 2008 at 9:32 PM, Christian MICHON
<christian.michon@gmail.com> wrote:
> using gcc-3.4.6 and uclibc-0.9.29 (not exactly everyone's
> configuration of course...)
> I get different numbers with CFLAGS=-Os and NO_CURL, NO_ICONV on plain
> git-1.5.6:
>
> sh-3.2# ls -lh git
> -rwxr-xr-x    3 root     root       699.7k Jun 22 23:26 git
>
> sh-3.2# size git
>   text    data     bss     dec     hex filename
>  616544   10960  272272  899776   dbac0 git
>
> after I use your patch, it goes to:
>
> sh-3.2# ls -lh git
> -rwxr-xr-x    1 root     root       652.6k Jun 22 23:30 git
>
> sh-3.2# size git
>   text    data     bss     dec     hex filename
>  568124   10960  272272  851356   cfd9c git
>
> So your patch obviously works here too but I get quite smaller figures too.
>
> curl and iconv are not available on my distro detaolb, maybe it's a
> big difference too...
>
> Could your figures come from recent gcc/glibc versions ?
>

Linus,

I've put my hands on a usb stick with ubuntu (gcc-4.2.3 and
glibc-2.7), containing all packages needed for a pristine compilation
of git-1.5.6.

I only changed CFLAGS to -Os, no debug info compiled.

ubuntu@ubuntu:~/Desktop/git-1.5.6$ ls -lh git
-rwxr-xr-x 1 ubuntu ubuntu 718K 2008-06-23 00:47 git

ubuntu@ubuntu:~/Desktop/git-1.5.6$ size git
   text	   data	    bss	    dec	    hex	filename
 616606	   9876	 270812	 897294	  db10e	git

There's more than 80k difference with your figures (I have not even
applied your patch yet).
May I ask which version of gcc you're using ? The numbers you provided
were really with -Os ?

Once your patch is applied, these are the final figures:

ubuntu@ubuntu:~/Desktop/git-1.5.6$ ls -lh git
-rwxr-xr-x 1 ubuntu ubuntu 692K 2008-06-23 00:59 git

ubuntu@ubuntu:~/Desktop/git-1.5.6$ size git
   text	   data	    bss	    dec	    hex	filename
 591554	   9876	 270812	 872242	  d4f32	git

-- 
Christian
--
http://detaolb.sourceforge.net/, a linux distribution for Qemu with Git inside !

Re: Shrink the git binary a bit by avoiding unnecessary inline functions

[Linus Torvalds]

On Mon, 23 Jun 2008, Christian MICHON wrote:
> 
> There's more than 80k difference with your figures (I have not even
> applied your patch yet).
> May I ask which version of gcc you're using ? The numbers you provided
> were really with -Os ?

gcc version 4.3.0 20080428 (Red Hat 4.3.0-8) (GCC) 

Also notice that this is x86-64, maybe you're 32-bit? 

You also have much smaller size difference (616606 -> 591554 is just over 
16kB), but if my version of gcc bloats things up more, then I'm not 
surprised that the difference is bigger too.

			Linus

Re: Shrink the git binary a bit by avoiding unnecessary inline functions

[Christian MICHON]

On Mon, Jun 23, 2008 at 6:54 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> gcc version 4.3.0 20080428 (Red Hat 4.3.0-8) (GCC)
>
> Also notice that this is x86-64, maybe you're 32-bit?

it is an AMD 64, but the ubuntu and detaolb are indeed 32 bits distros.

>
> You also have much smaller size difference (616606 -> 591554 is just over
> 16kB), but if my version of gcc bloats things up more, then I'm not
> surprised that the difference is bigger too.
>

I guess I should compare results between compiler versions too...

Thanks for the explainations.

-- 
Christian
--
http://detaolb.sourceforge.net/, a linux distribution for Qemu with Git inside !