odb: scan all sources' packfiles before loose objects (#975)

Switching branches in a VFS for Git or Scalar enlistment became drastically
slower in Git v2.55. The cause is the interaction of two independent changes,
not a single one.

Commit 062b914 (treewide: convert users of repo_has_object_file() to
has_object()), first released in v2.50, accidentally inverted the object
existence check in cache_tree_fully_valid(), so that function bailed out at the
root instead of validating the cache tree recursively. Commit 5217312
(cache-tree: fix inverted object existence check in cache_tree_fully_valid),
first released in v2.55, correctly restored the recursion.

In between, v2.54's per-source object database refactor changed the
cross-source lookup order: instead of scanning all packfiles before any loose
object store, it scans packed then loose per source. That change caused no
observable checkout regression in v2.54 precisely because the inverted check
prevented recursion and thereby masked the per-object cost. Conversely, v2.49
did recurse, yet stayed fast because its global pack-first lookup found the
objects in an alternate's packs before attempting any loose lookup. v2.55 is
therefore the first version combining recursive cache-tree validation with
per-source packed-then-loose lookup, at a cost of roughly one wasted primary
loose-object lstat() per cache-tree node.

In enlistments that use an alternate object cache, that cost is severe.
cache_tree_fully_valid() calls odb_has_object() hundreds of thousands of times,
and ODB_HAS_OBJECT_RECHECK_PACKED clears OBJECT_INFO_QUICK, so each call
performs a real lstat() in the primary loose object store before the object is
found in the alternate's packfile. On a measured index with about 2.4M entries:
381,006 cache-tree nodes, 380,944 wasted lstat() calls, not a single miss;
cache-tree validation took about 32 seconds and switching to a branch pointing
at the same commit about 36 seconds.

This merge restores the all-sources-packed-before-all-sources-loose order
whenever more than one source is present. The wasted stats are gone, validation
drops to about 2 seconds and branch switching to about 6-7 seconds. Presence
semantics are unchanged.

The trade-off needs to be stated plainly: the mitigation lives in the shared
object-info lookup, which is a slightly incorrect representation layer for a
problem specific to presence-only queries via odb_has_object(). When an object
is loose in the primary and packed in an alternate, metadata callers now
observe the alternate's packed representation. Observable differences include
reported on-disk size, mtime, delta base, corruption handling, and
promisor-pack classification. Object content, type and logical size, as well as
presence, remain correct because objects are content-addressed, which bounds
the fallout to the representation level. We accept those behavior changes
deliberately in exchange for fixing an intolerable regression in this fork now.

The architecturally correct fix is upstream's plan to move alternate handling
into the files backend. That backend would own both the primary and the
alternates and could therefore scan all relevant packs before any loose-object
lookup without violating the abstraction. The upstream contributor estimates
that work at "three to four patch years", i.e. likely months in Git project
time, and probably not before Git 2.56. microsoft/git cannot wait that long.

This merge is consequently an explicitly temporary mitigation specific to
Microsoft Git. Once upstream's fix lands, this implementation should be
replaced and the regression test reassessed. That test currently asserts
packed-versus-loose selection through %(deltabase), which necessarily pins
representation ordering and may not survive the upstream architecture. A future
replacement could instead verify the absence of the unwanted lstat() calls
directly, possibly as a Linux-only strace test, since the behavior itself is
platform-independent.

Author: dscho

odb.c

@@ -756,6 +756,19 @@ static int register_all_submodule_sources(struct object_database *odb)
 	return ret;
 }
 
+static int read_object_info_from_sources(struct object_database *odb,
+					 const struct object_id *oid,
+					 struct object_info *oi,
+					 enum object_info_flags flags)
+{
+	struct odb_source *source;
+
+	for (source = odb->sources; source; source = source->next)
+		if (!odb_source_read_object_info(source, oid, oi, flags))
+			return 0;
+	return -1;
+}
+
 static int do_oid_object_info_extended(struct object_database *odb,
 				       const struct object_id *oid,
 				       struct object_info *oi, unsigned flags)
