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

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jul 23, 2026 at 06:49:43AM -0400, Jeff King wrote:
> On Wed, Jul 22, 2026 at 08:49:50AM -0700, Junio C Hamano wrote:
> 
> > I suspect several of those 'transactions' are actually misspelt
> > 'temporary directories', but I catch your drift.  That said, a
> > redesign like that feels more or less independent of the fix for our
> > immediate performance regression.
> > 
> > After all, didn't Peff show us a case where no odb sources were
> > being flipped in the middle?  Simply setting up one object store to
> > borrow from another via the alternates mechanism demonstrated that
> > checking packs across all object stores before hunting for loose
> > objects in any of them makes a world of difference.
> 
> Yeah, exactly. This is really a regression in alternates performance,
> but it just so happens that the quarantine system is built on top of
> alternates so we noticed it there.
> 
> I'd expect "clone -s / --reference" to have similar problems, and also
> for sites like GitHub and GitLab that make heavy use of alternates for
> object sharing between forks. And those would pay the penalty on just
> about every operation (because we'd expect the alternate to be holding
> most of the objects in those cases).

You're right, I also realized that after sending my mail.

One other angle that Justin and I have been discussing (we were at an
offsite together over the last couple days) was that we can do a small
course correction: instead of handling alternates on the ODB level, we
may be able to start treating alternates as an implementation detail of
it. So both the handling of alternates, but also the handling of the
GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES environment
variables would be moved into the "files" backend itself.

This would solve a bunch of smaller issues that we're currently
grappling with where some of the concepts in Git really want to operate
across all of the alternates:

  - The OBJECT_INFO_SECOND_READ flag can be dropped as it becomes an
    implementation detail.

  - We can fix the performance regression because we can now easily
    reorder access to read via packfiles first across all sub-sources.

  - Commit graphs and bitmap really are a singleton, so loading them via
    multiple sources is awkward.

  - The object storage extension that I've written got quite a bit ugly
    as it wasn't quite clear where exactly to draw the line. Especially
    hadnling the environment variables mentioned above into the "files"
    backend removes one point of friction I encountered.

  - Object database maintenance needs to be aware of the other non-local
    sources.

Also, doing that change isn't as bad as it may sound at first. We'd
still retain the whole `struct odb_source` list because we want to have
them for submodule sources. Furthermore, alternates aren't required for
isolation either as we currently use them via the temporary object
directory. An alternative implementation may use a completely separate
mechanism to achieve write isolation, which is also why we have made the
environment variables pluggable that the `struct odb_transaction` ends
up passing to the child process.

I think overall this could simplify some of the design, and it makes a
bunch of issues that I have been struggling with go away. The devil may
be in the details of course, but I think transitioning towards this
should be doable.

So I'll work towards that goal. I've got a patch series already that
removes all of our calls to `odb_prepare_alternates()` as a first step,
but I'll only send that in two weeks once I'm back in office. I'll then
have a look at how bad the subsequent steps would be.

Thanks!

Patrick