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

[Junio C Hamano <gitster@pobox.com>]

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."