[PATCH v5 6/6] connected: add "auto" connectivity check mode

["Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>]

From: Kristofer Karlsson <krka@spotify.com>

The incremental connectivity check pays a per-commit cost.  For
every incoming commit it reads the new trees along the changed
paths, and the trees at the same paths in its parents to compare
against.  A parent tree is often read again even though an earlier
commit already read it: on git.git, about a third of the tree reads
for 2000 incoming commits are such re-reads.  The full check
instead pays a mostly fixed cost for walking the boundary trees,
so with many incoming commits the incremental check becomes the
slower one (see the previous commit).  Users who mostly
receive small pushes but occasionally a large one should not have
to choose between the two.

Add an "auto" value for transfer.connectivityCheck that picks the
mode based on the incoming history.  When the revision walk is
limited, which is the case whenever the receiving repository has
references, the incoming commit list is already computed by
limit_list(), so counting it is cheap.  "auto" then uses the
incremental check for up to 1000 incoming commits, and the full
check above that.

The cutoff is a fixed constant based on the p5413 fixture in the
previous commit, where breakeven is between 500 and 3000 commits.
It is conservative for repositories with large trees, where the
fixed cost of the full check is higher.  Measured on real
repositories (rev-list against a single boundary commit, mean
of at least 10 runs):

    repository   files  commits    full  incr.  full/incr
    internal A    556K     1000   1.71s  0.61s    2.8x
    internal A    556K     4000   2.78s  2.83s    1.0x
    internal B    288K     1000   1.78s  0.88s    2.0x
    linux          95K      598   0.33s  0.12s    2.7x
    linux          95K     1728   0.56s  0.35s    1.6x
    git           4.9K      984   0.22s  0.27s    0.8x

With small trees incremental can be slower within the cutoff,
but both modes are fast enough there (0.22s vs 0.27s for git
above) that the difference does not matter in practice.

No single cutoff is optimal for every repository, so it can be set
with "auto=<cutoff>"; plain "auto" uses 1000.  A malformed cutoff
is treated like any other unknown value: it falls back to "full"
with a warning.

Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
---
 Documentation/config/transfer.adoc  |  6 ++++++
 Documentation/rev-list-options.adoc |  2 +-
 builtin/rev-list.c                  | 28 ++++++++++++++++++++++++++--
 connected.c                         |  9 ++++++++-
 t/t5413-connectivity-check.sh       | 25 +++++++++++++++++++++++--
 5 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/Documentation/config/transfer.adoc b/Documentation/config/transfer.adoc
index cba7597a58..f77b25ff8b 100644
--- a/Documentation/config/transfer.adoc
+++ b/Documentation/config/transfer.adoc
@@ -18,6 +18,12 @@ transfer.connectivityCheck::
 	than `full` when there are many incoming commits.
 	Falls back to `full` for clones, pushes into an empty
 	repository and deepening fetches.
+`auto`;;
+`auto=<cutoff>`;;
+	Choose between `full` and `incremental` with a heuristic.
+	Currently `incremental` is used when there are at most
+	_<cutoff>_ incoming commits (1000 by default), and `full`
+	otherwise.  The heuristic may change in future versions.
 --
 +
 Unknown values are treated as `full`, with a warning.
diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 2023ad5f75..285fb3809f 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -1103,7 +1103,7 @@ It is an error to use `--missing-only` without `--missing=print` or
 	traversal, rather than just silencing errors about missing
 	objects.
 
-`--connectivity-check=(full|incremental)`::
+`--connectivity-check=(full|incremental|auto[=<cutoff>])`::
 	(For internal use only.)  Used by `check_connected()` to
 	select the connectivity check algorithm; see
 	`transfer.connectivityCheck` in linkgit:git-config[1].
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index c07ad6e30c..071a0e3cb5 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -29,23 +29,35 @@
 #include "quote.h"
 #include "strbuf.h"
 #include "tree-verify.h"
+#include "parse.h"
+
+#define INCREMENTAL_MAX_COMMITS 1000
 
 enum connectivity_check_mode {
 	CONNECTIVITY_CHECK_FULL,
 	CONNECTIVITY_CHECK_INCREMENTAL,
+	CONNECTIVITY_CHECK_AUTO,
 };
 
 struct connectivity_check {
 	enum connectivity_check_mode mode;
+	unsigned long max_commits;
 };
 
 static void parse_connectivity_check(const char *arg,
 				     struct connectivity_check *check)
 {
+	const char *cutoff;
+
 	if (!strcmp(arg, "full"))
 		check->mode = CONNECTIVITY_CHECK_FULL;
 	else if (!strcmp(arg, "incremental"))
 		check->mode = CONNECTIVITY_CHECK_INCREMENTAL;
+	else if (!strcmp(arg, "auto"))
+		check->mode = CONNECTIVITY_CHECK_AUTO;
+	else if (skip_prefix(arg, "auto=", &cutoff) &&
+		 git_parse_ulong(cutoff, &check->max_commits))
+		check->mode = CONNECTIVITY_CHECK_AUTO;
 	else
 		die(_("unknown --connectivity-check mode '%s'"), arg);
 }
@@ -748,7 +760,9 @@ int cmd_rev_list(int argc,
 	int bisect_find_all = 0;
 	int use_bitmap_index = 0;
 	int filter_provided_objects = 0;
-	struct connectivity_check connectivity_check = { 0 };
+	struct connectivity_check connectivity_check = {
+		.max_commits = INCREMENTAL_MAX_COMMITS,
+	};
 	const char *show_progress = NULL;
 	int ret = 0;
 
@@ -1001,9 +1015,19 @@ int cmd_rev_list(int argc,
 	 * The incremental check would still work, but it would verify
 	 * the whole object graph like the full check, with extra
 	 * per-commit overhead on top, so use the full check.
+	 *
+	 * Otherwise the walk is limited, and revs.commits already holds
+	 * all incoming commits, so counting them for "auto" is cheap.
 	 */
-	if (!revs.limited)
+	if (!revs.limited) {
 		connectivity_check.mode = CONNECTIVITY_CHECK_FULL;
+	} else if (connectivity_check.mode == CONNECTIVITY_CHECK_AUTO) {
+		if (commit_list_count(revs.commits) <=
+		    connectivity_check.max_commits)
+			connectivity_check.mode = CONNECTIVITY_CHECK_INCREMENTAL;
+		else
+			connectivity_check.mode = CONNECTIVITY_CHECK_FULL;
+	}
 
 	if (connectivity_check.mode == CONNECTIVITY_CHECK_INCREMENTAL) {
 		struct commit *commit;
diff --git a/connected.c b/connected.c
index c032987410..8ceaa45644 100644
--- a/connected.c
+++ b/connected.c
@@ -3,6 +3,7 @@
 #include "gettext.h"
 #include "hex.h"
 #include "odb.h"
+#include "parse.h"
 #include "run-command.h"
 #include "sigchain.h"
 #include "connected.h"
@@ -74,7 +75,8 @@ static void connectivity_check_mode(struct repository *repo,
 				    struct check_connected_options *opt,
 				    struct strbuf *mode)
 {
-	const char *algorithm = NULL;
+	const char *algorithm = NULL, *cutoff;
+	unsigned long max_commits;
 
 	/*
 	 * Deepening fetches do not have the usual boundary of
@@ -93,6 +95,11 @@ static void connectivity_check_mode(struct repository *repo,
 		strbuf_addstr(mode, "full");
 	else if (!strcasecmp(algorithm, "incremental"))
 		strbuf_addstr(mode, "incremental");
+	else if (!strcasecmp(algorithm, "auto"))
+		strbuf_addstr(mode, "auto");
+	else if (skip_iprefix(algorithm, "auto=", &cutoff) &&
+		 git_parse_ulong(cutoff, &max_commits))
+		strbuf_addf(mode, "auto=%lu", max_commits);
 	else {
 		warning(_("unknown transfer.connectivityCheck algorithm '%s', "
 			  "using 'full'"), algorithm);
diff --git a/t/t5413-connectivity-check.sh b/t/t5413-connectivity-check.sh
index 109ad17cff..60edf99b63 100755
--- a/t/t5413-connectivity-check.sh
+++ b/t/t5413-connectivity-check.sh
@@ -725,7 +725,7 @@ test_expect_success 'unknown transfer.connectivityCheck falls back to full' '
 	test_when_finished "rm -rf dst" &&
 	git init --bare dst &&
 	git -C dst fetch ../src base:refs/heads/main &&
-	for value in bogus
+	for value in bogus auto= auto=x
 	do
 		set_connectivity_check dst "$value" &&
 		run_traced git -C dst fetch ../src main &&
@@ -753,7 +753,7 @@ test_expect_success 'fetch uses incremental when configured' '
 	expect_mode incremental
 '
 
-for mode in incremental
+for mode in incremental auto
 do
 	test_expect_success "push with $mode into an empty repository uses full" '
 		test_when_finished "rm -rf dst" &&
@@ -771,4 +771,25 @@ do
 	'
 done
 
+for value in auto auto=1
+do
+	test_expect_success "push with $value uses incremental up to the cutoff" '
+		test_when_finished "rm -rf dst" &&
+		git init --bare dst &&
+		git -C src push ../dst base:refs/heads/main &&
+		set_connectivity_check dst $value &&
+		run_traced git -C src push ../dst main &&
+		expect_mode incremental
+	'
+done
+
+test_expect_success 'push with auto uses full above the cutoff' '
+	test_when_finished "rm -rf dst" &&
+	git init --bare dst &&
+	git -C src push ../dst base:refs/heads/main &&
+	set_connectivity_check dst auto=0 &&
+	run_traced git -C src push ../dst main &&
+	expect_mode full
+'
+
 test_done
-- 
gitgitgadget