Re: [PATCH 8/8] cache-tree: avoid path comparison loop when silent

From: René Scharfe <l.s.r@web.de>
Date: Dec 31, 2020, 12:34 UTC
Message-ID: <daeb59ee-861d-9c8d-3601-6aef1ac3fc94@web.de>
In-Reply-To: <20ea7050324cdd78b0966f54366b26224dfc7814.1609356414.git.gitgitgadget@gmail.com>

Am 30.12.20 um 20:26 schrieb Derrick Stolee via GitGitGadget:

Show 16 quoted lines

> From: Derrick Stolee <dstolee@microsoft.com>
>
> The verify_cache() method is used both for debugging issues with the
> cached tree extension but also to avoid using the extension when there
> are unmerged entries. It also checks for cache entries out of order with
> respect to file-versus-directory sorting.
>
> In 996277c (Refactor cache_tree_update idiom from commit, 2011-12-06),
> the silent option was added to remove the progress indicators from the
> initial loop looking for unmerged entries. This was changed to be a flag
> in e859c69 (cache-tree: update API to take abitrary flags, 2012-01-16).
>
> In both cases, the silent option is ignored for the second loop that
> checks for file-versus-directory sorting. It must be that this loop is
> intended only for debugging purposes and is not actually helpful in
> practice.

So you're saying that the directory/file conflict check not honoring the WRITE_TREE_SILENT flag would have been noticed as a bug and therefore doesn't happen?

I'm not sure I can follow that logic. I don't know how important that check is, how often it found a conflict and how likely such a find is overlooked or ignored, but disabling a check in silent mode that affects the return code instead of only suppressing its messages seems risky.

If we are sure that the check cannot trigger then we should remove it. If we are not so sure, but a conflict would be Git's fault (and not the user's) then we should always do the check and BUG out. And otherwise we should keep it.

Show 9 quoted lines

> Since verify_cache() is called in cache_tree_update(), which is run
> during 'git commit', we could speed up 'git commit' operations by not
> iterating through this loop another time. Of course, noticing this loop
> requires an incredibly large index.
>
> To get a measurable difference, I needed to run this test on the
> Microsoft Office monorepo, which has over three million entries in its
> index. I used 'git commit --amend --no-edit' as my command and saw the
> performance go from 752ms to 739ms with this change.

Could you please check the performance impact of the following code simplification?

diff --git a/cache-tree.c b/cache-tree.c
index a537a806c1..1105cfe6b7 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -187,10 +187,8 @@ static int verify_cache(struct cache_entry **cache,
 		 */
 		const char *this_name = cache[i]->name;
 		const char *next_name = cache[i+1]->name;
-		int this_len = strlen(this_name);
-		if (this_len < strlen(next_name) &&
-		    strncmp(this_name, next_name, this_len) == 0 &&
-		    next_name[this_len] == '/') {
+		const char *p;
+		if (skip_prefix(next_name, this_name, &p) && *p == '/') {
 			if (10 < ++funny) {
 				fprintf(stderr, "...\n");
 				break;