Performance regression in connectivity check during receive-pack (git 2.54)

Performance regression in connectivity check during receive-pack (git 2.54)

[Wolfgang Kritzinger]

Hi!

I'm a developer working on the on-premise version of Bitbucket at
Atlassian.

We noticed pushes to Bitbucket got much slower after upgrading Git
on the server side from 2.50 to 2.54. I traced the slow part to
the connectivity check that receive-pack runs:

git rev-list --objects --stdin --not --exclude-hidden=receive --all \
--quiet --alternate-refs --progress=Checking connectivity

`strace` shows that after a push, 2.54 does a failing open() of
of numerous loose objects -- once in the quarantine (incoming)
directory and once in the main object store -- before finding it
in a pack:

openat(".../objects/tmp_objdir-incoming-XXXX/ed/58..", O_RDONLY) = ENOENT
openat(".../objects/ed/58..", O_RDONLY) = ENOENT

2.50 does not do this. In most customer deployments of Bitbucket,
the Git data lives on an NFS share. The extra latency on NFS makes
this process of checking for non-existent loose objects take too
long, the push essentially hangs at the "Checking connectivity" step.

I believe this new behavior was introduced in the recent object
database rework. After using bisect, I belive the problem can be
traced back to commit 8384cbcb4c.

I don't know the codebase well, but from what I can see is that
the order in which objects are looked up in object databases
changed.

Assuming there are two object databases configured (Main repo,
and the quarantine directory, for example), the lookup order used
to be:

1. _quarantine dir_ packs
2. _main dir_ packs
3. _quarantine dir_ loose objects
4. _main dir_ loose objects

With Git 2.54, the order appears to have changed to:

1. _quarantine dir_ packs
2. _quarantine dir_ loose objects
3. _main dir_ packs
4. _main dir_ loose objects

In my testing, within a well-packed repo, Git 2.50 actually never
performed a loose object lookup.

The current design seems to iterate over the configured object
databases and perform a pack and loose object lookup for each.

Is there a way to avoid these costly loose object lookups?

Re: Performance regression in connectivity check during receive-pack (git 2.54)

[Jeff King]

On Tue, Jul 21, 2026 at 03:17:23PM +1200, Wolfgang Kritzinger wrote:

> `strace` shows that after a push, 2.54 does a failing open() of
> of numerous loose objects -- once in the quarantine (incoming)
> directory and once in the main object store -- before finding it
> in a pack:
> 
> openat(".../objects/tmp_objdir-incoming-XXXX/ed/58..", O_RDONLY) = ENOENT
> openat(".../objects/ed/58..", O_RDONLY) = ENOENT

Interesting. Here's a smaller reproduction recipe that shows the issue:

  # clone of git.git, or any other non-trivial repo; it should be mostly
  # packed
  src=/path/to/git

  git init empty
  export GIT_ALTERNATE_OBJECT_DIRECTORIES=$src/.git/objects
  strace -fe openat \
    git -C empty rev-list --objects $(git -C $src rev-parse HEAD) >/dev/null

In v2.50, we see almost no loose object open calls, because we check the
pack first. But in v2.54, we see tons of them.

> 2.50 does not do this. In most customer deployments of Bitbucket,
> the Git data lives on an NFS share. The extra latency on NFS makes
> this process of checking for non-existent loose objects take too
> long, the push essentially hangs at the "Checking connectivity" step.

Yeah, I can imagine. But even on a fast filesystem, we definitely want
to avoid all of those syscalls. Replacing "strace" above with a timing
harness, even on a system with fast syscalls and a warm cache, the v2.54
version is ~12% slower.

> I believe this new behavior was introduced in the recent object
> database rework. After using bisect, I belive the problem can be
> traced back to commit 8384cbcb4c.

Hmm, my bisect ended up at a593373b09 (packfile: refactor
`find_pack_entry()` to work on the packfile store, 2026-01-09), which is
nearby. I'm not sure if it might depend on other factors (e.g., presence
of commit graphs, midx, etc) or my reproduction is not exactly like
yours, or if one of us messed up bisection.

+cc Patrick as the author of both commits.

