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

On Wed, Jul 13, 2016 at 01:52:17PM +0300, Kirill Smelkov wrote:

Right. So I think your scheme is fine as long as you are doing your regular "pack all into one" repacks with a real walk, and then "branching" off of that with one-off bitmap-computed packs into files (even if you later take a bunch of those files and pull them into a single bitmapped, as long as that final "all into one" does the walk).

Or I guess another way to think about it would be that if you're computing bitmaps, you'd want to do the actual traversal.

The problem with the existing pack-reuse code is that it doesn't kick in often enough. I think it looks to see that the client wants some percentage of the pack (e.g., 90%), and then just sends the whole beginning. This works especially badly if you have a bunch of related repositories packed together (e.g., all of the forks of torvalds/linux on GitHub), because you'll never hit 90% of that big pack; it has too much unrelated cruft, even if most of the stuff you want _is_ at the beginning. And "percent of pack" is not really a useful metric anyway.

So the better scheme is more like:

  1. Generate the bitmap of objects to send using reachability bitmaps.

  2. Do a quick scan of their content in the packfile to see which can
     be reused verbatim. If they're base objects, we can send them
     as-is. If they're deltas, we can send them if their base is going
     to be sent. This fills in another bitmap of "reusable" objects.

     After a long string of unusable objects, you can give up and set
     the rest of the bitmap to zeroes.

  3. Walk the "reuse" bitmap and send out the objects more-or-less
     verbatim. You do have make adjustments to delta-base-offsets for
     any "holes" (so if an object's entry says "my base is 500 bytes
     back", but you omitted some objects in between, you have to adjust
     that offset).

The upside is that you can send out those objects without even making a "struct object_entry" for them, which drastically reduces the memory requirements for serving a clone. Any objects which didn't get marked for reuse just get handled in the usual way (so stuff that was not close by in the pack, or stuff that was pushed since your last big repack).

The downside is that because those objects aren't in our normal packing list, they're not available as delta bases for the new objects we _do_ send. So it can make the resulting pack a little bit bigger.

Correct, the defaults are for JGit compatibility. If you are not using JGit, you should have it on all the time. We went with the conservative default, but as more people using regular Git bitmaps, it would probably be good to make them less arcane and confusing to use.

Right, that makes sense. Even the pack-to-disk code invoked by git-repack is not foolproof for blob corruptions. It is only checking a crc, not the full sha1. So it's better than nothing, but not as careful as a full index-pack.

In my opinion, yes. If we had any debugging option, it should be something like "core.usebitmaps", to tell _all_ of git to pretend that bitmaps don't exist (right now only pack-objects respects it, but we could be using them to optimize more traversals).

I think we can give more motivation and context here with some of the bits that have come out in our discussion. Like:

  The reason for this split is that pack-objects tries to determine how
  "careful" it should be based on whether we are packing to disk or to
  stdout. Packing to disk implies "git repack", and that we will likely
  delete the old packs after finishing. We want to be more careful (so
  as not to carry forward a corruption, and to generate a more optimal
  pack), and we presumably run less frequently and can afford extra CPU.
  Whereas packing to stdout implies serving a remote via "git fetch" or
  "git push". This happens more frequently (e.g., a server handling many
  fetching clients), and we assume the receiving end takes more
  responsibility for verifying the data.

  But this isn't always the case. One might want to generate on-disk
  packfiles for a specialized object transfer. Just using "--stdout" and
  writing to a file is not optimal, as it will not generate the matching
  pack index.

  So it would be useful to have some way of overriding this heuristic:
  to tell pack-objects that even though it should generate on-disk
  files, it is still OK to use the reachability bitmaps to do the
  traversal.

So one reason is that it is not currently possible with the implementation. But I think it also gets to the above bit about "optimal" packs. We do not want to generate bitmaps off of bitmaps, because we lose information about the write order. That's probably worth mentioning here.

Yes, that's right. We definitely want pack-reuse off for this case.

Ah, this kind of covers the bits I talked about above. I think it makes more sense to introduce them as part of the motivation, though, rather than as a note here.

This part is important, though. Basically the reason we respect the command-line option is that we know that git-repack would never set it explicitly, so it is the hint that pack-objects can use to know which case we are serving: a careful repack of our data, or just extraction of some objects.

Hrm. Adding entries from the bitmap should ideally be very fast, but here we're introducing extra lookups in the object database. I guess it only kicks in when --local is given, though, which most bitmap-using paths would not do.

But is this check enough? The non-bitmap code path calls want_object_in_pack, which checks not only loose objects, but also non-local packs, and .keep.

Those don't kick in for your use case. I wonder if we should simply have something like:

  if (local || ignore_packed_keep)
	use_bitmap_index = 0;

and just skip bitmaps for those cases. That's easy to reason about, and I don't think anybody would care (your use case does not, and the repack use case is already not going to use bitmaps).

And that local/keep logic above would just become "hard" reasons included here.

-Peff