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

From: Jeff King <peff@peff.net>
Date: Jul 23, 2018, 21:30 UTC
Message-ID: <20180723213040.GA7870@sigill.intra.peff.net>
In-Reply-To: <CACsJy8Cex0Uby-eBRBqc_ScZd=EnCV6e0ZuaZEmopGkrvhrBzQ@mail.gmail.com>

On Mon, Jul 23, 2018 at 08:49:59PM +0200, Duy Nguyen wrote:

Show 17 quoted lines

> On Mon, Jul 23, 2018 at 8:38 PM Duy Nguyen <pclouds@gmail.com> wrote:
> > I will have to study the thread dispatch code more to have a better
> > answer, unfortunately.
> 
> Well.. I thought I would need this weekend for this, but a quick look
> and ll_find_deltas() suggests that what we're doing is safe. At least
> you and Jeff are way to familiar with the delta window concept in
> pack-objects. So in multithread mode, we have a big list of all
> objects, then the list is cut in N sublists and each is owned entirely
> by one thread. Each thread then can slide a window over this sublist
> to search for the best delta.
> 
> Because of this partitioning, if trg_entry is being processed now, it
> will not be accessed by any other thread. It's owned by this thread
> and will be accessed again as src_entry when the next entry becomes
> the delta target in the same thread. As long as we don't access
> objects of another thread (and my v1 violates this) we should be ok.

Yes, that matches my knowledge of how this all works. And if it didn't, I think the code would have been racy even _before_ your patches.

The only thing that this pack->delta_size approach is changing is that managing that array needs to happen under lock, because it touches the whole list.

And as long as we back-fill it from any arbitrary e->delta_size_, that means that touching e->delta_size_ needs to be done under lock. That's why I think keeping the individual valid flag in each entry makes the most sense. Then whenever we overflow a particular e->delta_size_, we don't have to care about anybody else's size.

Which I think is what your v2 patch is doing.

-Peff