Re: [PATCH 2/2] index-pack: prefetch missing REF_DELTA bases

From: Nicolas Pitre <nico@fluxnic.net>
Date: May 20, 2019, 23:04 UTC
Message-ID: <nycvar.YSQ.7.76.1905201803520.1558@knanqh.ubzr>
In-Reply-To: <CACsJy8AkhKX57RYL1Z+HZHqKbAKKOcLoRkgwg8bSnk+DW2+Nmg@mail.gmail.com>

On Sat, 18 May 2019, Duy Nguyen wrote:

Show 20 quoted lines

> On Fri, May 17, 2019 at 3:55 PM Jeff King <peff@peff.net> wrote:
> >
> > On Fri, May 17, 2019 at 02:20:42PM +0700, Duy Nguyen wrote:
> >
> > > On Fri, May 17, 2019 at 12:35 PM Jeff King <peff@peff.net> wrote:
> > > > As it turns out, index-pack does not handle these complicated cases at
> > > > all! In the final fix_unresolved_deltas(), we are only looking for thin
> > > > deltas, and anything that was not yet resolved is assumed to be a thin
> > > > object. In many of these cases we _could_ resolve them if we tried
> > > > harder. But that is good news for us because it means that these
> > > > expectations about delta relationships are already there, and the
> > > > pre-fetch done by your patch should always be 100% correct and
> > > > efficient.
> > >
> > > Is it worth keeping some of these notes in the "third pass" comment
> > > block in index-pack.c to help future readers?
> >
> > Perhaps. I started on the patch below, but I had trouble in the commit
> > message. I couldn't find the part of the code that explains why we would
> > never produce this combination, though empirically we do not.

Good question indeed.

> That still has some value even if your commit ends up with a question
> mark. There's not much to dig out of 636171cb80 (make index-pack able
> to complete thin packs., 2006-10-25). Adding Nico, maybe he still
> remembers...

What about this comment in fix_unresolved_deltas():

        /*
         * Since many unresolved deltas may well be themselves base objects
         * for more unresolved deltas, we really want to include the
         * smallest number of base objects that would cover as much delta
         * as possible by picking the
         * trunc deltas first, allowing for other deltas to resolve without
         * additional base objects.  Since most base objects are to be found
         * before deltas depending on them, a good heuristic is to start
         * resolving deltas in the same order as their position in the pack.
         */

Doesn't that cover it?

In pack-objects, another comment says:

 * Depth value does not matter - find_deltas() will
 * never consider reused delta as the base object to
 * deltify other objects against, in order to avoid
 * circular deltas.

Sorry if I'm not of any help here. Although I used to have my brain wrapped around this code pretty tightly, it's been quite a while, and the code did change as well since then.

Nicolas