{"thread":{"id":"19260","subject":"[PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'","startedAt":"2009-05-09T22:09:54Z","lastAt":"2009-05-10T18:40:53Z","messageCount":6,"participants":["Linus Torvalds","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"113427","messageId":"alpine.LFD.2.01.0905091501460.3586@localhost.localdomain","threadId":"19260","inReplyTo":null,"subject":"[PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-05-09T22:09:54Z","receivedAt":"2009-05-09T22:09:54Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds \nDate: Sat, 9 May 2009 14:57:30 -0700\n\nWhen we ask get_stat_data() to get the mode and size of an index entry,\nwe can avoid the lstat() call if we have marked the index entry as being\nuptodate due to earlier lstat() calls.\n\nThis avoids a lot of unnecessary lstat() calls in eg 'git checkout',\nwhere the last phase shows the differences to the working tree\n(requiring a diff), but earlier phases have already verified the index.\n\nOn the kernel repo (with a fast machine and everything cached), this \nchanges timings of a nul 'git checkout' from\n\n - Before (best of ten):\n\n\t0.14user 0.05system 0:00.19elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k\n\t0inputs+0outputs (0major+13237minor)pagefaults 0swaps\n\n - After \n\t0.11user 0.03system 0:00.15elapsed 98%CPU (0avgtext+0avgdata 0maxresident)k\n\t0inputs+0outputs (0major+13235minor)pagefaults 0swaps\n\nso it can obviously be noticeable, although equally obviously it's not a \nshow-stopper on this particular machine. The difference is likely larger \non slower machines, or with operating systems that don't do as good a job \nof name caching.\n\nSigned-off-by: Linus Torvalds \n---\nI sent this as part of the \"make 'git checkout' preload the index\" patch, \nbut since the preloading was of somewhat dubious value, and this part of \nit is not, I'll just send this one-liner as an \"obvious performance fix\".\n\n diff-lib.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex a310fb2..0aba6cd 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -214,7 +214,7 @@ static int get_stat_data(struct cache_entry *ce,\n \tconst unsigned char *sha1 = ce->sha1;\n \tunsigned int mode = ce->ce_mode;\n \n-\tif (!cached) {\n+\tif (!cached && !ce_uptodate(ce)) {\n \t\tint changed;\n \t\tstruct stat st;\n \t\tchanged = check_removed(ce, &st);\n-- \n1.6.3\n"},{"id":"113428","messageId":"alpine.LFD.2.01.0905091517000.3586@localhost.localdomain","threadId":"19260","inReplyTo":"alpine.LFD.2.01.0905091501460.3586@localhost.localdomain","subject":"[PATCH] Teach 'git checkout' to preload the index contents","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-05-09T22:41:17Z","receivedAt":"2009-05-09T22:41:17Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nFrom: Linus Torvalds \nDate: Sat, 9 May 2009 15:11:17 -0700\n\nThis makes git checkout know to use the threaded index preloading if it\nis enabled in the config file. You need to have\n\n\t[core]\n\t\tpreloadindex = true\n\nin your config file to see it, and for that feature to make sense your\nfilesystem needs to be able to do concurrent 'lstat()' lookups, but when\nthat is the case (especially NFS over a high-latency network), this can\nbe a noticeable performance win.\n\nBut with a low-latency network and at least older Linux NFS clients, this \nwill clearly potentially cause a lot of lock contention. It may still \nspeed up the uncached case, but the threading and locking overhead will \nresult in the cached case likely slowing down.\n\nThat was almost certainly fixed by Linux commit fc0f684c2 (\"NFS: Remove \nBKL from NFS lookup code\"), but that one got merged into 2.6.27-rc1, so \nolder kernel versions than 2.6.27 will not scale very well.\n\nBut regardless, it's the right thing to do. If your filesystem doesn't \nscale, don't enable index preloading. \n\nSigned-off-by: Linus Torvalds \n---\n\nOn local filesystems, I've actually seen signs of tiny speedups (iow, \n0.15s -> 0.14s in the cached case on a much-too-fast-for-this-to-matter \nNehalem machine) due to SMP scaling.\n\nBut it's probably more likely to slow things down, unless you have a disk \nthat does TCQ and really gets improved by having multiple outstanding \nrequests.\n\nAgain, this is not so much a \"git checkout\" issue, as a generic \n\"core.preloadindex\" issue.\n\n builtin-checkout.c | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 15f0c32..3100ccd 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -216,7 +216,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n \tnewfd = hold_locked_index(lock_file, 1);\n-\tif (read_cache() < 0)\n+\tif (read_cache_preload(pathspec) < 0)\n \t\treturn error(\"corrupt index file\");\n \n \tif (source_tree)\n@@ -367,7 +367,7 @@ static int merge_working_tree(struct checkout_opts *opts,\n \tint newfd = hold_locked_index(lock_file, 1);\n \tint reprime_cache_tree = 0;\n \n-\tif (read_cache() < 0)\n+\tif (read_cache_preload(NULL) < 0)\n \t\treturn error(\"corrupt index file\");\n \n \tcache_tree_free(&active_cache_tree);\n-- \n1.6.3.1.g3f31\n"},{"id":"113441","messageId":"7v8wl5613c.fsf@alter.siamese.dyndns.org","threadId":"19260","inReplyTo":"alpine.LFD.2.01.0905091501460.3586@localhost.localdomain","subject":"Re: [PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-10T04:20:07Z","receivedAt":"2009-05-10T04:20:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> From: Linus Torvalds \n> Date: Sat, 9 May 2009 14:57:30 -0700\n>\n> When we ask get_stat_data() to get the mode and size of an index entry,\n> ...\n> Signed-off-by: Linus Torvalds \n> ---\n> I sent this as part of the \"make 'git checkout' preload the index\" patch, \n> but since the preloading was of somewhat dubious value, and this part of \n> it is not, I'll just send this one-liner as an \"obvious performance fix\".\n\nOops.\n\nI have been processing the patches in my \"after 1.6.3\" mailbox from the rc\nfreeze period and have already queued this one, but the re-integration of\nfour branches is taking a bit longer than usual. It takes too much time\nto run tests (and as a part of my build procedure I do docs, too) for all\nintegration branches, and until they all pass tests on Debian5 (primary\ndevelopment box) and Fedora9 (at k.org) I do not push the result out, so\nit is a bit painful for me to replace a patch once a day's testing cycle\nbegins.\n\nI was about ready to push the whole thing out, but with this much improved\ncommit log message, it is _very tempting_ to rewind what I queued and redo\ntoday's cycle.\n\nI'll see how many other such (re)-sends I've got; most likely I'll redo\ntoday's cycle after replacing what I earlier have queued with this and\nyour 'checkout' one, and also a patch from Dave Olszewski I want to\nreplace with the 'real' one.\n\nThanks.\n"},{"id":"113470","messageId":"alpine.LFD.2.01.0905100943370.3586@localhost.localdomain","threadId":"19260","inReplyTo":"7v8wl5613c.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2009-05-10T16:50:41Z","receivedAt":"2009-05-10T16:50:41Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Sat, 9 May 2009, Junio C Hamano wrote:\n>\n> I have been processing the patches in my \"after 1.6.3\" mailbox from the rc\n> freeze period and have already queued this one, but the re-integration of\n> four branches is taking a bit longer than usual. It takes too much time\n> to run tests (and as a part of my build procedure I do docs, too) for all\n> integration branches, and until they all pass tests on Debian5 (primary\n> development box) and Fedora9 (at k.org) I do not push the result out, so\n> it is a bit painful for me to replace a patch once a day's testing cycle\n> begins.\n\nHeh.\n\nI can attest from personal experience that if some test-sequence takes a \ncouple of hours, and keeps you from making sane changes from the source, \nyou'll eventually go mad. It was one of the reasons I absolutely hated \nworking with CVS at transmeta - not only did the pre-checking tests take \nforever, but you effectively couldn't do anything else while they were \nrunning due to the whole \"branches don't work\".\n\nNow, at least you don't have _that_ issue, but if testing keeps you from \ndoing the sane thing, you'll still end up having some of the same things \nhappen.\n\nI've found that \"make -j64 test\" does fairly well, bringing the cost down \nto something reasonable. Some of the SVN tests seem to sometimes randomly \nfail when done in parallel (I've not tried to debug it, I assume it's \neither some SVN bug, or just the test infrastructure having some shared \nSVN central repo thing), but it happens rarely enough that even if you \nhave to run it twice, it's still worth it. \n\n[ Side note: the success output of \"make test\" makes it almost impossible \n to debug the error cases when you do that \"make -j64\" thing. Sad. It \n would be good to have the tests that fail clearly say exactly what test \n failed, because when you run 64 tests at the same time, having \"case 9\" \n fail is almost totally useless information - test 9 of _which_ \n testsuite? ]\n\nI don't generally build docs, but they should run in parallel too, and at \nleast your fedora build on kernel.org has a nice quad-core machine with \nlots of memory, so \"-j8\" or something is reasonable.\n\n\t\t\t\tLinus\n"},{"id":"113472","messageId":"alpine.DEB.1.00.0905101942250.27348@pacific.mpi-cbg.de","threadId":"19260","inReplyTo":"alpine.LFD.2.01.0905100943370.3586@localhost.localdomain","subject":"Re: [PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-05-10T17:45:47Z","receivedAt":"2009-05-10T17:45:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 10 May 2009, Linus Torvalds wrote:\n\n> Some of the SVN tests seem to sometimes randomly fail when done in \n> parallel\n\nThis is probably the problem that the SVN \"server\" is not set up \nper-testcase, but globally. (I am speaking from memory here, it might be \npossible that this was fixed already.)\n\n> [ Side note: the success output of \"make test\" makes it almost impossible \n> to debug the error cases when you do that \"make -j64\" thing.\n\nThere are left-over trash directories. (Another indicator are the files \nin t/test-results/.)\n\nMy common workflow is to \"make -j50 && make -j50 -k test\", and if it \nfails, run only those tests that have left-over trash directories.\n\nCiao,\nDscho\n"},{"id":"113473","messageId":"7v7i0oztqi.fsf@alter.siamese.dyndns.org","threadId":"19260","inReplyTo":"alpine.LFD.2.01.0905100943370.3586@localhost.localdomain","subject":"Re: [PATCH] Avoid unnecessary 'lstat()' calls in 'get_stat_data()'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-05-10T18:40:53Z","receivedAt":"2009-05-10T18:40:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> I've found that \"make -j64 test\" does fairly well, bringing the cost down \n> to something reasonable. Some of the SVN tests seem to sometimes randomly \n> fail when done in parallel (I've not tried to debug it, I assume it's \n> either some SVN bug, or just the test infrastructure having some shared \n> SVN central repo thing), but it happens rarely enough that even if you \n> have to run it twice, it's still worth it. \n>\n> [ Side note: the success output of \"make test\" makes it almost impossible \n> to debug the error cases when you do that \"make -j64\" thing. Sad. It \n> would be good to have the tests that fail clearly say exactly what test \n> failed, because when you run 64 tests at the same time, having \"case 9\" \n> fail is almost totally useless information - test 9 of _which_ \n> testsuite? ]\n>\n> I don't generally build docs, but they should run in parallel too, and at \n> least your fedora build on kernel.org has a nice quad-core machine with \n> lots of memory, so \"-j8\" or something is reasonable.\n\nThanks; unfortunately I already do all the tricks known to me, including\nrunning make in parallel (I happen to use -j4) and maintaining a separate\nbuild farm for each of the branches to avoid recompilation of programs and\nreformatting docs. However, on a\n\n model name : AMD Athlon(tm) 64 X2 Dual Core Processor 3800+\n stepping : 1\n cpu MHz : 2004.185\n cache size : 512 KB\n\nwith slow IDE disks, optimizing and cheating at the software level goes\nonly so far... For example, a typical cycle I just did looks like this:\n\n$ /usr/bin/time Meta/Dothem --pedantic >:all.log 2>&1; tail -n 2 :all.log\n2089.15user 1610.16system 45:56.91elapsed 134%CPU (0avgtext+0avgdata 0maxresident)k\n220088inputs+3723296outputs (696major+305868883minor)pagefaults 0swaps\n\nYou have to remember that people use boxes that are a bit slower compared\nto the boxes you are used to ;-)\n"}]}