Re: [PATCH 3/3] pack-objects (mingw): initialize `packing_data` mutex in the correct spot

From: Jeff King <peff@peff.net>
Date: Oct 17, 2018, 10:14 UTC
Message-ID: <20181017101411.GA12554@sigill.intra.peff.net>
In-Reply-To: <8979693922364ef2e4a84a29298b081e86fa9ffa.1539723766.git.gitgitgadget@gmail.com>

On Tue, Oct 16, 2018 at 02:02:52PM -0700, Johannes Schindelin via GitGitGadget wrote:

Show 19 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> In 9ac3f0e5b3e4 (pack-objects: fix performance issues on packing large
> deltas, 2018-07-22), a mutex was introduced that is used to guard the
> call to set the delta size. This commit even added code to initialize
> it, but at an incorrect spot: in `init_threaded_search()`, while the
> call to `oe_set_delta_size()` (and hence to `packing_data_lock()`) can
> happen in the call chain `check_object()` <- `get_object_details()` <-
> `prepare_pack()` <- `cmd_pack_objects()`, which is long before the
> `prepare_pack()` function calls `ll_find_deltas()` (which initializes
> the threaded search).
> 
> Another tell-tale that the mutex was initialized in an incorrect spot is
> that the function to initialize it lives in builtin/, while the code
> that uses the mutex is defined in a libgit.a header file.
> 
> Let's use a more appropriate function: `prepare_packing_data()`, which
> not only lives in libgit.a, but *has* to be called before the
> `packing_data` struct is used that contains that mutex.

Nicely explained. I think this is a good solution.

Both before and after your patch, we do still take the lock even in single-threaded scenarios (the case you found where we are not yet in the delta search phase, but also when --threads=1). I think that should be fine. It looks like we do that with the other locks in pack-objects.c already.

In index-pack.c, we check a threads_active flag before looking at the lock, which could be another possible solution. I doubt it's any faster, though (which is why I assume index-pack.c does it). Locking/unlocking a mutex should not really be much slower than checking the conditional flag in the first place.

Which is all a roundabout way of saying "looks good to me".

-Peff