threads / patch / 19260

patchAvoid unnecessary 'lstat()' calls in 'get_stat_data()'

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

## tl;dr

6 messages between May 9, 2009 and May 10, 2009. Diffs are folded; open one to read it.

replies: 5people: 3as markdown or json

Linus Torvalds· May 9, 2009, 22:09 UTC · lore
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(-)
Show changes to diff-lib.c +1 −1
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
Linus Torvalds· May 9, 2009, 22:41 UTC · re: Linus Torvalds · lore

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

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(-)
Show changes to builtin-checkout.c +2 −2
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
Junio C Hamano· May 10, 2009, 04:20 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 10 quoted lines
> 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.
Linus Torvalds· May 10, 2009, 16:50 UTC · re: Junio C Hamano · lore

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

On Sat, 9 May 2009, Junio C Hamano wrote:
Show 9 quoted lines
>
> 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
Johannes Schindelin· May 10, 2009, 17:45 UTC · re: Linus Torvalds · lore

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

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

Junio C Hamano· May 10, 2009, 18:40 UTC · re: Linus Torvalds · lore

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

Linus Torvalds <torvalds@linux-foundation.org> writes:
Show 17 quoted lines
> 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 ;-)

← back to recent threads