[PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'

[PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 9 May 2009 14:57:30 -0700

When we ask get_stat_data() to get the mode and size of an index entry,
we can avoid the lstat() call if we have marked the index entry as being
uptodate due to earlier lstat() calls.

This avoids a lot of unnecessary lstat() calls in eg 'git checkout',
where the last phase shows the differences to the working tree
(requiring a diff), but earlier phases have already verified the index.

On the kernel repo (with a fast machine and everything cached), this 
changes timings of a nul 'git checkout' from

 - Before (best of ten):

	0.14user 0.05system 0:00.19elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
	0inputs+0outputs (0major+13237minor)pagefaults 0swaps

 - After 
	0.11user 0.03system 0:00.15elapsed 98%CPU (0avgtext+0avgdata 0maxresident)k
	0inputs+0outputs (0major+13235minor)pagefaults 0swaps

so it can obviously be noticeable, although equally obviously it's not a 
show-stopper on this particular machine. The difference is likely larger 
on slower machines, or with operating systems that don't do as good a job 
of name caching.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
I sent this as part of the "make 'git checkout' preload the index" patch, 
but since the preloading was of somewhat dubious value, and this part of 
it is not, I'll just send this one-liner as an "obvious performance fix".

 diff-lib.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/diff-lib.c b/diff-lib.c
index a310fb2..0aba6cd 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -214,7 +214,7 @@ static int get_stat_data(struct cache_entry *ce,
 	const unsigned char *sha1 = ce->sha1;
 	unsigned int mode = ce->ce_mode;
 
-	if (!cached) {
+	if (!cached && !ce_uptodate(ce)) {
 		int changed;
 		struct stat st;
 		changed = check_removed(ce, &st);
-- 
1.6.3

[PATCH] Teach 'git checkout' to preload the index contents

[Linus Torvalds]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 9 May 2009 15:11:17 -0700

This makes git checkout know to use the threaded index preloading if it
is enabled in the config file. You need to have

	[core]
		preloadindex = true

in your config file to see it, and for that feature to make sense your
filesystem needs to be able to do concurrent 'lstat()' lookups, but when
that is the case (especially NFS over a high-latency network), this can
be a noticeable performance win.

But with a low-latency network and at least older Linux NFS clients, this 
will clearly potentially cause a lot of lock contention. It may still 
speed up the uncached case, but the threading and locking overhead will 
result in the cached case likely slowing down.

That was almost certainly fixed by Linux commit fc0f684c2 ("NFS: Remove 
BKL from NFS lookup code"), but that one got merged into 2.6.27-rc1, so 
older kernel versions than 2.6.27 will not scale very well.

But regardless, it's the right thing to do. If your filesystem doesn't 
scale, don't enable index preloading. 

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

On local filesystems, I've actually seen signs of tiny speedups (iow, 
0.15s -> 0.14s in the cached case on a much-too-fast-for-this-to-matter 
Nehalem machine) due to SMP scaling.

But it's probably more likely to slow things down, unless you have a disk 
that does TCQ and really gets improved by having multiple outstanding 
requests.

Again, this is not so much a "git checkout" issue, as a generic 
"core.preloadindex" issue.

 builtin-checkout.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 15f0c32..3100ccd 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -216,7 +216,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
 	newfd = hold_locked_index(lock_file, 1);
-	if (read_cache() < 0)
+	if (read_cache_preload(pathspec) < 0)
 		return error("corrupt index file");
 
 	if (source_tree)
@@ -367,7 +367,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 	int newfd = hold_locked_index(lock_file, 1);
 	int reprime_cache_tree = 0;
 
-	if (read_cache() < 0)
+	if (read_cache_preload(NULL) < 0)
 		return error("corrupt index file");
 
 	cache_tree_free(&active_cache_tree);
-- 
1.6.3.1.g3f31

Re: [PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'

[Junio C Hamano]

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

> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sat, 9 May 2009 14:57:30 -0700
>
> When we ask get_stat_data() to get the mode and size of an index entry,
> ...
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
> I sent this as part of the "make 'git checkout' preload the index" patch, 
> but since the preloading was of somewhat dubious value, and this part of 
> it is not, I'll just send this one-liner as an "obvious performance fix".

Oops.

I have been processing the patches in my "after 1.6.3" mailbox from the rc
freeze period and have already queued this one, but the re-integration of
four branches is taking a bit longer than usual.  It takes too much time
to run tests (and as a part of my build procedure I do docs, too) for all
integration branches, and until they all pass tests on Debian5 (primary
development box) and Fedora9 (at k.org) I do not push the result out, so
it is a bit painful for me to replace a patch once a day's testing cycle
begins.

I was about ready to push the whole thing out, but with this much improved
commit log message, it is _very tempting_ to rewind what I queued and redo
today's cycle.

I'll see how many other such (re)-sends I've got; most likely I'll redo
today's cycle after replacing what I earlier have queued with this and
your 'checkout' one, and also a patch from Dave Olszewski I want to
replace with the 'real' one.

Thanks.

Re: [PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'

[Linus Torvalds]

On Sat, 9 May 2009, Junio C Hamano wrote:
>
> I have been processing the patches in my "after 1.6.3" mailbox from the rc
> freeze period and have already queued this one, but the re-integration of
> four branches is taking a bit longer than usual.  It takes too much time
> to run tests (and as a part of my build procedure I do docs, too) for all
> integration branches, and until they all pass tests on Debian5 (primary
> development box) and Fedora9 (at k.org) I do not push the result out, so
> it is a bit painful for me to replace a patch once a day's testing cycle
> begins.

Heh.

I can attest from personal experience that if some test-sequence takes a 
couple of hours, and keeps you from making sane changes from the source, 
you'll eventually go mad. It was one of the reasons I absolutely hated 
working with CVS at transmeta - not only did the pre-checking tests take 
forever, but you effectively couldn't do anything else while they were 
running due to the whole "branches don't work".

Now, at least you don't have _that_ issue, but if testing keeps you from 
doing the sane thing, you'll still end up having some of the same things 
happen.

I've found that "make -j64 test" does fairly well, bringing the cost down 
to something reasonable. Some of the SVN tests seem to sometimes randomly 
fail when done in parallel (I've not tried to debug it, I assume it's 
either some SVN bug, or just the test infrastructure having some shared 
SVN central repo thing), but it happens rarely enough that even if you 
have to run it twice, it's still worth it. 

[ Side note: the success output of "make test" makes it almost impossible 
  to debug the error cases when you do that "make -j64" thing. Sad. It 
  would be good to have the tests that fail clearly say exactly what test 
  failed, because when you run 64 tests at the same time, having "case 9" 
  fail is almost totally useless information - test 9 of _which_ 
  testsuite? ]

I don't generally build docs, but they should run in parallel too, and at 
least your fedora build on kernel.org has a nice quad-core machine with 
lots of memory, so "-j8" or something is reasonable.

				Linus

Re: [PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'

[Johannes Schindelin]

Hi,

On Sun, 10 May 2009, Linus Torvalds wrote:

> Some of the SVN tests seem to sometimes randomly fail when done in 
> parallel

This is probably the problem that the SVN "server" is not set up 
per-testcase, but globally.  (I am speaking from memory here, it might be 
possible that this was fixed already.)

> [ Side note: the success output of "make test" makes it almost impossible 
>   to debug the error cases when you do that "make -j64" thing.

There are left-over trash directories.  (Another indicator are the files 
in t/test-results/.)

My common workflow is to "make -j50 && make -j50 -k test", and if it 
fails, run only those tests that have left-over trash directories.

Ciao,
Dscho

Re: [PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'

[Junio C Hamano]

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

> I've found that "make -j64 test" does fairly well, bringing the cost down 
> to something reasonable. Some of the SVN tests seem to sometimes randomly 
> fail when done in parallel (I've not tried to debug it, I assume it's 
> either some SVN bug, or just the test infrastructure having some shared 
> SVN central repo thing), but it happens rarely enough that even if you 
> have to run it twice, it's still worth it. 
>
> [ Side note: the success output of "make test" makes it almost impossible 
>   to debug the error cases when you do that "make -j64" thing. Sad. It 
>   would be good to have the tests that fail clearly say exactly what test 
>   failed, because when you run 64 tests at the same time, having "case 9" 
>   fail is almost totally useless information - test 9 of _which_ 
>   testsuite? ]
>
> I don't generally build docs, but they should run in parallel too, and at 
> least your fedora build on kernel.org has a nice quad-core machine with 
> lots of memory, so "-j8" or something is reasonable.

Thanks; unfortunately I already do all the tricks known to me, including
running make in parallel (I happen to use -j4) and maintaining a separate
build farm for each of the branches to avoid recompilation of programs and
reformatting docs.  However, on a

    model name      : AMD Athlon(tm) 64 X2 Dual Core Processor 3800+
    stepping        : 1
    cpu MHz         : 2004.185
    cache size      : 512 KB

with slow IDE disks, optimizing and cheating at the software level goes
only so far...  For example, a typical cycle I just did looks like this:

$ /usr/bin/time Meta/Dothem --pedantic >:all.log 2>&1; tail -n 2 :all.log
2089.15user 1610.16system 45:56.91elapsed 134%CPU (0avgtext+0avgdata 0maxresident)k
220088inputs+3723296outputs (696major+305868883minor)pagefaults 0swaps

You have to remember that people use boxes that are a bit slower compared
to the boxes you are used to ;-)