Re: [PATCH] pack-objects: fix performance issues on packing large deltas

From: Jeff King <peff@peff.net>
Date: Jul 23, 2018, 21:37 UTC
Message-ID: <20180723213752.GB7870@sigill.intra.peff.net>
In-Reply-To: <CACsJy8AcDqNtV1VfHB+ZD=wvOxRyhobacreaNpjSm3NcLCMKWA@mail.gmail.com>

On Sat, Jul 21, 2018 at 06:23:32AM +0200, Duy Nguyen wrote:

Show 10 quoted lines

> > I'm not sure I completely agree with this. While 4GB deltas should be
> > pretty rare, the nice thing about 64-bit is that you never have to even
> > think about whether the variable is large enough. I think the 2^32
> > limitations on Windows are something we should be fixing in the long
> > term (though there it is even worse because it is not just deltas, but
> > entire objects).
> 
> I guess that means we stick to uint64_t then? It does increase more
> memory on 32-bit architecture (and probably processing cost as well
> because 64-bit uses up 2 registers).

Yes, but if we switch away from an array to a hash, then we get the best of both worlds: we are using 64-bits to store the size, but we only need an entry for deltas that are actually big.

Then the common small-delta case remains efficient in both CPU and memory, and we pay the costs only in proportion to the number of large deltas (though the hash is a slightly higher cost for those large deltas than an array).

Show 10 quoted lines

> > This is new in this iteration. I guess this is to cover the case where
> > we are built with pthread support, but --threads=1?
> 
> If you mean the "lock_initialized" variable, not really. the other
> _lock() macros in builtin/ call pthread_mutex_lock() unconditionally,
> which is fine. But I feel a bit uncomfortable doing the same in
> pack-objects.h which technically is library code (but yes practically
> is a long arm of builtin/pack-objects.c), so lock_initialized is there
> to make sure we don't touch uninitialized locks if somebody forgets to
> init them first.

I think the ones in builtin/ check threads_active to avoid actually locking. And that's set in init_thread(), which we do not call if we are using a single thread. So I think this is basically doing the same thing, but with a separate flag (since the library code does not know about threads_active).

Show 6 quoted lines

> > Your original patch had to copy the oe_* helpers into the file to handle
> > that. But I think we're essentially just locking the whole functions:
> 
> I'll try to avoid this lock when deltas are small and see if it helps
> the linux.git case on Elijah's machine. So we may end up locking just
> a part of these functions.

Yeah, I think my suggestion doesn't work once we start doing more complex locking logic. Let's just forget it. I think the "lock_initialized" thing is probably the right approach.

It might be worth getting rid of builtin/pack-objects.c's local threads_active variable, and just using to_pack.threads_active. The two flag would always want to match.

-Peff