From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Wed, 17 Feb 2010 01:01:03 GMT
Subject: Re: [PATCH] fix threaded grep for machines with only one cpu
Message-ID: <alpine.DEB.1.00.1002170200370.20986@pacific.mpi-cbg.de>
In-Reply-To: <7viq9wzq8g.fsf@alter.siamese.dyndns.org>

Hi,

On Tue, 16 Feb 2010, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> diff --git a/builtin-grep.c b/builtin-grep.c
> >> index 26d4deb..5c1545e 100644
> >> --- a/builtin-grep.c
> >> +++ b/builtin-grep.c
> >> @@ -81,8 +81,8 @@ static pthread_mutex_t read_sha1_mutex;
> >>  
> >>  #define grep_lock() pthread_mutex_lock(&grep_mutex)
> >>  #define grep_unlock() pthread_mutex_unlock(&grep_mutex)
> >> -#define read_sha1_lock() pthread_mutex_lock(&read_sha1_mutex)
> >> -#define read_sha1_unlock() pthread_mutex_unlock(&read_sha1_mutex)
> >> +#define read_sha1_lock() do { if (use_threads) pthread_mutex_lock(&read_sha1_mutex); } while (0)
> >> +#define read_sha1_unlock() do { if (use_threads) pthread_mutex_unlock(&read_sha1_mutex); } while (0)
> >
> > This is inconsistent. Just look at the code above and tell me why it is so 
> > different.
> 
> It is because grep_mutex is protected by "use_threads" very high in the
> callchain and do not need nor want extra if().
> 
> But I think this is much cleaner.  The patch replaces the one you are
> replying to (i.e. read_sha1_{lock,unlock}() are unconditional).
> 
> -- >8 --
> Subject: Fix use of mutex in threaded grep
> 
> The program can decide at runtime not to use threading even if the support
> is compiled in.  In such a case, mutexes are not necessary and left
> uninitialized.  But the code incorrectly tried to take and release the
> read_sha1_mutex unconditionally.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Acked-by: Fredrik Kuivinen <frekui@gmail.com>
> ---

Yes, this one looks much, much nicer.

Ciao,
Dscho