> I don't know the codebase well, but from what I can see is that
> the order in which objects are looked up in object databases
> changed.
> 
> Assuming there are two object databases configured (Main repo,
> and the quarantine directory, for example), the lookup order used
> to be:
> 
> 1. _quarantine dir_ packs
> 2. _main dir_ packs
> 3. _quarantine dir_ loose objects
> 4. _main dir_ loose objects
> 
> With Git 2.54, the order appears to have changed to:
> 
> 1. _quarantine dir_ packs
> 2. _quarantine dir_ loose objects
> 3. _main dir_ packs
> 4. _main dir_ loose objects
> 
> In my testing, within a well-packed repo, Git 2.50 actually never
> performed a loose object lookup.

Yeah, and that type of regression makes sense for what a593373b09 was
trying to do. But I think the v2.54 behavior is wrong. We should check
all packs before any loose objects.

I'm not sure of the correct fix. This is working against the whole "odb
sources are independent and abstract" refactoring that a593373b09 was
going for. But I think it's an important optimization. I guess the
abstract version would be that each source has "fast" and "slow" lookups
or something like that, and we check all fast ones before slow ones. But
that is pretty gross.

I'll leave it to Patrick to ponder further. I haven't really been paying
a lot of attention to the odb refactoring.

-Peff

Re: Performance regression in connectivity check during receive-pack (git 2.54)

[Taylor Blau]

On Mon, Jul 20, 2026 at 11:57:33PM -0400, Jeff King wrote:
> On Tue, Jul 21, 2026 at 03:17:23PM +1200, Wolfgang Kritzinger wrote:
>
> > `strace` shows that after a push, 2.54 does a failing open() of
> > of numerous loose objects -- once in the quarantine (incoming)
> > directory and once in the main object store -- before finding it
> > in a pack:
> >
> > openat(".../objects/tmp_objdir-incoming-XXXX/ed/58..", O_RDONLY) = ENOENT
> > openat(".../objects/ed/58..", O_RDONLY) = ENOENT
>
> Interesting. Here's a smaller reproduction recipe that shows the issue:
>
>   # clone of git.git, or any other non-trivial repo; it should be mostly
>   # packed
>   src=/path/to/git
>
>   git init empty
>   export GIT_ALTERNATE_OBJECT_DIRECTORIES=$src/.git/objects
>   strace -fe openat \
>     git -C empty rev-list --objects $(git -C $src rev-parse HEAD) >/dev/null
>
> In v2.50, we see almost no loose object open calls, because we check the
> pack first. But in v2.54, we see tons of them.
>
> > 2.50 does not do this. In most customer deployments of Bitbucket,
> > the Git data lives on an NFS share. The extra latency on NFS makes
> > this process of checking for non-existent loose objects take too
> > long, the push essentially hangs at the "Checking connectivity" step.
>
> Yeah, I can imagine. But even on a fast filesystem, we definitely want
> to avoid all of those syscalls. Replacing "strace" above with a timing
> harness, even on a system with fast syscalls and a warm cache, the v2.54
> version is ~12% slower.
>
> > I believe this new behavior was introduced in the recent object
> > database rework. After using bisect, I belive the problem can be
> > traced back to commit 8384cbcb4c.


> Hmm, my bisect ended up at a593373b09 (packfile: refactor
> `find_pack_entry()` to work on the packfile store, 2026-01-09), which is
> nearby. I'm not sure if it might depend on other factors (e.g., presence
> of commit graphs, midx, etc) or my reproduction is not exactly like
> yours, or if one of us messed up bisection.
>
> +cc Patrick as the author of both commits.

I think that both bisection results are equally valid for different
reasons.

Before 8384cbcb4c, 'find_pack_entry()' did

    packfile_store_prepare(r->objects->sources->packfiles);

, then tried each of the stores in order to first see if (1) a MIDX was
available to locate the object in some pack, or (2) failing that, if
there exists some non-MIDX'd pack which could do the same.

Worth noting is that 'packfile_store_prepare()' effectively did:

    for (s = store->source->odb->sources; s; s = s->next) {
        prepare_multi_pack_index_one(s);
        prepare_packed_git_one(s);
    }

Thus preparing the first store also prepared every alternate store,
enabling 'find_pack_entry()' to search through all store's MIDX and
pack lists/sources.

8384cbcb4c changes this such that 'packfile_store_prepare()' now only
prepares its owning source:

    prepare_multi_pack_index_one(store->source);
    prepare_packed_git_one(store->source);

