Re: [PATCH] pack-bitmaps: plug memory leak, fix allocation size for recent_bitmaps

On Tue, May 19, 2015 at 12:45:46PM -0700, Junio C Hamano wrote:

quoted

I looked over the function one more time to make sure it is the function
that is wrong, and not my suggestion. :) The current code seems pretty
obviously wrong.

I actually cannot guess what the current code is trying to do.  Was
it an attempt to cache that many entries, but instead allocated and
discarded the space it tried to use as a cache every time?

Sort of. There are two caches at work.

The bitmaps on disk may be stored as XORs against nearby bitmaps up to
MAX_XOR_OFFSET slots away. So our goal is to reconstruct the actual
bitmaps and put them in our cache for later use. That's actually done by
store_bitmap(), which puts them into a hash table indexed by sha1.

But because we get the XOR base as an offset, we also need to be able to
quickly say "what was the bitmap that was N slots ago?", and the hash
cannot answer that quickly. So we keep a sliding window of the last
MAX_XOR_OFFSET bitmaps (pointing to the cached bitmaps stored in the
hash), and then we can index that directly by offset. And we can reuse
the array as a circular buffer (notice we always index it modulo the max
offset).

So you can think of the recent_bitmaps as an auxiliary index into the
bitmaps already cached by store_bitmap(). We don't need it after the xor
reconstruction (actually I think we don't do the xor reconstruction
here, but instead retain a pointer to the xor base and lazily do it, but
the point is that we've created that pointer).

-Peff