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

From: Jeff King <peff@peff.net>
Date: May 19, 2015, 21:19 UTC
Message-ID: <20150519211952.GA23904@peff.net>
In-Reply-To: <xmqqoalg73md.fsf@gitster.dls.corp.google.com>

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

Show 7 quoted lines

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