git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Junio C Hamano <gitster@pobox.com>,
	"Shawn O. Pearce" <spearce@spearce.org>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: Remove diff machinery dependency from read-cache
Date: Thu, 21 Jan 2010 14:18:34 -0800 (PST)	[thread overview]
Message-ID: <alpine.LFD.2.00.1001211355500.13231@localhost.localdomain> (raw)
In-Reply-To: <alpine.LFD.2.00.1001211215080.13231@localhost.localdomain>



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

  reply	other threads:[~2010-01-21 22:20 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-01-21 19:37 Remove diff machinery dependency from read-cache Linus Torvalds
2010-01-21 20:07 ` Junio C Hamano
2010-01-21 20:15   ` Linus Torvalds
2010-01-21 22:18     ` Linus Torvalds [this message]
2010-01-21 23:25       ` Linus Torvalds
2010-01-22  0:45         ` Junio C Hamano
2010-01-22  0:59           ` Johannes Schindelin
2010-01-22  1:01             ` Junio C Hamano
2010-01-22  1:43               ` Johannes Schindelin
2010-01-22  3:50                 ` Junio C Hamano
2010-01-22  2:20           ` Linus Torvalds
2010-01-22  2:25             ` Linus Torvalds
2010-01-22  3:50               ` Linus Torvalds
2010-01-22  8:43               ` Johannes Sixt
2010-01-22 11:47                 ` [PATCH] merge-tree: remove unnecessary call of git_extract_argv0_path Johannes Sixt
2010-01-22 16:40                   ` Linus Torvalds
2010-01-22  2:35           ` Remove diff machinery dependency from read-cache Nicolas Pitre
2010-01-22  2:44             ` Linus Torvalds
2010-01-22  3:56             ` Junio C Hamano
2010-01-22  4:21               ` Linus Torvalds
2010-01-22  4:31             ` Linus Torvalds
2010-01-22  3:58 ` Linus Torvalds
2010-01-23  6:31 ` Brian Campbell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LFD.2.00.1001211355500.13231@localhost.localdomain \
    --to=torvalds@linux-foundation.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=spearce@spearce.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).