send-pack: set core.warnAmbiguousRefs=false

I've been looking into the performance of git push for very large repos. Our users are reporting that 60-80% of git push time is spent during the "Enumerating objects" phase of git pack-objects.

A git push process runs several processes during its run, but one includes git send-pack which calls git pack-objects and passes the known have/wants into stdin using object ids. However, the default setting for core.warnAmbiguousRefs requires git pack-objects to check for ref names matching the ref_rev_parse_rules array in refs.c. This means that every object is triggering at least six "file exists?" queries.

When there are a lot of refs, this can add up significantly! My PerfView trace for a simple push measured 3 seconds spent checking these paths.

The fix for this is simple: set core.warnAmbiguousRefs to false for this specific call of git pack-objects coming from git send-pack. We don't want to default it to false for all calls to git pack-objects, as it is valid to pass ref names instead of object ids. This helps regain these seconds during a push.

In addition to this patch submission, we are looking into merging it into our fork sooner [1].

[1] https://github.com/Microsoft/git/pull/67

Derrick Stolee (1):
send-pack: set core.warnAmbiguousRefs=false

 send-pack.c | 2 ++
 1 file changed, 2 insertions(+)

base-commit: cae598d9980661a978e2df4fb338518f7bf09572
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-68%2Fderrickstolee%2Fsend-pack-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-68/derrickstolee/send-pack-config-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/68

gitgitgadget

From: Derrick Stolee <[email protected]>

During a 'git push' command, we run 'git send-pack' inside of our transport helper. This creates a 'git pack-objects' process and passes a list of object ids. If this list is large, then the pack-objects process can spend a lot of time checking the possible refs these strings could represent.

Remove this extra check by setting core.warnAmbiguousRefs to false as we call 'git pack-objects'.

---
 send-pack.c | 2 ++
 1 file changed, 2 insertions(+)

index e920ca57df..5055150fe1 100644
@@ -64,6 +64,8 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc
 	int i;
 	int rc;
 
+	argv_array_push(&po.args, "-c");
+	argv_array_push(&po.args, "core.warnAmbiguousRefs=false");
 	argv_array_push(&po.args, "pack-objects");
 	argv_array_push(&po.args, "--all-progress-implied");
 	argv_array_push(&po.args, "--revs");

gitgitgadget

Some of this might be useful in the commit message. :)

I don't think you actually care about the ambiguity check between refs here; you just care about avoiding the ref check when we've seen (and are mostly expecting) a 40-hex sha1. We have a more specific flag for
that: warn_on_object_refname_ambiguity.

And I think it would be OK to enable that all the time for pack-objects, which is plumbing that does typically expect object names. See prior art in 25fba78d36 (cat-file: disable object/refname ambiguity check for batch mode, 2013-07-12) and 4c30d50402 (rev-list: disable object/refname ambiguity check with --stdin, 2014-03-12).

Whenever I see a change like this to the pack-objects invocation for send-pack, it makes me wonder if upload-pack would want the same thing.

It's a moot point if we just set the flag directly in inside pack-objects, though.

-Peff

I'd probably do it here:

index e50c6cd1ff..d370638a5d 100644
@@ -3104,6 +3104,7 @@ static void get_object_list(int ac, const char **av)
 	struct rev_info revs;
 	char line[1000];
 	int flags = 0;
+	int save_warning;
 
 	repo_init_revisions(the_repository, &revs, NULL);
 	save_commit_buffer = 0;

@@ -3112,6 +3113,9 @@ static void get_object_list(int ac, const char **av)
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	save_warning = warn_on_object_refname_ambiguity;
+	warn_on_object_refname_ambiguity = 0;
+
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		int len = strlen(line);
 		if (len && line[len - 1] == '\n')

@@ -3138,6 +3142,8 @@ static void get_object_list(int ac, const char **av)
 			die(_("bad revision '%s'"), line);
 	}
 
+	warn_on_object_refname_ambiguity = save_warning;
+
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
 		return;
 

But I'll leave it to you to wrap that up in a patch, since you probably should re-check your timings (which it would be interesting to include in the commit message, if you have reproducible timings).

-Peff

The timings change a lot depending on the disk cache and the remote refs, which is unfortunate, but I have measured a three-second improvement.

Thanks,
-Stolee

Thanks for these pointers. Helps to know there is precedent for shutting down the behavior without relying on "-c" flags.

I'll send a v2 that does just that.

Thanks,
-Stolee

I've been looking into the performance of git push for very large repos. Our users are reporting that 60-80% of git push time is spent during the "Enumerating objects" phase of git pack-objects.

A git push process runs several processes during its run, but one includes git send-pack which calls git pack-objects and passes the known have/wants into stdin using object ids. However, the default setting for core.warnAmbiguousRefs requires git pack-objects to check for ref names matching the ref_rev_parse_rules array in refs.c. This means that every object is triggering at least six "file exists?" queries.

When there are a lot of refs, this can add up significantly! My PerfView trace for a simple push measured 3 seconds spent checking these paths.

The fix is to set the global warn_on_object_refname_ambiguity to 0 for the section that is performing these object reads.

In addition to this patch submission, we are looking into merging it into our fork sooner [1].

[1] https://github.com/Microsoft/git/pull/67

Changes in V2: Instead of using the "-c" flag from send-pack, just set the global. I left the name of the cover letter the same to not confuse anyone viewing the message without threading.

Derrick Stolee (1):
pack-objects: ignore ambiguous object warnings

 builtin/pack-objects.c | 6 ++++++
 1 file changed, 6 insertions(+)

