{"thread":{"id":"43061","subject":"Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.","startedAt":"2006-12-14T10:07:46Z","lastAt":"2006-12-14T11:50:54Z","messageCount":6,"participants":["Johannes Schindelin","Shawn Pearce","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"297732","messageId":"20061214100746.GA31191@spearce.org","threadId":"43061","inReplyTo":null,"subject":"[PATCH 1/1] Bypass expensive content comparsion during rename detection.","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-14T10:07:46Z","receivedAt":"2006-12-14T10:07:46Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"When comparing file contents during the second loop through a rename\ndetection attempt we can skip the expensive byte-by-byte comparsion\nif both source and destination files have valid SHA1 values.  This\nimproves performance by avoiding either an expensive open/mmap to\nread the working tree copy, or an expensive inflate of a blob object.\n\nUnfortunately we still have to at least initialize the sizes of the\nsource and destination files even if the SHA1 values don't match.\nFailing to initialize the sizes causes a number of test cases to fail\nand start reporting different copy/rename behavior than was expected.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n diffcore-rename.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 57a74b6..91fa2be 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -109,6 +109,8 @@ static int is_exact_match(struct diff_filespec *src,\n \t\treturn 0;\n \tif (src->size != dst->size)\n \t\treturn 0;\n+\tif (src->sha1_valid && dst->sha1_valid)\n+\t    return !hashcmp(src->sha1, dst->sha1);\n \tif (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0))\n \t\treturn 0;\n \tif (src->size == dst->size &&\n-- \n"},{"id":"293797","messageId":"Pine.LNX.4.63.0612141151220.3635@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"43061","inReplyTo":"20061214100746.GA31191@spearce.org","subject":"Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-14T10:53:08Z","receivedAt":"2006-12-14T10:53:08Z","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, Shawn O. Pearce wrote:\n\n> When comparing file contents during the second loop through a rename \n> detection attempt we can skip the expensive byte-by-byte comparsion if \n> both source and destination files have valid SHA1 values.  This improves \n> performance by avoiding either an expensive open/mmap to read the \n> working tree copy, or an expensive inflate of a blob object.\n> \n> Unfortunately we still have to at least initialize the sizes of the \n> source and destination files even if the SHA1 values don't match. \n> Failing to initialize the sizes causes a number of test cases to fail \n> and start reporting different copy/rename behavior than was expected.\n\nIt has a wrong feel to it when you say we have to check the sizes first. \nAfter all, we are heavily relying on the hashes being different, including \nthe case when the sizes are different. So, the order of the checks should \n_not_ matter. I suspect that somewhere sha1_valid should be set to 0, but \nisn't.\n\nCiao,\n"},{"id":"296584","messageId":"20061214110858.GE1747@spearce.org","threadId":"43061","inReplyTo":"Pine.LNX.4.63.0612141151220.3635@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-14T11:08:58Z","receivedAt":"2006-12-14T11:08:58Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> On Thu, 14 Dec 2006, Shawn O. Pearce wrote:\n> > Unfortunately we still have to at least initialize the sizes of the \n> > source and destination files even if the SHA1 values don't match. \n> > Failing to initialize the sizes causes a number of test cases to fail \n> > and start reporting different copy/rename behavior than was expected.\n> \n> It has a wrong feel to it when you say we have to check the sizes first. \n> After all, we are heavily relying on the hashes being different, including \n> the case when the sizes are different. So, the order of the checks should \n> _not_ matter. I suspect that somewhere sha1_valid should be set to 0, but \n> isn't.\n\nYes.  :-)\n\nI'll admit, I don't understand the diffcore rename code very well\nso I'm treading around in code that I'm not used to.  I'm not sure\nwhy the size member of diff_filespec needs to be initialized to get\nrename and copy detection to work properly, but it apparently does.\n\n\nMy first version of the patch had the hash comparsion right after\nwe called diff_populate_filespec to get the size data.  But then\nI realized that very often the sizes will be different and the\nsrc->size != dst->size comparsion will tend to be true most of\nthe time, thus saving us a (relatively) expensive hash comparsion,\nwhich we know must fail anyway if the sizes differed.\n\n-- \n"},{"id":"297172","messageId":"20061214111304.GF1747@spearce.org","threadId":"43061","inReplyTo":"20061214110858.GE1747@spearce.org","subject":"Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-14T11:13:04Z","receivedAt":"2006-12-14T11:13:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Shawn Pearce <spearce@spearce.org> wrote:\n> I'll admit, I don't understand the diffcore rename code very well\n> so I'm treading around in code that I'm not used to.  I'm not sure\n> why the size member of diff_filespec needs to be initialized to get\n> rename and copy detection to work properly, but it apparently does.\n\nThis chunk of code is probably a perfect example of why side-effects\ncan be so bad.  Its fast because the size information is loaded\nonce and reused later on; its horrible to maintain because you don't\nrealize that this simple predicate is actually doing something that\nmatters downstream even though it returned false!\n\n-- \n"},{"id":"294591","messageId":"Pine.LNX.4.63.0612141214440.3635@wbgn013.biozentrum.uni-wuerzburg.de","threadId":"43061","inReplyTo":"20061214110858.GE1747@spearce.org","subject":"Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2006-12-14T11:18:30Z","receivedAt":"2006-12-14T11:18:30Z","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, Shawn Pearce wrote:\n\n> My first version of the patch had the hash comparsion right after we \n> called diff_populate_filespec to get the size data.  But then I realized \n> that very often the sizes will be different and the src->size != \n> dst->size comparsion will tend to be true most of the time, thus saving \n> us a (relatively) expensive hash comparsion, which we know must fail \n> anyway if the sizes differed.\n\nAh! I misunderstood. Since the call to diff_populate_filespec was not \nvisible in the hunk, I erroneously assumed that you meant to _check_ the \nsizes before checking the hashes.\n\nBut your explanation makes lots of sense to me. May I request a short \ncomment above the new code, like \"let diff_populate_filespec() do its \nthing since we need the filesize later on anyway, and having that, do the \ncheaper filesize check before the more expensive hashcmp()\"?\n\nCiao,\nDscho\n"},{"id":"295794","messageId":"20061214115054.GJ1747@spearce.org","threadId":"43061","inReplyTo":"Pine.LNX.4.63.0612141214440.3635@wbgn013.biozentrum.uni-wuerzburg.de","subject":"Re: [PATCH 1/1] Bypass expensive content comparsion during rename detection.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-12-14T11:50:54Z","receivedAt":"2006-12-14T11:50:54Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> Hi,\n> \n> On Thu, 14 Dec 2006, Shawn Pearce wrote:\n> \n> > My first version of the patch had the hash comparsion right after we \n> > called diff_populate_filespec to get the size data.  But then I realized \n> > that very often the sizes will be different and the src->size != \n> > dst->size comparsion will tend to be true most of the time, thus saving \n> > us a (relatively) expensive hash comparsion, which we know must fail \n> > anyway if the sizes differed.\n> \n> Ah! I misunderstood. Since the call to diff_populate_filespec was not \n> visible in the hunk, I erroneously assumed that you meant to _check_ the \n> sizes before checking the hashes.\n> \n> But your explanation makes lots of sense to me. May I request a short \n> comment above the new code, like \"let diff_populate_filespec() do its \n> thing since we need the filesize later on anyway, and having that, do the \n> cheaper filesize check before the more expensive hashcmp()\"?\n\n-- 8> --\nBypass expensive content comparsion during rename detection.\n\nWhen comparing file contents during the second loop through a rename\ndetection attempt we can skip the expensive byte-by-byte comparsion\nif both source and destination files have valid SHA1 values.  This\nimproves performance by avoiding either an expensive open/mmap to\nread the working tree copy, or an expensive inflate of a blob object.\n\nUnfortunately we still have to at least initialize the sizes of the\nsource and destination files even if the SHA1 values don't match.\nFailing to initialize the sizes causes a number of test cases to fail\nand start reporting different copy/rename behavior than was expected.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n diffcore-rename.c |    9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex 57a74b6..f7748ce 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -109,6 +109,15 @@ static int is_exact_match(struct diff_filespec *src,\n \t\treturn 0;\n \tif (src->size != dst->size)\n \t\treturn 0;\n+\t/* Although we can avoid a byte-by-byte comparsion by checking\n+\t * hashes we needed to allow diff_populate_filespec to fill in\n+\t * the size members, as we need that later on to correctly do\n+\t * rename and copy detection.  Not filling in size before we\n+\t * return back when contents_too is true causes all sorts of\n+\t * havoc (been there, done that, lets not try it again).\n+\t */\n+\tif (src->sha1_valid && dst->sha1_valid)\n+\t    return !hashcmp(src->sha1, dst->sha1);\n \tif (diff_populate_filespec(src, 0) || diff_populate_filespec(dst, 0))\n \t\treturn 0;\n \tif (src->size == dst->size &&\n-- \n1.4.4.2.g72f5\n"}]}