Re: rather slow 'git repack' in 'blob:none' partial clones

From: Jeff King <peff@peff.net>
Date: Apr 14, 2021, 05:14 UTC
Message-ID: <YHZ6JvLBNpZVOqiX@coredump.intra.peff.net>
In-Reply-To: <20210413180552.GI2947267@szeder.dev>

On Tue, Apr 13, 2021 at 08:05:52PM +0200, SZEDER Gábor wrote:

Show 6 quoted lines

> >   [but now ask it to exclude promisor objects, and it's much slower;
> >   this is because is_promisor_object() opens up each tree in the pack in
> >   order to see which "promised" objects it mentions]
> 
> I don't understand this: 'git rev-list --count --all' only counts
> commit objects, so why should it open any trees at all?

Because the promisor code is a bit over-eager. There's actually one small error in what I wrote above. In that particular rev-list, we're not calling is_promisor_object() at all, because we'll already have excluded all the promisor objects by marking them UNINTERESTING and SEEN.

So:

  - in is_promisor_object(), we load the _whole_ list of promisor
    objects, which requires opening trees to find out about referenced
    blobs. In theory it could know that we only care about commits, but
    it's not connected to a particular traversal, so it gets the whole
    list.

  - mark_uninteresting() is the code where we pre-mark the objects from
    the promisor pack as UNINTERESTING, and that was loading all of the
    trees in this case. And it could know that we are not looking at
    --objects, so there's no need to touch trees. But after my patches,
    we do not load the contents of _any_ objects at all in that
    function. We could avoid even creating "struct object" for
    non-commits there, too, but that would imply looking up the type of
    each object (so more CPU, though it would save us some memory when
    we only care about commits). I suspect in practice that most callers
    would generally pass --objects anyway, though (e.g., your original
    pack-objects that started this thread certainly cares about
    non-commits).

Show 5 quoted lines

> > +	/*
> > +	 * yikes, do we really need to parse here? maybe
> 
> Heh, a "yikes" here and a "yuck" in your previous patch...  This issue
> was worth reporting :)

Yeah. I think the client side of a lot of this partial-clone / promisor stuff is not very mature. It's waiting on people to start using it and finding all of these rough edges.

-Peff