{"thread":{"id":"13990","subject":"'setup_work_tree()' considered harmful","startedAt":"2008-06-17T00:45:25Z","lastAt":"2008-06-18T16:26:18Z","messageCount":5,"participants":["Linus Torvalds","Johannes Schindelin","Mike Hommey"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"80091","messageId":"alpine.LFD.1.10.0806161723081.2949@woody.linux-foundation.org","threadId":"13990","inReplyTo":null,"subject":"'setup_work_tree()' considered harmful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-17T00:45:25Z","receivedAt":"2008-06-17T00:45:25Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n[ Dscho cc'd because I think he is the primary culprits for this thing, I \n  think. Commit e90fdc39b6903502192b2dd11e5503cea721a1ad in particular, \n  methinks. ]\n\nSo because I was looking at system call traces for object creation (due to \nthose \"rename to the final resting place\" patches I did that got merged \nrecently), I've noticed that some commands have *much* worse system call \npatterns than others.\n\nIn particular, doing a \"git add .\" will use absolute pathnames for all git \nfiles, while a \"git diff\" will not. And this is quite noticeable - the \nabsolute pathnames are not just longer, they have more path components in \nthem. Making them a lot slower to look up and use.\n\nOf course, \"a lot\" depends on things a bit, but it really is noticeable.\n\nTo test, I created a kernel tree (no .git), and did a \"git init\" followed \nby a \"git add .\". Here's the timings for current git head:\n\n\t[torvalds@woody kernel]$ time git add .\n\n\treal    0m8.377s\n\tuser    0m6.556s\n\tsys     0m1.656s\n\nAfter that, I fixed \"write_loose_object()\" to not unnecessarily try to \nopen a git object file, because every single caller has already done a \n\"has_sha1_file(sha1)\" or \"has_loose_object(sha1)\" check before calling \nthat function, so trying to open it again is just pointless.\n\nAs a result, git add sped up a tiny bit:\n\n\t[torvalds@woody kernel]$ time ~/git/git-add .\n\n\treal    0m8.341s\n\tuser    0m6.588s\n\tsys     0m1.548s\n\nbut I'll admit that it's not exactly noticeable. Half a percentage point \nis not a big deal.\n\nThen I created a hack that just made \"setup_work_tree()\" a no-op (because \nit does all that crazy stuff that forces GIT_DIR to be an absolute path \netc). As a result I got:\n\n\t[torvalds@woody kernel]$ time ~/git/git-add .\n\n\treal    0m7.849s\n\tuser    0m6.420s\n\tsys     0m1.296s\n\nie now we're talking about a 5%+ performance difference.\n\nOf course, this is all for the hot-cache case, and it wouldn't be \nnoticeable for a cold-cache case, but it really can be a real performance \nissue. And no, it's not that \"setup_work_tree()\" itself is expensive, but \ndepending on how GIT_DIR is set up, you get very different system call \npatterns.\n\nHere's what current git does for one file (sound/usb/usbmixer_maps.c):\n\n\tlstat(\"sound/usb/usbmixer_maps.c\", {st_mode=S_IFREG|0664, st_size=10230, ...}) = 0\n\topen(\"sound/usb/usbmixer_maps.c\", O_RDONLY) = 4\n\tmmap(NULL, 10230, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7fa247228000\n\tclose(4)                                = 0\n\taccess(\"/home/torvalds/kernel/.git/objects/d7/55be0ad8115da5c1296211678f81a7774277d5\", F_OK) = -1 ENOENT (No such file or directory)\n\topen(\"/home/torvalds/kernel/.git/objects/d7/55be0ad8115da5c1296211678f81a7774277d5\", O_RDONLY) = -1 ENOENT (No such file or directory)\n\topen(\"/home/torvalds/kernel/.git/objects/d7/tmp_obj_55be0ad8115da5c12962116_vDqXRl\", O_RDWR|O_CREAT|O_EXCL, 0600) = 4\n\twrite(4, \"x\\1\\315ZmS\\333H\\22\\276\\257\\370W\\364\\361\\t\\210\\1K\\206$\\300.\\273\\6\\314\\306\\273@(l\\'\"..., 3402) = 3402\n\tfchmod(4, 0444)                         = 0\n\tclose(4)                                = 0\n\tlink(\"/home/torvalds/kernel/.git/objects/d7/tmp_obj_55be0ad8115da5c12962116_vDqXRl\", \"/home/torvalds/kernel/.git/objects/d7/55be0ad8115da5c1296211678f81a7774277d5\") = 0\n\tunlink(\"/home/torvalds/kernel/.git/objects/d7/tmp_obj_55be0ad8115da5c12962116_vDqXRl\") = 0\n\tmunmap(0x7fa247228000, 10230)           = 0\n\nand here is the fixed version:\n\n\tlstat(\"sound/usb/usbmixer_maps.c\", {st_mode=S_IFREG|0664, st_size=10230, ...}) = 0\n\topen(\"sound/usb/usbmixer_maps.c\", O_RDONLY) = 4\n\tmmap(NULL, 10230, PROT_READ, MAP_PRIVATE, 4, 0) = 0x7f54c231a000\n\tclose(4)                                = 0\n\taccess(\".git/objects/d7/55be0ad8115da5c1296211678f81a7774277d5\", F_OK) = -1 ENOENT (No such file or directory)\n\topen(\".git/objects/d7/tmp_obj_55be0ad8115da5c12962116_fKy6px\", O_RDWR|O_CREAT|O_EXCL, 0600) = 4\n\twrite(4, \"x\\1\\315ZmS\\333H\\22\\276\\257\\370W\\364\\361\\t\\210\\1K\\206$\\300.\\273\\6\\314\\306\\273@(l\\'\"..., 3402) = 3402\n\tfchmod(4, 0444)                         = 0\n\tclose(4)                                = 0\n\tlink(\".git/objects/d7/tmp_obj_55be0ad8115da5c12962116_fKy6px\", \".git/objects/d7/55be0ad8115da5c1296211678f81a7774277d5\") = 0\n\tunlink(\".git/objects/d7/tmp_obj_55be0ad8115da5c12962116_fKy6px\") = 0\n\tmunmap(0x7f54c231a000, 10230)           = 0\n\nie note how it does one unnecessary \"open()\" less, but more importantly, \nnotice the difference between \"/home/torvalds/kernel/.git/*\" and \".git/*\", \nand realize that that second difference was the much more noticeable one.\n\nAnd no, obviously the right fix is not to just comment out all of \n\"setup_work_tree()\" (it will break stuff that depends on GIT_WORKTREE), \nbut I did that as a minimal example of showing what the bad effect of that \nfunction is.\n\nIn general, I think we've gone in the wrong direction with a lot of the \n\"make_absolute_path\" stuff. See above. 5% performance loss is not good.\n\n\t\tLinus\n"},{"id":"80092","messageId":"alpine.LFD.1.10.0806161746170.2949@woody.linux-foundation.org","threadId":"13990","inReplyTo":"alpine.LFD.1.10.0806161723081.2949@woody.linux-foundation.org","subject":"Re: 'setup_work_tree()' considered harmful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-17T00:52:03Z","receivedAt":"2008-06-17T00:52:03Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Mon, 16 Jun 2008, Linus Torvalds wrote:\n> \n> After that, I fixed \"write_loose_object()\" to not unnecessarily try to \n> open a git object file, because every single caller has already done a \n> \"has_sha1_file(sha1)\" or \"has_loose_object(sha1)\" check before calling \n> that function, so trying to open it again is just pointless.\n\nIn case anybody cares, the obvious diff for this part is appended. The \nFIXME! was wrong anyway - any collision detection needs to be done by the \ncallers when they do the \"do we already have this object\". Doing it in \nwrite_loose_object() is too late (not to mention the wrong place to try to \ndo so anyway - not all callers generate new objects: some callers just \nturn a packed object into a loose one).\n\n\t\t\tLinus\n\n---\nFrom: Linus Torvalds <torvalds@linux-foundation.org>\nDate: Mon, 16 Jun 2008 17:17:10 -0700\nSubject: [PATCH] write_loose_object: don't bother trying to read an old object\n\nBefore even calling this, all callers have done a \"has_sha1_file(sha1)\"\nor \"has_loose_object(sha1)\" check, so there is no point in doing a\nsecond check.\n\nIf something races with us on object creation, we handle that in the\nfinal link() that moves it to the right place.\n\nSigned-off-by: Linus Torvalds <torvalds@linux-foundation.org>\n---\n sha1_file.c |   14 --------------\n 1 files changed, 0 insertions(+), 14 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex f9ec069..4255099 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2160,20 +2160,6 @@ static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,\n \tstatic char tmpfile[PATH_MAX];\n \n \tfilename = sha1_file_name(sha1);\n-\tfd = open(filename, O_RDONLY);\n-\tif (fd >= 0) {\n-\t\t/*\n-\t\t * FIXME!!! We might do collision checking here, but we'd\n-\t\t * need to uncompress the old file and check it. Later.\n-\t\t */\n-\t\tclose(fd);\n-\t\treturn 0;\n-\t}\n-\n-\tif (errno != ENOENT) {\n-\t\treturn error(\"sha1 file %s: %s\\n\", filename, strerror(errno));\n-\t}\n-\n \tfd = create_tmpfile(tmpfile, sizeof(tmpfile), filename);\n \tif (fd < 0) {\n \t\tif (errno == EPERM)\n-- \n1.5.6.rc3.7.g336d0.dirty\n"},{"id":"80129","messageId":"alpine.DEB.1.00.0806171142290.6439@racer","threadId":"13990","inReplyTo":"alpine.LFD.1.10.0806161723081.2949@woody.linux-foundation.org","subject":"Re: 'setup_work_tree()' considered harmful","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-06-17T11:01:43Z","receivedAt":"2008-06-17T11:01:43Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 16 Jun 2008, Linus Torvalds wrote:\n\n> [ Dscho cc'd because I think he is the primary culprits for this thing, I \n>   think. Commit e90fdc39b6903502192b2dd11e5503cea721a1ad in particular, \n>   methinks. ]\n\nYes, I am not happy with work-tree.  In fact, I grew to positively hate \nit.  IMHO it was not well designed when it entered Git, and unfortunately \nmy endeavors to clean it up were not very successful, either.\n\n> [...]\n>\n> In particular, doing a \"git add .\" will use absolute pathnames for all \n> git files, while a \"git diff\" will not. And this is quite noticeable - \n> the absolute pathnames are not just longer, they have more path \n> components in them. Making them a lot slower to look up and use.\n> \n> [...]\n> \n> In general, I think we've gone in the wrong direction with a lot of the \n> \"make_absolute_path\" stuff. See above. 5% performance loss is not good.\n\nYes, that is true.  However, I think we need it for the case where \nwork_tree is set (technically, we could try to be clever when work_tree is \nset in such a manner that we can operate with relative paths, but I think \nthat is just not worth it).\n\nSo I am thinking about reverting to the old behavior, but _just_ for the \ncommon case that no work tree was set.\n\nThis might be more tricky than it used to be, because of the many special \ncases work trees require.\n\nI briefly considered working on this now, but I simply do not have the \ntime, and I seem to be unconcentrated these days.  Probably the best thing \nwould be to scrap the whole work-tree thing and throw it out, admitting \nthat it was a mistake to begin with.\n\nCiao,\nDscho\n"},{"id":"80209","messageId":"20080618090527.GA24254@glandium.org","threadId":"13990","inReplyTo":"alpine.LFD.1.10.0806161723081.2949@woody.linux-foundation.org","subject":"Re: 'setup_work_tree()' considered harmful","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2008-06-18T09:05:27Z","receivedAt":"2008-06-18T09:05:27Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, Jun 16, 2008 at 05:45:25PM -0700, Linus Torvalds wrote:\n> ie note how it does one unnecessary \"open()\" less, but more importantly, \n> notice the difference between \"/home/torvalds/kernel/.git/*\" and \".git/*\", \n> and realize that that second difference was the much more noticeable one.\n> \n> And no, obviously the right fix is not to just comment out all of \n> \"setup_work_tree()\" (it will break stuff that depends on GIT_WORKTREE), \n> but I did that as a minimal example of showing what the bad effect of that \n> function is.\n> \n> In general, I think we've gone in the wrong direction with a lot of the \n> \"make_absolute_path\" stuff. See above. 5% performance loss is not good.\n\nMaybe using openat, fstatat, etc. when they are available, could be a\ngood thing, already, though it wouldn't help for other platforms.\n\nMike\n"},{"id":"80229","messageId":"alpine.LFD.1.10.0806180924170.2907@woody.linux-foundation.org","threadId":"13990","inReplyTo":"20080618090527.GA24254@glandium.org","subject":"Re: 'setup_work_tree()' considered harmful","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2008-06-18T16:26:18Z","receivedAt":"2008-06-18T16:26:18Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 18 Jun 2008, Mike Hommey wrote:\n> \n> Maybe using openat, fstatat, etc. when they are available, could be a\n> good thing, already, though it wouldn't help for other platforms.\n\nI agree that openat() and friends would be nice for this (use a file \ndescriptor instead of a string to point to object directories etc), but \nit's unportable enough that the pain of having to have two totally \ndifferent access routines is just not worth it. It could be done, but it \nwould need some really nifty abstraction layer.\n\nWe do already have _part_ of that abstraction layer with things like \n\"git_path()\" etc, but it would have to be extended upon quite a bit. \n\n\t\tLinus\n"}]}