Re: [PATCH v2 1/6] odb/source-files: fix performance when reading objects with alternates

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 8247 bytes --]

Hi Patrick,

On Wed, 14 Oct 2026, Patrick Steinhardt wrote:

> With the move towards pluggable object databases we reorganized how we
> access alternates. Before the introduction of pluggability, we were
> reading objects by first scanning through packfiles from all alternates
> before scanning through all loose object directories. With the change
> though, we are now iterating through the sources one by one, checking
> each of them for whether they contain the object either as a packed or
> loose object.
> 
> Checking whether an object exists in packfiles is comparatively cheap
> because the list of packs is cached, and the pack indices are likely to
> be in the page cache. In contrast to that, checking whether an object
> exists in loose format always incurs one stat(3p) syscall. So scanning
> for objects one alternate at a time led to a performance regression in
> in the case where most of the objects are contained in a packfile of an
> alternate, as reported in [1].
> 
> We could fix that issue by introducing another flag that tells the
> backends to only search the optimized storage backend. But that is
> adding more complexity to the object database layers that is ultimately
> only really required because of the way the "files" backend stores its
> data. So for an alternate backend, such new infrastructure would not
> really be a good fit.
> 
> But since then we have moved handling of alternates into the "files"
> backend itself. One of the consequences of this is that this backend is
> now having full control over all of its sub-sources, and that also makes
> it way easier for us to optimize for this access pattern again.
> 
> Do so by first trying to read objects from packfiles located in any of
> its object directories before trying to read loose objects.
> 
> The following benchmark demonstrates that this significantly improves
> performance in some cases. The benchmark uses the Git repository itself
> and sets up its object directory as an alternate, while the primary
> object directory is simply an empty object directory. It then reads all
> commits enumerated via `git rev-list HEAD` with git-cat-file(1):
> 
>   Benchmark 1: git cat-file --batch-check --unordered (rev = HEAD~)
>     Time (mean ± σ):     179.4 ms ±   2.6 ms    [User: 114.6 ms, System: 61.4 ms]
>     Range (min … max):   177.1 ms … 187.7 ms    16 runs
> 
>   Benchmark 2: git cat-file --batch-check --unordered (rev = HEAD)
>     Time (mean ± σ):      95.0 ms ±   1.5 ms    [User: 79.8 ms, System: 15.0 ms]
>     Range (min … max):    93.0 ms …  97.9 ms    30 runs
> 
>   Summary
>     git cat-file --batch-check --unordered (rev = HEAD) ran
>       1.89 ± 0.04 times faster than git cat-file --batch-check --unordered (rev = HEAD~)
> 
> [1]: <CAFXJcxvpKHoVDwE5mBOd=w-A5vPdUmehqr8SHLUD7qv1qB00rA@mail.gmail.com>
> 
> Reported-by: Wolfgang Kritzinger <wkritzinger@atlassian.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Thank you so much for this! This fixes the bug we had to patch
independently in dc305fc33833 (odb: scan all sources’ packfiles before
loose objects), via https://github.com/microsoft/git/pull/975, first
contained in Microsoft Git v2.55.0.vfs.0.7. Would you kindly include (my
colleague) Tyrie Vella's regression test in the next iteration of this
patch series?

-- snip --
From: Tyrie Vella <tyrielv@gmail.com>
Date: Wed, 14 Oct 2026 15:01:45 +0200
Subject: [PATCH] t5615: verify packed alternates precede local loose objects

This regression tests protects the packed-first lookup order required for
decent performance in setups where the majority of Git objects lives in a
separate alternate's packfiles (such as is the case with typical Scalar or
VFS for Git setups).

The test was originally added to Microsoft Git via
https://github.com/microsoft/git/pull/975

Assisted-by: GPT-6.1 Sol
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5615-alternate-env.sh | 43 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 9d6aa2187f2..c982e8e95d2 100755
--- a/t/t5615-alternate-env.sh
+++ b/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
-- snap --

I ran the test on Linux with both SHA-1 and SHA-256. In both formats, the
unfixed base passes the nine existing tests; With vanilla v2.56.0 the
added case fails at its final lookup-order assertion: `%(deltabase)`
returns the zero OID despite the packed-delta precondition passing. Patch
1 (and of course the full patch series) pass the tests.

Ciao,
Johannes

> ---
>  odb/source-files.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/odb/source-files.c b/odb/source-files.c
> index 455f409db0..9268f5aa9a 100644
> --- a/odb/source-files.c
> +++ b/odb/source-files.c
> @@ -426,6 +426,7 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  {
>  	struct odb_source_files *files = odb_source_files_downcast(source);
>  	enum odb_read_status status = ODB_READ_NOT_FOUND;
> +	enum odb_read_status ret;
>  
>  	/*
>  	 * Reading an object may fail even though the object exists, for
> @@ -434,15 +435,15 @@ static enum odb_read_status odb_source_files_read_object_info(struct odb_source
>  	 * prefer the first such error in case multiple reads have failed.
>  	 */
>  	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
> -		enum odb_read_status ret;
> -
>  		ret = odb_source_read_object_info(&dir->packed->base, oid, oi, flags,
>  						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
>  		if (!ret)
>  			return 0;
>  		if (ret != ODB_READ_NOT_FOUND && status == ODB_READ_NOT_FOUND)
>  			status = ret;
> +	}
>  
> +	for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next) {
>  		ret = odb_source_read_object_info(&dir->loose->base, oid, oi, flags,
>  						  status == ODB_READ_NOT_FOUND ? errmsg : NULL);
>  		if (!ret)
> 
> --