On Tue, Oct 16, 2018 at 02:02:52PM -0700, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@xxxxxx> > > 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
