On Mon, Nov 23, 2020 at 09:48:22PM -0500, Jeff King wrote: > > I think that we probably could just use ALLOC_GROW() as you suggest. > > Funny enough, reading through GitHub's chat logs, apparently this is > > something that Peff and I talked about. So, 16 probably came from > > alloc_nr(), but we probably stopped short of realizing that we could > > just use ALLOC_GROW as-is. > > That would probably be OK. It's a bit more aggressive, which could > matter if you have a large number of very small bitmaps. My original > goal of the "grow less aggressively" patch was to keep memory usage > down, knowing that I was going to be holding a lot of bitmaps in memory > at once. But even with micro-optimizations like this, it turned out to > be far too big in practice (and hence Stolee's work on top to reduce the > total number we hold at once). Oh, sorry, I was mixing this patch up with patches 6 and 7, which touch buffer_grow(). This is a totally separate spot, and this is a pure bug-fix. I think the main reason we didn't use ALLOC_GROW() here in the beginning is that the ewah code was originally designed to be a separate library (a port of the java ewah library), and didn't depend on Git code. These days we pull in xmalloc, etc, so we should be fine to use ALLOC_GROW(). Likewise... > I think the real test would be measuring the peak heap of the series as > you posted it in v2, and this version replacing this patch (and the > "grow less aggressively" one) with ALLOC_GROW(). On something big, like > repacking all of the torvalds/linux or git/git fork networks. > > If there's no appreciable difference, then definitely I think it's worth > the simplicity of reusing ALLOC_GROW(). All of this is nonsense (though it does apply to the question of using ALLOC_GROW() in bitmap_grow(), via patch 7). We have many fewer ewah bitmaps in memory at one time, so I don't think it's worth micro-managing a few extra bytes of growth. Using ALLOC_GROW() for this case would be fine. -Peff