@@ -781,9 +794,28 @@ static int do_oid_object_info_extended(struct object_database *odb,
 		extern int core_use_gvfs_helper;
 		struct odb_source *source;
 
-		for (source = odb->sources; source; source = source->next)
-			if (!odb_source_read_object_info(source, real, oi, flags))
+		/*
+		 * With one or more alternates, scan the packfiles of
+		 * every source before consulting any source's loose
+		 * object store. Otherwise a primary-source loose lookup
+		 * -- a filesystem stat that, without OBJECT_INFO_QUICK,
+		 * bypasses the cached loose index -- runs for every
+		 * object that resides in an alternate's packfile.
+		 * cache_tree_fully_valid() checks many tree objects that
+		 * live in an alternate, so this avoids a stat() per
+		 * object.
+		 */
+		if (odb->sources && odb->sources->next) {
+			if (!read_object_info_from_sources(odb, real, oi,
+					flags | OBJECT_INFO_SKIP_LOOSE))
 				return 0;
+			if (!read_object_info_from_sources(odb, real, oi,
+					flags | OBJECT_INFO_SKIP_PACKED))
+				return 0;
+		} else if (!read_object_info_from_sources(odb, real, oi,
+							  flags)) {
+			return 0;
+		}
 
 		if (core_use_gvfs_helper && !tried_gvfs_helper) {
 			enum gh_client__created ghc;

odb.h

@@ -332,6 +332,18 @@ enum object_info_flags {
 	 */
 	OBJECT_INFO_SECOND_READ = (1 << 4),
 
+	/*
+	 * Only consult the packed object store of a source, skipping its loose
+	 * object store (OBJECT_INFO_SKIP_LOOSE), or vice versa
+	 * (OBJECT_INFO_SKIP_PACKED). These are used by
+	 * odb_read_object_info_extended() to scan the packfiles of all sources
+	 * before consulting any source's loose object store, so that an object
+	 * that resides in an alternate's packfile is not preceded by a spurious
+	 * loose-object lookup on an earlier source.
+	 */
+	OBJECT_INFO_SKIP_LOOSE = (1 << 5),
+	OBJECT_INFO_SKIP_PACKED = (1 << 6),
+
 	/*
 	 * This is meant for bulk prefetching of missing blobs in a partial
 	 * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.

odb/source-files.c

@@ -55,7 +55,11 @@ static int odb_source_files_read_object_info(struct odb_source *source,
 {
 	struct odb_source_files *files = odb_source_files_downcast(source);
 
-	if (!packfile_store_read_object_info(files->packed, oid, oi, flags) ||
+	if (!(flags & OBJECT_INFO_SKIP_PACKED) &&
+	    !packfile_store_read_object_info(files->packed, oid, oi, flags))
+		return 0;
+
+	if (!(flags & OBJECT_INFO_SKIP_LOOSE) &&
 	    !odb_source_read_object_info(&files->loose->base, oid, oi, flags))
 		return 0;
 

t/t5615-alternate-env.sh

@@ -88,4 +88,47 @@ test_expect_success !MINGW 'broken quoting falls back to interpreting raw' '
 	EOF
 '
 
+test_expect_success 'packs across sources are checked before loose objects' '
+	# Regression test for a performance issue in which an object that
+	# resides in an alternate as a packed object caused a spurious loose
+	# object lookup (a filesystem stat) on the main object store before the
+	# alternate packfile was consulted. Reading such an object must resolve
+	# to the alternate packfile, never to a loose copy in the main store.
+	#
+	# Build an alternate whose object "B" is stored as a delta in a
+	# packfile. git deltifies successive versions of a tracked file, so the
+	# older, shorter blob "B" becomes a delta against the newer, longer
+	# blob "O". A loose object has no delta base, so %(deltabase) tells us
+	# which store answered the read: the alternate pack (O) or a loose copy
+	# (the zero oid).
+	git init alt-src &&
+	test_seq 1 200 >alt-src/file &&
+	git -C alt-src add file &&
+	git -C alt-src commit -q -m base &&
+	B=$(git -C alt-src rev-parse HEAD:file) &&
+	git -C alt-src cat-file blob "$B" >b-content &&
+	test_seq 1 210 >alt-src/file &&
+	git -C alt-src add file &&
+	git -C alt-src commit -q -m more &&
+	O=$(git -C alt-src rev-parse HEAD:file) &&
+	git -C alt-src repack -adf --window=10 --depth=50 &&
+
+	# Precondition: in the alternate, B is a delta based on O.
+	echo "$B" >in &&
+	echo "$O" >expect &&
+	git -C alt-src cat-file --batch-check="%(deltabase)" <in >actual &&
+	test_cmp expect actual &&
+
+	# Main repo: write B as a loose object, before any alternate is active.
+	git init main &&
+	git -C main hash-object -w --stdin <b-content >/dev/null &&
+	test -e "main/.git/objects/$(test_oid_to_path "$B")" &&
+
+	# With the alternate active, B must resolve to the alternate packfile
+	# (deltabase O), not to the main store loose copy (deltabase zero oid).
+	GIT_ALTERNATE_OBJECT_DIRECTORIES="$PWD/alt-src/.git/objects" \
+		git -C main cat-file --batch-check="%(deltabase)" <in >actual &&
+	test_cmp expect actual
+'
+
 test_done