Re: [PATCH] delta-islands: free island-related data after use

From: Jeff King <peff@peff.net>
Date: Nov 16, 2022, 18:44 UTC
Message-ID: <Y3UvhsRC9uCXJJ8P@coredump.intra.peff.net>
In-Reply-To: <20221116105013.1777440-1-e@80x24.org>

On Wed, Nov 16, 2022 at 10:50:13AM +0000, Eric Wong wrote:

Show 7 quoted lines

> On my use case involving 771 islands of Linux on kernel.org,
> this reduces memory usage by around 25MB.  The bulk of that
> comes from free_remote_islands, since free_island_regexes only
> saves around 40k.
> 
> This memory is saved early in the memory-intensive pack process,
> making it available for the remainder of the long process.

I think this works and is a reasonable thing to do. The non-obvious question is whether the island data is ever used later in the process. Certainly the island bitmaps themselves are, but the initial mapping of refs to islands doesn't.

I do agree with Ævar that it would be a lot easier to confirm that if these variables were given a more appropriate scope (this was mostly just laziness on my part writing the initial delta-island code; so much of it has to stick around for the whole process and I didn't distinguish between the two).

>   Will try to hunt down more memory savings in the nearish future.

Yes, you've probably noticed that pack-objects does not distinguish much between what is necessary for the various phases. A few obvious things to look at:

  1. After the write phase, you can probably ditch the island bitmaps,
     too. In many repacks we're basically done then, but if you're
     generating bitmaps, that happens afterwards in the same process.

  2. The object traversal for pack-objects is done in-process these
     days. But after it finishes, I suspect that we do not generally
     need those object structs anymore, because all of the book-keeping
     is done in the bit object_entry array in packing_data.

     The exception would be generating bitmaps, which does need to do
     some traversal (and I think may hang on to actual "struct commit"
     pointers).

     I also don't think we have any code that clears obj_hash or
     released the pooled object pointers.

     So it's probably pretty tricky, but I suspect would yield big
     savings, since it's a per-object cost on the order of 64 bytes (so
     ~640MB on the kernel).

  3. During the bitmap phase I'm not sure if we still care about the
     object_entry struct in packing_data. It's the other big
     bytes-per-object user of memory. We need it all the way through the
     write phase for obvious reasons, but if we could ditch it for the
     bitmap phase, that may reduce peak memory during that phase.

-Peff