Re: [PATCH] pack-objects: document about thread synchronization

From: Duy Nguyen <pclouds@gmail.com>
Date: Aug 4, 2018, 05:56 UTC
Message-ID: <CACsJy8DRQqTTV_vqxxV5dG_+UnONWNit76VMqb0=MdXsvH+2-Q@mail.gmail.com>
In-Reply-To: <20180802193912.GB29084@sigill.intra.peff.net>

On Thu, Aug 2, 2018 at 9:39 PM Jeff King <peff@peff.net> wrote:

Show 19 quoted lines

>
> On Sun, Jul 29, 2018 at 05:46:17PM +0200, Duy Nguyen wrote:
>
> > tOn Sun, Jul 29, 2018 at 5:36 PM Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> > >
> > > These extra comments should be make it easier to understand how to use
> > > locks in pack-objects delta search code. For reference, see
> >
> > Side note. I wonder if we could take progress_lock() less often in
> > find_deltas() to reduce contention. Right now we take the lock every
> > time we check out one object_entry but we could pull like 16 items out
> > of the list per lock. I don't know how much actual contention on this
> > lock though so maybe not worth doing.
>
> I doubt it really matters that much, since we hold it for such a small
> amount of time (basically just a few pointer/integer tweaks). Compared
> to the heavy lifting of actually loading objects, you're not likely to
> see a huge amount of contention, since at any given moment most threads
> will be doing that work (or actually computing deltas).

That was also my assumption when I added the lock to that oe_get_delta_size() but it seemed to slow lots-of-core system significantly. My theory is when we do small deltas, turnaround time could be very quick so if you have lots of threads we'll end up racing to hold the lock.

What I missed here though, is that we do try_delta() on the whole window in a thread before we would need progress_lock again, so it's a lot more work than a single try_delta() and likely less contention.

Show 11 quoted lines

>
> Of course I could be wrong. If you hit a point where many threads are
> skipping work (e.g., because they are considering deltas from objects in
> the same pack, and skip forward without actually doing any work), then
> they'd be ripping through the find_deltas() loop pretty quickly.
>
> OTOH, in cases like that (and the ultimate case would just be running
> "git repack -ad" twice in a row), the compression phase seems to go
> quite quickly, implying we're not spending a lot of time there.
>
> -Peff

-- 
Duy