{"thread":{"id":"43395","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","startedAt":"2006-12-14T11:15:57Z","lastAt":"2006-12-16T13:38:21Z","messageCount":10,"participants":["Alex Riesen","Johannes Schindelin","Junio C Hamano","Shawn Pearce","Shawn O. Pearce","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"296401","messageId":"20061214111557.GA24297@spearce.org","threadId":"43395","inReplyTo":null,"subject":"[PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-14T11:15:57Z","receivedAt":"2006-12-14T11:15:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"The Cygwin folks have done a fine job at creating a POSIX layer\non Windows That Just Works(tm). However it comes with a penalty;\naccessing files in the working tree by way of stat/open/mmap can\nbe slower for diffcore than inflating the data from a blob which\nis stored in a packfile.\n\nThis performance problem is especially an issue in merge-recursive\nwhen dealing with nearly 7000 added files, as we are loading\neach file's content from the working directory to perform rename\ndetection. I have literally seen (and sadly watched) paint dry in\nless time than it takes for merge-recursive to finish such a merge.\nOn the other hand this very same merge runs very fast on Solaris.\n\nIf Git is compiled with NO_FAST_WORKING_DIRECTORY set then we will\navoid looking at the working directory when the blob in question\nis available within a packfile and the caller doesn't need the data\nunpacked into a temporary file.\n\nWe don't use loose objects as they have the same open/mmap/close\ncosts as the working directory file access, but have the additional\nCPU overhead of needing to inflate the content before use. So it\nis still faster to use the working tree file over the loose object.\n\nIf the caller needs the file data unpacked into a temporary file\nits likely because they are going to call an external diff program,\npassing the file as a parameter. In this case reusing the working\ntree file will be faster as we don't need to inflate the data and\nwrite it out to a temporary file.\n\nThe NO_FAST_WORKING_DIRECTORY feature is enabled by default on\nCygwin, as that is the platform which currently appears to benefit\nthe most from this option.\n\nSigned-off-by: Shawn O. Pearce \n---\n Makefile | 7 +++++++\n diff.c | 20 +++++++++++++++++---\n 2 files changed, 24 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex bf4c65d..cd9992a 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -69,6 +69,9 @@ all:\n #\n # Define NO_MMAP if you want to avoid mmap.\n #\n+# Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n+# generally faster on your platform than accessing the working directory.\n+#\n # Define NO_IPV6 if you lack IPv6 support and getaddrinfo().\n #\n # Define NO_SOCKADDR_STORAGE if your platform does not have struct\n@@ -355,6 +358,7 @@ ifeq ($(uname_O),Cygwin)\n \tNO_SYMLINK_HEAD = YesPlease\n \tNEEDS_LIBICONV = YesPlease\n \tNO_C99_FORMAT = YesPlease\n+\tNO_FAST_WORKING_DIRECTORY = UnfortunatelyYes\n \t# There are conflicting reports about this.\n \t# On some boxes NO_MMAP is needed, and not so elsewhere.\n \t# Try uncommenting this if you see things break -- YMMV.\n@@ -506,6 +510,9 @@ ifdef NO_MMAP\n \tCOMPAT_CFLAGS += -DNO_MMAP\n \tCOMPAT_OBJS += compat/mmap.o\n endif\n+ifdef NO_FAST_WORKING_DIRECTORY\n+\tBASIC_CFLAGS += -DNO_FAST_WORKING_DIRECTORY\n+endif\n ifdef NO_IPV6\n \tBASIC_CFLAGS += -DNO_IPV6\n endif\ndiff --git a/diff.c b/diff.c\nindex 0b284b3..565b23c 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1172,7 +1172,7 @@ void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,\n * the work tree has that object contents, return true, so that\n * prepare_temp_file() does not have to inflate and extract.\n */\n-static int work_tree_matches(const char *name, const unsigned char *sha1)\n+static int work_tree_matches(const char *name, const unsigned char *sha1, int want_file)\n {\n \tstruct cache_entry *ce;\n \tstruct stat st;\n@@ -1193,6 +1193,20 @@ static int work_tree_matches(const char *name, const unsigned char *sha1)\n \tif (!active_cache)\n \t\treturn 0;\n \n+#ifdef NO_FAST_WORKING_DIRECTORY\n+\t/* We want to avoid the working directory if our caller\n+\t * doesn't need the data in a normal file, this system\n+\t * is rather slow with its stat/open/mmap/close syscalls,\n+\t * and the object is contained in a pack file. The pack\n+\t * is probably already open and will be faster to obtain\n+\t * the data through than the working directory. Loose\n+\t * objects however would tend to be slower as they need\n+\t * to be individually opened and inflated.\n+\t */\n+\tif (!want_file && has_sha1_pack(sha1, NULL))\n+\t\treturn 0;\n+#endif\n+\n \tlen = strlen(name);\n \tpos = cache_name_pos(name, len);\n \tif (pos < 0)\n@@ -1279,7 +1293,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \tif (s->data)\n \t\treturn err;\n \tif (!s->sha1_valid ||\n-\t work_tree_matches(s->path, s->sha1)) {\n+\t work_tree_matches(s->path, s->sha1, 0)) {\n \t\tstruct stat st;\n \t\tint fd;\n \t\tif (lstat(s->path, &st) < 0) {\n@@ -1386,7 +1400,7 @@ static void prepare_temp_file(const char *name,\n \t}\n \n \tif (!one->sha1_valid ||\n-\t work_tree_matches(name, one->sha1)) {\n+\t work_tree_matches(name, one->sha1, 1)) {\n \t\tstruct stat st;\n \t\tif (lstat(name, &st) < 0) {\n \t\t\tif (errno == ENOENT)\n-- \n"},{"id":"298072","messageId":"81b0412b0612140557u225ca00du5b15823d05fda4b9@mail.gmail.com","threadId":"43395","inReplyTo":"20061214111557.GA24297@spearce.org","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2006-12-14T13:57:07Z","receivedAt":"2006-12-14T13:57:07Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 12/14/06, Shawn O. Pearce wrote:\n> If Git is compiled with NO_FAST_WORKING_DIRECTORY set then we will\n> avoid looking at the working directory when the blob in question\n> is available within a packfile and the caller doesn't need the data\n> unpacked into a temporary file.\n\n"},{"id":"295033","messageId":"Pine.LNX.4.63.0612141511110.3635@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"43395","inReplyTo":"81b0412b0612140557u225ca00du5b15823d05fda4b9@mail.gmail.com","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-14T14:12:47Z","receivedAt":"2006-12-14T14:12:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 14 Dec 2006, Alex Riesen wrote:\n\n> On 12/14/06, Shawn O. Pearce wrote:\n> > If Git is compiled with NO_FAST_WORKING_DIRECTORY set then we will\n> > avoid looking at the working directory when the blob in question\n> > is available within a packfile and the caller doesn't need the data\n> > unpacked into a temporary file.\n> \n> Why can't it be useful in generic code? What are the downsides?\n\nIt is usually cheaper to just read the file, especially if it is still \ncached, because the alternative means unpacking the loose object, or \nworse, unpacking the packed object _along_ with the objects in its delta \nchain.\n\nNot every OS sucks cache-wise, and you should not make others suffer for \nRedmond's shortcomings.\n\nCiao,\nDscho\n"},{"id":"294610","messageId":"81b0412b0612140649i71643aaar847460ca9e4cea48@mail.gmail.com","threadId":"43395","inReplyTo":"Pine.LNX.4.63.0612141511110.3635@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Alex Riesen","fromEmail":"raa.lkml@gmail.com","sentAt":"2006-12-14T14:49:38Z","receivedAt":"2006-12-14T14:49:38Z","isPatch":true,"sender":{"key":"raa.lkml@gmail.com","avatar":"https://avatars.githubusercontent.com/u/324101?v=4"},"body":"On 12/14/06, Johannes Schindelin wrote:\n> > > If Git is compiled with NO_FAST_WORKING_DIRECTORY set then we will\n> > > avoid looking at the working directory when the blob in question\n> > > is available within a packfile and the caller doesn't need the data\n> > > unpacked into a temporary file.\n> >\n> > Why can't it be useful in generic code? What are the downsides?\n>\n> It is usually cheaper to just read the file, especially if it is still\n> cached, because the alternative means unpacking the loose object, or\n> worse, unpacking the packed object _along_ with the objects in its delta\n> chain.\n\nBut you have to read less, and even that could be in cache as well\nand unpacking in userspace could be faster than open/write temporary/\nread temporary/close/unlink temporary file on a normal system\n\n> Not every OS sucks cache-wise, and you should not make others suffer for\n> Redmond's shortcomings.\n\nI'm just do not understand why avoiding temporary file wouldn't help\n"},{"id":"298672","messageId":"Pine.LNX.4.63.0612141633340.3635@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"43395","inReplyTo":"81b0412b0612140649i71643aaar847460ca9e4cea48@mail.gmail.com","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-14T15:37:35Z","receivedAt":"2006-12-14T15:37:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 14 Dec 2006, Alex Riesen wrote:\n\n> On 12/14/06, Johannes Schindelin wrote:\n> > \n> > It is usually cheaper to just read the file, especially if it is still \n> > cached, because the alternative means unpacking the loose object, or \n> > worse, unpacking the packed object _along_ with the objects in its \n> > delta chain.\n> \n> But you have to read less, and even that could be in cache as well and \n> unpacking in userspace could be faster than open/write temporary/ read \n> temporary/close/unlink temporary file on a normal system\n\nYou have to unpack anyway, since even the loose objects are packed. But to \nreconstruct a deltified object, you have to reconstruct possibly many \nobjects.\n\nSo yes, if you have it in the working directory (unpacked), it should be \nfaster to just read it, especially if it is still in the filesystem cache.\n\n> > Not every OS sucks cache-wise, and you should not make others suffer \n> > for Redmond's shortcomings.\n> \n> I'm just do not understand why avoiding temporary file wouldn't help all \n> OSes, even if they do not suck cache-wise.\n\nAh! But it is not temporary! It is the \"working copy\", which means the \nfile you have in the working directory. IOW it is an unpacked blob, which \nhappens to be already unpacked anyway.\n\nCiao,\nDscho\n"},{"id":"298170","messageId":"eltr9v$b4q$1@sea.gmane.org","threadId":"43395","inReplyTo":"20061214111557.GA24297@spearce.org","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-12-15T09:55:32Z","receivedAt":"2006-12-15T09:55:32Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Shawn O. Pearce wrote:\n\n> +# Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n> +# generally faster on your platform than accessing the working directory.\n> +#\n\nHmmm... I wonder if generated by autoconf ./configure script can autodetect\nthat. Probably not.\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n\n"},{"id":"295859","messageId":"20061215150500.GC17860@spearce.org","threadId":"43395","inReplyTo":"eltr9v$b4q$1@sea.gmane.org","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-15T15:05:00Z","receivedAt":"2006-12-15T15:05:00Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Jakub Narebski wrote:\n> Shawn O. Pearce wrote:\n> \n> > +# Define NO_FAST_WORKING_DIRECTORY if accessing objects in pack files is\n> > +# generally faster on your platform than accessing the working directory.\n> > +#\n> \n> Hmmm... I wonder if generated by autoconf ./configure script can autodetect\n> that. Probably not.\n\nI think it would be difficult to perform an automatic performance\ntest prior to building Git to determine how we should build Git on\nthe current platform. Besides, Cygwin users tend to not use the\n./configure script as it takes longer than just using the Makefile.\n\nAFAIK the only \"OS\" which suffers from this problem is Windows.\nAnything else that Git has been ported too has reasonably fast\nsyscalls for file IO, which means this should be unset.\n\n-- \n"},{"id":"295595","messageId":"7vpsakl6mu.fsf@assigned-by-dhcp.cox.net","threadId":"43395","inReplyTo":"20061214111557.GA24297@spearce.org","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-16T06:03:21Z","receivedAt":"2006-12-16T06:03:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> If Git is compiled with NO_FAST_WORKING_DIRECTORY set then we will\n> avoid looking at the working directory when the blob in question\n> is available within a packfile and the caller doesn't need the data\n> unpacked into a temporary file.\n\nI'd take the patch as is, but...\n\n> -static int work_tree_matches(const char *name, const unsigned char *sha1)\n> +static int work_tree_matches(const char *name, const unsigned char *sha1, int want_file)\n\nthis feels wrong. It is not about \"work tree matches\" anymore.\nreuse_worktree_copy(), perhaps.\n\n> @@ -1193,6 +1193,20 @@ static int work_tree_matches(const char *name, const unsigned char *sha1)\n> \tif (!active_cache)\n> \t\treturn 0;\n> \n> +#ifdef NO_FAST_WORKING_DIRECTORY\n> +\t/* We want to avoid the working directory if our caller\n> +\t * doesn't need the data in a normal file, this system\n> +\t * is rather slow with its stat/open/mmap/close syscalls,\n> +\t * and the object is contained in a pack file. The pack\n> +\t * is probably already open and will be faster to obtain\n> +\t * the data through than the working directory. Loose\n> +\t * objects however would tend to be slower as they need\n> +\t * to be individually opened and inflated.\n> +\t */\n> +\tif (!want_file && has_sha1_pack(sha1, NULL))\n> +\t\treturn 0;\n> +#endif\n> +\n\nAlso I'd prefer doing this without #ifdef;\n\n if (defined(NO_FAST_WORKING_DIRECTORY) &&\n \t!want_file && has_sha1_pack(sha1, NULL))\n\t\treturn 0;\n"},{"id":"297278","messageId":"Pine.LNX.4.63.0612161253080.3635@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"43395","inReplyTo":"7vpsakl6mu.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-16T11:54:54Z","receivedAt":"2006-12-16T11:54:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 15 Dec 2006, Junio C Hamano wrote:\n\n> Also I'd prefer doing this without #ifdef;\n> \n> if (defined(NO_FAST_WORKING_DIRECTORY) &&\n> \t!want_file && has_sha1_pack(sha1, NULL))\n> \t\treturn 0;\n\nAre you sure? AFAIU it is an OS dependent problem, so it should not be \nconfigurable at runtime anyway.\n\nCiao,\nDscho\n"},{"id":"297816","messageId":"7v7iwsezaq.fsf@assigned-by-dhcp.cox.net","threadId":"43395","inReplyTo":"Pine.LNX.4.63.0612161253080.3635@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH] Avoid accessing a slow working copy during diffcore operations.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-16T13:38:21Z","receivedAt":"2006-12-16T13:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 15 Dec 2006, Junio C Hamano wrote:\n>\n>> Also I'd prefer doing this without #ifdef;\n>> \n>> if (defined(NO_FAST_WORKING_DIRECTORY) &&\n>> \t!want_file && has_sha1_pack(sha1, NULL))\n>> \t\treturn 0;\n>\n> Are you sure? AFAIU it is an OS dependent problem, so it should not be \n> configurable at runtime anyway.\n\nWell, defined(SYMBOL) does not seem to be a valid C X-<, and I\nrewrote it in what I committed. But the point was that the\nwhole thing:\n\n\tif (!FAST_WORKING_DIRECTORY && whatever)\n\t\tstatement;\n\nwhen FAST_WORKING_DIRECTORY is a compilation time constant,\nwould be optimized out for sane platforms with a reasonable\ncompiler.\n"}]}