Subject: Re: [PATCH 01/23] ewah/ewah_bitmap.c: grow buffer past 1

From: Taylor Blau <[email-masked]>
Date: Tue, 1 Dec 2020 17:56:37 -0500

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