Re: [PATCH] pack-objects: do not reuse packfiles without --delta-base-offset

From: Jeff King <peff@peff.net>
Date: Apr 4, 2014, 23:13 UTC
Message-ID: <20140404231301.GA2528@sigill.intra.peff.net>
In-Reply-To: <xmqqsipsohjz.fsf@gitster.dls.corp.google.com>

On Fri, Apr 04, 2014 at 03:28:48PM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > We could convert OFS_DELTA to REF_DELTA on the fly. That _may_ have a
> > performance impact. Right now, we are basically doing the equivalent of
> > sendfile(), and conversion would involve iterating through each object
> > and examining the header.  I think that's probably not too bad, though.
> > The most expensive part of that, stepping to the next object, requires
> > scanning through the zlib packets, but we should be able to use the
> > revidx to avoid that.
> >
> > I'm not sure it's even worth the code complexity, though. The non-reuse
> > codepath is not that much slower, and it should be extremely rare for a
> > client not to support OFS_DELTA these days.
> 
> OK, together with the fact that only ancient versions of fetcher
> would trigger this "do not reuse" codepath, I agree that we should
> go the simplest route this patch takes.

By the way, we may want to revisit this if we grow more features that do not allow straight byte-for-byte reuse. I am thinking specifically if we grow a packv4-like representation for an object, and we plan to convert on-the-fly to existing packv2 clients. But I think the sensible steps for that are:

  1. If we have v4 on disk and are outputting v2, add this case to the
     "can_reuse" function I just added. I.e., start out correct, and
     turn off the optimization.

  2. Experiment with on-the-fly conversion. It may be that the
     conversion is so expensive that the reuse optimization gets lost in
     the noise. Or maybe we can reclaim most of the advantage of the
     reuse code path, and it is worth going object-by-object and
     converting. But we won't know until we can measure.

-Peff