Re: [PATCH v2 02/15] pack-bitmap: fix leak of haves/wants object lists

From: Jeff King <peff@peff.net>
Date: Feb 18, 2020, 20:02 UTC
Message-ID: <20200218200219.GC21774@coredump.intra.peff.net>
In-Reply-To: <28c2f199-e9eb-4405-9044-92a0af7632ec@gmail.com>

On Tue, Feb 18, 2020 at 12:58:10PM -0500, Derrick Stolee wrote:

Show 10 quoted lines

> On 2/14/2020 1:22 PM, Jeff King wrote:
> > When we do a bitmap-aware revision traversal, we create an object_list
> > for each of the "haves" and "wants" tips. After creating the result
> > bitmaps these are no longer needed or used, but we never free the list
> > memory.
> 
> It's surprising that we got away with this for so long. Is it
> possible that this loop of freeing memory could provide significant
> performance drawbacks? I'm assuming that the free() loop is much
> faster than the malloc() loop, so the impact is negligible.

I don't think it's surprising. We're only talking about leaking hundreds or perhaps thousands of structs. So maybe a few kilobytes at most. And we'd typically only run one such bitmap traversal per program.

Meanwhile rev-list is generally storing that same set of objects in a pending array, and I think we don't generally clean up after it (nor is there even a convenient function to do so).

So I think it's sort of a drop in the bucket of Git's "eh, this pretty much lasts about the whole program anyway" leaks. I don't think there's much performance difference either way in freeing or not. It's mostly just a cleanliness thing. (I do one day dream of being able to run the test suite through a leakchecker, but we're still a ways off, I think).

-Peff