On Mon, Nov 23, 2020 at 09:51:41PM -0500, Jeff King wrote: > 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). You and I timed this a week or two ago, but I only just returned to this topic today. Switching to ALLOC_GROW() doesn't affect the final memory usage at all, so I changed patch 7 up to use that instead of more or less open-coding alloc_nr(). > -Peff Thanks, Taylor