, which is reasonable, but 'find_pack_entry()' still loops over all
sources starting from 'r->objects->sources' and calls the function
'packfile_store_prepare()'. But! It calls that function over the same
argument each time, like so:

    for (source = r->objects->sources; source; source = source->next) {
        packfile_store_prepare(r->objects->sources->packfiles);
        if (source->midx && fill_midx_entry(source->midx, oid, e))
            return 1;
    }

So we never prepare the packfile store from other sources!

In Wolfgang's case, if we have an quarantine store followed by the main
object store, our lookup order will be:

 1. prepare the quarantine object store
 2. search packs in the quarantine object store
 3. search packs in the main object store (which will fail, since this
    list is guaranteed to be empty since we never called
    'packfile_store_prepare()')
 4. search loose objects in the quarantine object store
 5. search loose objects in the main object store
 6. haven't found anything, so we must reprepare
 7. search packs in the main object store, which will now succeed, as
    the previous reprepare called 'packfile_store_prepare()' on the main
    object store's packfile source.

Commit a593373b09 changes things, since it makes 'find_pack_entry()' no
longer operate over the entire repository, but over a single store.
Before searching that store, it prepares it, like so:

    static int find_pack_entry(struct packfile_store *store,
                               const struct object_id *oid,
                               sturct pack_entry *e)
    {
        struct packfile_list_entry *l;

        packfile_store_prepare(store);
        if (store->source->midx && fill_midx_entry(...))
            return 1;

        for (l = store->packs.head; l; l = l->next) {
            struct packed_git *p = l->pack;
            if (!p->multi_pack_index && fill_pack_entry(oid, e, p)) {
                /* ... */
                return 1;
            }
        }

        return 0;
    }

So commit a593373b09 indeed squashes the bug introduced by 8384cbcb4c,
and when lookup reaches the main store, it prepares the main store
correctly.

But a593373b09 also changes the lookup order, because the caller in
'do_oid_object_info_extended()` already loops over sources!

    static int do_oid_object_info_extended(struct object_database *odb,
                                           const struct object_id *oid,
                                           struct object_info *oi, unsigned flags)
    {
        /* replace objects, cached lookups, etc., ... */

        odb_prepare_alterantes(odb);

        while (1) {
            struct odb_source *source;

            for (source = odb->sources; source; source = source->next) {
                if (!packfile_store_read_object_info(source->packfiles,
                                                     real, oi, flags) ||
                    !odb_source_loose_read_object_info(source, real, oi,
                                                       flags))
                    return 0;
            }
        }
    }

Before a593373b09, that call to 'packfile_store_read_object_info()'
looped over all sources, since it still called 'find_pack_entry()'.

In other words, prior to a593373b09, the lookup proceeded like so:

 1. search packfiles in quarantine
 2. search packfiles in the main object store
 3. search loose objects in quarantine
 4. search loose objects in the main object store

But a593373b09 changes that to instead proceed store-by-store, as
follows:

 1. search packfiles in quarantine
 2. search loose objects in quarantine
 3. search packfiles in the main object store
 4. search loose objects in the main object store

So even with all object sources prepared, every object found in a later
source pays a failed loose object lookup in an earlier one, which I
believe matches what Peff strace'd above.

So both bisections make sense. If the later store has not been prepared
yet, commit 8384cbcb4c is where Git first fails to see its packs and
falls through to loose object checks. If the stores are already
prepared, that problem does not show up, and a593373b09 is where Git
first starts checking loose objects in an earlier source before looking
in a later source's packs.

I think that something like the following (untested) would fix the
immediate issue:

