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

From: Jeff King <peff@peff.net>
Date: Nov 14, 2022, 22:31 UTC
Message-ID: <Y3LBtl+x2rCzsoIh@coredump.intra.peff.net>
In-Reply-To: <Y3K9zVx7L9K85NrE@nand.local>

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

Show 10 quoted lines

> 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).

Show 9 quoted lines

> >   - 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