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

[Jeff King <peff@peff.net>]

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

> 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