--- 8< ---
diff --git a/odb.c b/odb.c
index cf6e7938c0..aeb2915f0f 100644
--- a/odb.c
+++ b/odb.c
@@ -568,9 +568,28 @@ static int do_oid_object_info_extended(struct object_database *odb,
 	while (1) {
 		struct odb_source *source;

-		for (source = odb->sources; source; source = source->next)
-			if (!odb_source_read_object_info(source, real, oi, flags))
+		/*
+		 * Check all packed sources before trying loose ones. A loose
+		 * miss requires a filesystem lookup, and receive-pack's
+		 * quarantine source makes the main object directory an
+		 * alternate.
+		 */
+		for (source = odb->sources; source; source = source->next) {
+			struct odb_source_files *files =
+				odb_source_files_downcast(source);
+
+			if (!odb_source_read_object_info(&files->packed->base,
+							 real, oi, flags))
 				return 0;
+		}
+		for (source = odb->sources; source; source = source->next) {
+			struct odb_source_files *files =
+				odb_source_files_downcast(source);
+
+			if (!odb_source_read_object_info(&files->loose->base,
+							 real, oi, flags))
+				return 0;
+		}

 		/*
 		 * When the object hasn't been found we try a second read and
--- >8 ---

But...

> I'm not sure of the correct fix. This is working against the whole "odb
> sources are independent and abstract" refactoring that a593373b09 was
> going for. But I think it's an important optimization. I guess the
> abstract version would be that each source has "fast" and "slow" lookups
> or something like that, and we check all fast ones before slow ones. But
> that is pretty gross.

...that fix is breaking the very abstraction that the pluggable-ODB
effort is trying to create in the first place, at least in my
understanding of the project's goals.

> I'll leave it to Patrick to ponder further. I haven't really been paying
> a lot of attention to the odb refactoring.

I am genuinely not sure what the right path forward here is, given that
I do not have a super firm understanding of all of the refactoring that
has taken place here. I would be likewise eager to hear from Patrick or
others with thoughts on how to resolve this.

Thanks,
Taylor

Re: Performance regression in connectivity check during receive-pack (git 2.54)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, and that type of regression makes sense for what a593373b09 was
> trying to do. But I think the v2.54 behavior is wrong. We should check
> all packs before any loose objects.
>
> I'm not sure of the correct fix. This is working against the whole "odb
> sources are independent and abstract" refactoring that a593373b09 was
> going for. But I think it's an important optimization. I guess the
> abstract version would be that each source has "fast" and "slow" lookups
> or something like that, and we check all fast ones before slow ones. But
> that is pretty gross.
>
> I'll leave it to Patrick to ponder further. I haven't really been paying
> a lot of attention to the odb refactoring.

I think checking the fast sources before the slow ones is probably
the best we can do if we want to retain the 'each odb source is an
opaque object' abstraction.

Stepping back a bit, the 'rev-list' command used for the
connectivity check is curious in multiple aspects.

 * On the surface, it looks as if the caller wants an enumeration of
   all objects that appear in the range.  However, the caller is not
   interested in the actual list of objects.  Instead, they are
   interested only in a single bit: whether the traversal succeeds
   or dies due to a missing object.  This is because the traversal
   determines whether we need to fetch, or whether we are already up
   to date, to decide whether the proposed 'fetch' is a no-op.  The
   positive ends of the traversal represent what we are about to
   fetch; if we already have all the objects needed to reach those
   tips in our repository, we can do without actually downloading
   anything [*].

 * A false positive answer to the question "does the traversal die
   due to a missing object?" does not affect correctness, as this is
   merely an optimization to save downloads (though a false negative
   is unacceptable).

Given this non-standard use of the command, we can pass
application-specific cues (such as "we are doing this traversal for
a connectivity check") down to the machinery as a hint to help it
optimize its operation, and I suspect that such a hint might have
value.

For example, we could enumerate all loose objects in the loose
object store using 256 opendir() and readdir() calls for about
10,000 files (since once you have more than 6,700 loose objects,
auto-gc would pack them) and store them in an in-core table [**].
This would enable us to say "the object with that name does not
exist here" without running lstat() at all.  I wonder how many
lstat() calls we would need to save for such a scheme to pay off.

There may be other highly application-specific optimization
opportunities, as utilizing revision traversal for
connectivity checking has peculiar correctness requirements
that differ from the normal use of the API.

[Footnote]

 * It follows that in a lazily cloned repository with promisor
   remotes, the traversal could download everything needed as it
   goes, only to conclude: "No need for the main fetch; we have
   everything we need."  I would expect this to be a fairly slow
   process that defeats the entire reason we have this connectivity
   check up front as an optimization.  While I have not checked, I
   believe the actual code prevents this either by skipping the
   connectivity check altogether, or by instructing the connectivity
   checker to treat promised (but not immediately available) objects
   as missing and abort.  But my point is that theoretically one
   does not even need 'git fetch' in a lazily cloned repository.  It
   is sufficient to use 'git ls-remote' to determine the tips of
   remote refs, and run 'rev-list' to fill the range.

** If in-core memory pressure is a concern, we could use a Bloom
   filter, as we only need to know "the object is definitely not
   here" and can tolerate "that object might be here, but we are not
   certain."