Re: [PATCH] pack-objects: Use reachability bitmap index when generating non-stdout pack too

From: Jeff King <peff@peff.net>
Date: Jul 7, 2016, 20:52 UTC
Message-ID: <20160707205223.GA11753@sigill.intra.peff.net>
In-Reply-To: <20160707190917.20011-1-kirr@nexedi.com>

On Thu, Jul 07, 2016 at 10:09:17PM +0300, Kirill Smelkov wrote:

Show 7 quoted lines

> Starting from 6b8fda2d (pack-objects: use bitmaps when packing objects)
> if a repository has bitmap index, pack-objects can nicely speedup
> "Counting objects" graph traversal phase. That however was done only for
> case when resultant pack is sent to stdout, not written into a file.
> 
> We can teach pack-objects to use bitmap index for initial object
> counting phase when generating resultant pack file too:

I'm not sure this is a good idea in general. When bitmaps are in use, we cannot fill out the details in the object-packing list as thoroughly. In particular:

  - we will not compute the same write order (which is based on
    traversal order), leading to packs that have less efficient cache
    characteristics

  - we don't learn about the filename of trees and blobs, which is going
    to make the delta step much less efficient. This might be mitigated
    by turning on the bitmap name-hash cache; I don't recall how much
    detail pack-objects needs on the name (i.e., the full name versus
    just the hash).

There may be other subtle things, too. The general idea of tying the bitmap use to pack_to_stdout is that you _do_ want to use it for serving fetches and pushes, but for a full on-disk repack via gc, it's more important to generate a good pack.

Your use case:

> git-backup extracts many packs on repositories restoration. That was my
> initial motivation for the patch.

Seems to be somewhere in between. I'm not sure I understand how you're invoking pack-objects here, but I wonder if you should be using "pack-objects --stdout" yourself.

But even if it is the right thing for your use case to be using bitmaps to generate an on-disk bitmap, I think we should be making sure it _doesn't_ trigger when doing a normal repack.

-Peff