Re: [PATCH 2/2] repack: repack promisor objects if -a or -A is set

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 7, 2018, 23:23 UTC
Message-ID: <20180807232304.181764-1-jonathantanmy@google.com>
In-Reply-To: <xmqqzhxxki84.fsf@gitster-ct.c.googlers.com>

Show 5 quoted lines

> for_each_object_in_pack() is a fine way to make sure that you list
> everythning in a pack, but I suspect it is a horrible way to feed a
> list of objects to pack-objects, as it goes by the .idx order, which
> is by definition a way to enumerate objects in a randomly useless
> order.

That's true.

> Do we already have an access to the in-core reverse index for the
> pack at this point in the code?

As far as I can tell, no. These patches construct the list of promisor objects in repack.c (which does not use the revindex at all), to be processed by pack-objects in a different process (which does use the revindex in reuse-delta mode, which is the default). I guess that we could have access to the revindex if we were to libify pack-objects and run it in the same process as repack.c or if we were to add a special mode to pack-objects that reads for itself the list of all the promisor objects.

Show 9 quoted lines

> If so, we can enumerate the objects
> in the offset order without incurring a lot of cost (building the
> in-core revindex is the more expensive part).  When writing a pack,
> we try to make sure that related objects are written out close to
> each other [*1*], so listing them in the offset order (with made-up
> pathname information to _force_ that objects that live close
> together in the original pack are grouped together by getting
> similar names) might give us a better than horrible deltification.
> I dunno.

Before I write the NEEDSWORK comment, just to clarify - do you mean better than horrible object locality? I think deltification still occurs because pack-objects sorts the input when doing the windowed deltification, for example:

  git rev-parse HEAD:fetch-pack.c HEAD HEAD^ HEAD^^ \
	HEAD^^^ v2.17.0:fetch-pack.c | git pack-objects --window=2 abc

produces a packfile with a delta, as checked by `verify-pack -v`.

> 	Side note *1*: "related" has two axis, and one is relevant
> 	for better deltification, while the other is not useful.
> 	The one I have in mind here is that we write set of blobs
> 	that belong to the same "delta family" together.

As far as I can see, they do not need to be adjacent in the packfile to have one be a delta against the other.

> I do not think such a "make it a bit better than horrible" is necessary
> in an initial version, but it deserves an in-code NEEDSWORK comment
> to remind us that we need to measure and experiment.

OK, I'll do this in the next reroll.