Re: [PATCH] diff: do not reuse worktree files that need "clean" conversion

Jeff King [off-list ref] writes:

The would_convert_to_git() function is not a free operation.  It
needs to prime the attribute stack, so it needs to open/read/parse a
few files ($GIT_DIR/info/attributes and .gitattributes at least, and
more if your directory hierarchy is deep) on the filesystem.  The
cost is amortized across paths, but we do not even enable the
optimization if we have to pay the cost of reading the index
ourselves.

I suspect that we may be better off disabling this optimization if
we need to always call the helper.

On Fri, Jul 22, 2016 at 10:44:08AM -0700, Junio C Hamano wrote:

quoted

diff --git a/diff.c b/diff.c
index 7d03419..b43d3dd 100644
--- a/diff.c
+++ b/diff.c

@@ -2683,6 +2683,13 @@ static int reuse_worktree_file(const char *name, const unsigned char *sha1, int
 	if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))
 		return 0;
 
+	/*
+	 * Similarly, if we'd have to convert the file contents anyway, that
+	 * makes the optimization not worthwhile.
+	 */
+	if (!want_file && would_convert_to_git(name))
+		return 0;

The would_convert_to_git() function is not a free operation.  It
needs to prime the attribute stack, so it needs to open/read/parse a
few files ($GIT_DIR/info/attributes and .gitattributes at least, and
more if your directory hierarchy is deep) on the filesystem.  The
cost is amortized across paths, but we do not even enable the
optimization if we have to pay the cost of reading the index
ourselves.

Yeah, I almost commented on that, and its position in the function, but
forgot to.

The only code path which will trigger this is diff_populate_filespec.
After reuse_worktree_file() says "yes, we can reuse", we drop into a
conditional that will end in us calling convert_to_git() anyway, which
will do the same lookup. We don't need to cache, because the expensive
parts of the attribute-lookup are already cached for us by the attribute
code.

So my initial thought was to put it at the end of reuse_worktree_file(),
where it would have the least impact. If we find we cannot reuse the
file, then we would skip both this new attr lookup and the one in
diff_populate_filespec().

But in practice, I think we'll already have cached those attrs before we
even hit this function, because we'll hit the userdiff_find_by_path()
code earlier in the diff process (e.g., to see if it's binary, if we
need to textconv, etc). Those look for different attributes, but I think
the expensive bits (finding, opening, reading attribute files) are
cached across all lookups.

So I think we actually _can_ think of would_convert_to_git() as
basically free. Or as free as other efficient-lookup functions we call
like cache_name_pos(). And so I moved it further up in the function,
where it lets us avoid doing more out-of-process work (like calling
lstat() so we can ce_match_stat() on the result).

Possibly it should go after the cache_name_pos() call, though. That's
likely to be less expensive than the actual walk of the attr tree.

I suspect that we may be better off disabling this optimization if
we need to always call the helper.

The thought "does this tree reuse even speed things up enough to justify
its complexity" definitely crossed my mind. It's basically swapping
open/mmap for zlib inflating the content.

But I do think it helps in the "want_file" case (i.e., when we are
writing out a tempfile for an external command via prepare_temp_file()).
There it helps us omit writing a tempfile to disk entirely, including
any possible conversion.

-Peff