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

From: Duy Nguyen <pclouds@gmail.com>
To: Jeff King <peff@peff.net>, Nicolas Pitre <nico@fluxnic.net>
Cc: Jonathan Tan <jonathantanmy@google.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 2/2] index-pack: prefetch missing REF_DELTA bases
Date: Sat, 18 May 2019 18:39:32 +0700	[thread overview]
Message-ID: <CACsJy8AkhKX57RYL1Z+HZHqKbAKKOcLoRkgwg8bSnk+DW2+Nmg@mail.gmail.com> (raw)
In-Reply-To: <20190517085509.GA20039@sigill.intra.peff.net>

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.

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

> -- >8 --
> Subject: [PATCH] index-pack: describe an implication of our thin resolving
>
> After digging into the delta resolution code, I discovered a surprising
> (to me, anyway) implication of our strategy: we could never find a
> non-thin delta with a thin delta as its base. This is OK because
> pack-objects will never produce such a combination, because....?
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/index-pack.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index ccf4eb7e9b..f40f4560d4 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1224,6 +1224,13 @@ static void resolve_deltas(void)
>   * Third pass:
>   * - append objects to convert thin pack to full pack if required
>   * - write the final pack hash
> + *
> + * Note that we assume all deltas at this phase are thin. We take only a
> + * single pass over the unresolved objects, and we look for bases only
> + * in our set of already-existing objects, _not_ other objects within this
> + * pack. This means that we would never find an object A stored as a delta
> + * against another object B in this pack, when B is a thin delta against a base
> + * not in the pack.
>   */
>  static void fix_unresolved_deltas(struct hashfile *f);
>  static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned char *pack_hash)
> --
> 2.22.0.rc0.544.g1eb4087842
>

-- 
Duy