threads / discuss / 14087

Shrink the git binary a bit by avoiding unnecessary inline functions

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

## tl;dr

7 messages between Jun 22, 2008 and Jun 23, 2008.

replies: 6people: 3as markdown or json

Linus Torvalds· Jun 22, 2008, 19:19 UTC · lore

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;
+}
Christian MICHON· Jun 22, 2008, 21:32 UTC · re: Linus Torvalds · lore

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

On Sun, Jun 22, 2008 at 9:19 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote:

Show 6 quoted lines
>
> 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 !
Christian MICHON· Jun 23, 2008, 01:01 UTC · re: Christian MICHON · lore

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

On Sun, Jun 22, 2008 at 9:32 PM, Christian MICHON <christian.michon@gmail.com> wrote:

Show 28 quoted lines
> 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 !
Linus Torvalds· Jun 23, 2008, 04:54 UTC · re: Christian MICHON · lore

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

On Mon, 23 Jun 2008, Christian MICHON wrote:
Show 5 quoted lines
> 
> 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
Christian MICHON· Jun 23, 2008, 05:26 UTC · re: Linus Torvalds · lore

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

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.
Show 5 quoted lines
>
> 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 !
Matthieu Moy· Jun 22, 2008, 22:20 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 11 quoted lines
> -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
Linus Torvalds· Jun 22, 2008, 22:40 UTC · re: Matthieu Moy · lore

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

On Mon, 23 Jun 2008, Matthieu Moy wrote:
Show 5 quoted lines
> 
> 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

← back to recent threads