| Message ID | 20201007181943.GB1976631@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | jt/threaded-inex-pack leftovers | expand |
> The type_cas lock lost all of its callers in f08cbf60fe (index-pack: > make quantum of work smaller, 2020-09-08), so we can safely delete it. > The compiler didn't alert us that the variable became unused, because we > still call pthread_mutex_init() and pthread_mutex_destroy() on it. > > It's worth considering also whether that commit was in error to remove > the use of the lock. Why don't we need it now, if we did before, as > described in ab791dd138 (index-pack: fix race condition with duplicate > bases, 2014-08-29)? I think the answer is that we now look at and assign > the child_obj->real_type field in the main thread while holding the > work_lock(). So we don't have to worry about racing with the worker > threads. Yeah - I probably should have made the change without removing the compare-and-swap, and then removed the compare-and-swap in a subsequent patch. Thanks for catching this.
diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 0f05533902..f8f1b48e56 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -159,10 +159,6 @@ static pthread_mutex_t deepest_delta_mutex; #define deepest_delta_lock() lock_mutex(&deepest_delta_mutex) #define deepest_delta_unlock() unlock_mutex(&deepest_delta_mutex) -static pthread_mutex_t type_cas_mutex; -#define type_cas_lock() lock_mutex(&type_cas_mutex) -#define type_cas_unlock() unlock_mutex(&type_cas_mutex) - static pthread_key_t key; static inline void lock_mutex(pthread_mutex_t *mutex) @@ -186,7 +182,6 @@ static void init_thread(void) init_recursive_mutex(&read_mutex); pthread_mutex_init(&counter_mutex, NULL); pthread_mutex_init(&work_mutex, NULL); - pthread_mutex_init(&type_cas_mutex, NULL); if (show_stat) pthread_mutex_init(&deepest_delta_mutex, NULL); pthread_key_create(&key, NULL); @@ -209,7 +204,6 @@ static void cleanup_thread(void) pthread_mutex_destroy(&read_mutex); pthread_mutex_destroy(&counter_mutex); pthread_mutex_destroy(&work_mutex); - pthread_mutex_destroy(&type_cas_mutex); if (show_stat) pthread_mutex_destroy(&deepest_delta_mutex); for (i = 0; i < nr_threads; i++)
The type_cas lock lost all of its callers in f08cbf60fe (index-pack: make quantum of work smaller, 2020-09-08), so we can safely delete it. The compiler didn't alert us that the variable became unused, because we still call pthread_mutex_init() and pthread_mutex_destroy() on it. It's worth considering also whether that commit was in error to remove the use of the lock. Why don't we need it now, if we did before, as described in ab791dd138 (index-pack: fix race condition with duplicate bases, 2014-08-29)? I think the answer is that we now look at and assign the child_obj->real_type field in the main thread while holding the work_lock(). So we don't have to worry about racing with the worker threads. Signed-off-by: Jeff King <peff@peff.net> --- builtin/index-pack.c | 6 ------ 1 file changed, 6 deletions(-)