base-commit: cae598d9980661a978e2df4fb338518f7bf09572
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-68%2Fderrickstolee%2Fsend-pack-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-68/derrickstolee/send-pack-config-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/68

Range-diff vs v1:

 1:  1ef2c51550 < -:  ---------- send-pack: set core.warnAmbiguousRefs=false
 -:  ---------- > 1:  002868ee6b pack-objects: ignore ambiguous object warnings

gitgitgadget

From: Derrick Stolee <[email protected]>

A git push process runs several processes during its run, but one includes git send-pack which calls git pack-objects and passes the known have/wants into stdin using object ids. However, the default setting for core.warnAmbiguousRefs requires git pack-objects to check for ref names matching the ref_rev_parse_rules array in refs.c. This means that every object is triggering at least six "file exists?" queries. When there are a lot of refs, this can add up significantly! I observed a simple push spending three seconds checking these paths.

The fix here is similar to 4c30d50 "rev-list: disable object/refname ambiguity check with --stdin". Save the value of the global warn_on_object_refname_ambiguity variable (which is usually set to the boolean config variable core.warnAmbiguousRefs) and change the state to false. Do this only during the get_object_list() method which reads the objects from stdin.

---
 builtin/pack-objects.c | 6 ++++++
 1 file changed, 6 insertions(+)

index d1144a8f7e..f703e6df9b 100644
@@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)
 	struct rev_info revs;
 	char line[1000];
 	int flags = 0;
+	int save_warning;
 
 	init_revisions(&revs, NULL);
 	save_commit_buffer = 0;

@@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	save_warning = warn_on_object_refname_ambiguity;
+	warn_on_object_refname_ambiguity = 0;
+
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		int len = strlen(line);
 		if (len && line[len - 1] == '\n')

@@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)
 			die(_("bad revision '%s'"), line);
 	}
 
+	warn_on_object_refname_ambiguity = save_warning;
+
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
 		return;
 

gitgitgadget

Thanks, this fills out the details from the cover letter a bit better.

I think this parenthetical isn't quite right. We never change the value of warn_on_object_refname_ambiguity based on user config. It's just that if warn_ambiguous_refs is off, we do not even bother looking at the object_refname variant.

So we'd never expect to see anything except "1" in our save_warning variable. Doing a save/restore is just about code hygiene and maintainability.

Other than that minor nit, this whole thing looks good to me.

-Peff

Here is what I plan to queue. Thanks, both.

-- >8 --
From: Derrick Stolee <[email protected]>
Date: Tue, 6 Nov 2018 12:34:47 -0800
Subject: [PATCH] pack-objects: ignore ambiguous object warnings

A git push process runs several processes during its run, but one includes git send-pack which calls git pack-objects and passes the known have/wants into stdin using object ids. However, the default setting for core.warnAmbiguousRefs requires git pack-objects to check for ref names matching the ref_rev_parse_rules array in refs.c. This means that every object is triggering at least six "file exists?" queries. When there are a lot of refs, this can add up significantly! I observed a simple push spending three seconds checking these paths.

The fix here is similar to 4c30d50 "rev-list: disable object/refname ambiguity check with --stdin". While the get_object_list() method reads the objects from stdin, turn warn_on_object_refname_ambiguity flag (which is usually true) to false. Just for code hygiene, save away the original at the beginning and restore it once we are done.

---
 builtin/pack-objects.c | 6 ++++++
 1 file changed, 6 insertions(+)

index d1144a8f7e..f703e6df9b 100644
@@ -2988,6 +2988,7 @@ static void get_object_list(int ac, const char **av)
 	struct rev_info revs;
 	char line[1000];
 	int flags = 0;
+	int save_warning;
 
 	init_revisions(&revs, NULL);
 	save_commit_buffer = 0;

@@ -2996,6 +2997,9 @@ static void get_object_list(int ac, const char **av)
 	/* make sure shallows are read */
 	is_repository_shallow(the_repository);
 
+	save_warning = warn_on_object_refname_ambiguity;
+	warn_on_object_refname_ambiguity = 0;
+
 	while (fgets(line, sizeof(line), stdin) != NULL) {
 		int len = strlen(line);
 		if (len && line[len - 1] == '\n')

@@ -3022,6 +3026,8 @@ static void get_object_list(int ac, const char **av)
 			die(_("bad revision '%s'"), line);
 	}
 
+	warn_on_object_refname_ambiguity = save_warning;
+
 	if (use_bitmap_index && !get_object_list_from_bitmap(&revs))
 		return;
 

2.19.1-856-g8858448bb4

I actually think this is a bit too broad. The calling process of this program does know that it is feeding list of raw object names (prefixed with ^ for negative ones), but the codepath this patch touches does not know who is calling it with what. It would be safer to introduce a mechanism for the caller to tell this codepath not to bother checking refnames, as it knows it is feeding the raw object names and not refnames.

After all, you can feed things like "refs/heads/master" from the standard input of "git pack-objects --revs", no?

Keep in mind that this is not disallowing "refs/heads/master", nor even disabling the ambiguity checking if we feed a "foo" that exists as both a tag and a branch.

It is only disabling the check that when the caller feeds a 40-hex sha1, we do not double-check to make sure that there is not a ref of the same name (which is not even really an ambiguity, but just an informational message for the user; the object id has always and must always take precedence).

So yes, the caller does know better "I am only going to feed object ids". But you can likewise look from the callee's perspective: "I am going to take a lot of inputs, and spending time for an informational message for each one is not worth doing".

So I think it's justifiable from that point of view. And from a practical point of view, it's much simpler: we do not have teach every caller to specify its input, or risk being slowed down by a low-value informational check.

-Peff