Re: [PATCH 07/12] fetch-pack: cache results of for_each_alternate_ref

[Jeff King <peff@peff.net>]

On Wed, Jan 25, 2017 at 11:21:11AM -0800, Junio C Hamano wrote:

> > diff --git a/object.h b/object.h
> > index 614a00675..f52957dcb 100644
> > --- a/object.h
> > +++ b/object.h
> > @@ -29,7 +29,7 @@ struct object_array {
> >  /*
> >   * object flag allocation:
> >   * revision.h:      0---------10                                26
> > - * fetch-pack.c:    0---4
> > + * fetch-pack.c:    0---5
> >   * walker.c:        0-2
> >   * upload-pack.c:       4       11----------------19
> >   * builtin/blame.c:               12-13
> 
> This is a tangent, but I am not sure how much it buys us to keep
> track of the information here in object.h, as all that picture says
> is "revision traversal machinery given by revision.[ch] can never be
> used inside fetch-pack and upload-pack", and that is already said by
> the current picture that says fetch-pack.c uses 0 thru 4, and
> updating it to say that we now use 0 thru 5 would not change the
> conclusion.

I agree that bumping 4 to 5 does not help at all, given the current
allocations. But it could eventually, if another allocation wanted to
pick up 5, and might plausibly be used together with fetch-pack.

The main thing is that we should keep this up to date, and that it
should cause textual conflicts when two topics update the allocation, so
that a human looks at it. I actually think we fail at the latter,
because a change to revision.h to use bit 20 would probably get
auto-resolved against a change to allocate the same bit to blame.c.

Probably a better organization is:

  bit 0: revision.h, fetch-pack.c, walker.c
  bit 1: revision.h, fetch-pack.c, walker.c
  ...
  bit 10: revision.h
  bit 11: upload-pack.c

and so forth. It's more tedious to read and write, but any two changes
to the same bit would be forced to generate a conflict (assuming a
line-oriented merge, of course :) ).

> What I am trying to get at is that we may want to (re)consider how
> we manage these bits.  But that is totally outside the scope of this
> series, and updating the above is the right thing to do here.

Perhaps you meant something like the above when you said this. But what
I'd _really_ love is to stop using global bits entirely, and have some
way to allocate a bitset and efficiently convert "struct object" to an
index into the bitset. Then cleaning up just becomes throwing out your
bitset, and by default operations do not see the bits from other
unrelated operations. That makes it impossible to have bugs like the one
fixed by 5c08dc48a (checkout: clear commit marks after detached-orphan
check, 2011-03-20).

I can't think of a way to do that efficiently besides something like the
commit-slab system, but extended to all objects. The lookups there are
fairly cheap, though it does have worse cache performance. I don't know
if that would matter in practice or not.

But yeah, definitely outside the scope of this series. :)

-Peff