Re: [PATCH v6 00/11] nd/pack-objects-pack-struct updates

From: Jeff King <peff@peff.net>
Date: Mar 22, 2018, 09:32 UTC
Message-ID: <20180322093212.GB28907@sigill.intra.peff.net>
In-Reply-To: <CACsJy8DkF3TpTGKp5MdS1ApC8Yj6FOL5uAg5U__PjBAQY1vdNw@mail.gmail.com>

On Wed, Mar 21, 2018 at 04:59:19PM +0100, Duy Nguyen wrote:

Show 7 quoted lines

> > I hate to be a wet blanket, but am I the only one who is wondering
> > whether the tradeoffs is worth it? 8% memory reduction doesn't seem
> > mind-bogglingly good,
> 
> AEvar measured RSS. If we count objects[] array alone, the saving is
> 40% (136 bytes per entry down to 80). Some is probably eaten up by
> mmap in rss.

Measuring actual heap usage with massif, I get before/after peak heaps of 1728 and 1346MB respectively when repacking linux.git. So that's ~22% savings overall.

Of the used heap after your patches:

 - ~40% of that is from packlist_alloc()
 - ~17% goes to "struct object"
 - ~10% for the object.c hash table to store all the "struct object"
 - ~7% goes to the delta cache
 - ~7% goes to the pack revindex (actually, there's a duplicate 7%
       there, too; I think our peak is when we're sorting the revindex
       and have to keep two copies in memory at once)
 - ~5% goes to the packlist_find() hash table
 - ~3.5% for the get_object_details() sorting list (this is only held
	 for a minute, but again, our peak comes during this sort, which
	 in turn loads the revindex)

So 27% of the total heap goes away if you switch to a separate rev-list. Though it's mostly just going to a different process, it does help peak because that process would have exited by the time we get to the revindex bits.

I suspect you could get the same effect by just teaching pack-objects to clear obj_hash and all of the allocated object structs. I think that should be safe to do as long as we clear _all_ of the objects, so there are no dangling pointers.

Show 5 quoted lines

> About the 16k limit (and some other limits as well), I'm making these
> patches with the assumption that large scale deployment probably will
> go with custom builds anyway. Adjusting the limits back should be
> quite easy while we can still provide reasonable defaults for most
> people.

I think this 16k limit is the thing I _most_ dislike about the series. If we could tweak that case such that we always made forward progress, I think I'd be a lot less nervous. I responded elsewhere in the thread (before seeing that both Junio and you seemed aware of the issues ;) ), but I think it would be acceptable to have git-repack enforce the limit.

That would still mean you could get into a broken state for serving fetches, but you could at least get out of it by running "git repack".

-Peff