Re: [PATCH v3 1/2] pack-bitmap.c: remove unnecessary "open_pack_index()" calls

[Jeff King <peff@peff.net>]

On Mon, Nov 14, 2022 at 05:14:37PM -0500, Taylor Blau wrote:

> OK, so with 10K packs, we see about a 1.6-fold improvement, which is
> definitely substantial.
> 
> On a fresh clone of git.git, repeating your experiment with only 1K
> packs (which is definitely a number of packs that GitHub sees in
> under-maintained repositories), the runtime goes from 25.3ms -> 20.9ms
> on my machine, or about a 1.2-fold improvement.
> 
> So definitely smaller, but even at 1/10th the number of packs from your
> experiment, still noticeable.

Interesting. I had tried it initially with 1000 and the improvements were
much smaller. I just did it again, though, and got the same 20% speedup.
I'm not sure what I screwed up earlier (I may have been confused by the
timestamp/sorting issue; I only realized it was important midway through
looking into this).

> >   - this probably isn't helping anybody much in the real world, as
> >     evidenced by the contortions I had to go through to set up the
> >     situation (and which would be made much better by repacking, which
> >     would also speed up non-bitmap operations).
> 
> Per above, I'm not sure I totally agree ;-). 1K packs is definitely an
> extreme amount of packs, but not out-of-this-world. It probably would
> show up in carefully-picked graphs, but not in "overall git rev-list
> time" or something as broad/noisy as that.

Yeah, I agree that 1k is a lot more compelling. The big impractical
thing I think is that if the bitmapped pack is older (and it usually
is), then we'd often open all the other packs anyway:

  - if the start of the traversal is in the bitmapped pack, then we
    fruitlessly open each of the others looking for the object (since
    the bitmapped one will come last in the reverse-chronological
    sorting)

  - if it isn't in the bitmapped pack, then we'll end up opening all
    those other packs anyway to fill out the bitmap (since by definition
    it can't be included in the on-disk bitmaps)

So I'd be surprised if it ever mattered in the real world. Though again,
I think the new code is less surprising in general, and could matter if
we changed other things (e.g., if we prioritized lookups in a pack with
a .bitmap).

-Peff