Re: Performance problems in diffcore rename

Subsystems: the rest

3 messages, 2 authors, 2016-08-11 · open the first message on its own page

Re: Performance problems in diffcore rename

From: Junio C Hamano <hidden>
Date: 2016-08-11 19:44:24

Shawn Pearce [off-list ref] writes:

But looking at is_exact_match I'm now wondering why we bother
to open the working tree file at all here.

You are right.  When we _know_ both SHA-1 can be trusted, we
should be able to do just this, regardless of contents_too
(whose only purpose is to delay comparing a cache dirty working
tree file with something else):

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 57a74b6..677db85 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c

@@ -100,9 +100,8 @@ static int is_exact_match(struct diff_filespec *src,
 			  struct diff_filespec *dst,
 			  int contents_too)
 {
-	if (src->sha1_valid && dst->sha1_valid &&
-	    !hashcmp(src->sha1, dst->sha1))
-		return 1;
+	if (src->sha1_valid && dst->sha1_valid)
+		return !hashcmp(src->sha1, dst->sha1);
 	if (!contents_too)
 		return 0;
 	if (diff_populate_filespec(src, 1) || diff_populate_filespec(dst, 1))

Re: Performance problems in diffcore rename

From: Shawn Pearce <hidden>
Date: 2016-08-11 19:25:37

Junio C Hamano [off-list ref] wrote:

quoted hunk

Shawn Pearce [off-list ref] writes:

quoted

But looking at is_exact_match I'm now wondering why we bother
to open the working tree file at all here.

You are right.  When we _know_ both SHA-1 can be trusted, we
should be able to do just this, regardless of contents_too
(whose only purpose is to delay comparing a cache dirty working
tree file with something else):

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 57a74b6..677db85 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c

@@ -100,9 +100,8 @@ static int is_exact_match(struct diff_filespec *src,
 			  struct diff_filespec *dst,
 			  int contents_too)
 {
-	if (src->sha1_valid && dst->sha1_valid &&
-	    !hashcmp(src->sha1, dst->sha1))
-		return 1;
+	if (src->sha1_valid && dst->sha1_valid)
+		return !hashcmp(src->sha1, dst->sha1);
 	if (!contents_too)
 		return 0;
 	if (diff_populate_filespec(src, 1) || diff_populate_filespec(dst, 1))

OK, I've stared at this problem for hours.  The above patch breaks
a number of test cases (such as ./t4005-diff-rename-2.sh) as it
causes the rename/copy detection to come out differently.

It turns out the difference is not filling in the size field of
struct diff_filespec.  By bypassing that step in is_exact_match
during the second pass we are altering the outcome.

--

Re: Performance problems in diffcore rename

From: Shawn Pearce <hidden>
Date: 2016-08-11 19:32:07

Junio C Hamano [off-list ref] wrote:

quoted hunk

Shawn Pearce [off-list ref] writes:

quoted

But looking at is_exact_match I'm now wondering why we bother
to open the working tree file at all here.

You are right.  When we _know_ both SHA-1 can be trusted, we
should be able to do just this, regardless of contents_too
(whose only purpose is to delay comparing a cache dirty working
tree file with something else):

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 57a74b6..677db85 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c

@@ -100,9 +100,8 @@ static int is_exact_match(struct diff_filespec *src,
 			  struct diff_filespec *dst,
 			  int contents_too)
 {
-	if (src->sha1_valid && dst->sha1_valid &&
-	    !hashcmp(src->sha1, dst->sha1))
-		return 1;
+	if (src->sha1_valid && dst->sha1_valid)
+		return !hashcmp(src->sha1, dst->sha1);
 	if (!contents_too)
 		return 0;
 	if (diff_populate_filespec(src, 1) || diff_populate_filespec(dst, 1))

Have you tested the above patch?  I have the exact same one in
my working directory right now and its failing a huge number of
the tests.  At least great minds think alike that we can make this
optimization here, but there's something amiss...

--

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help