Re: [PATCH] fix threaded grep for machines with only one cpu
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 16, 2010, 23:59 UTC
- Message-ID
- <7viq9wzq8g.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <alpine.DEB.1.00.1002160914140.20986@pacific.mpi-cbg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 15 quoted lines
>> 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> ---
builtin-grep.c | 25 ++++++++++++++++--------- 1 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/builtin-grep.c b/builtin-grep.c index 26d4deb..8cec8b6 100644 --- a/builtin-grep.c +++ b/builtin-grep.c @@ -408,15 +408,25 @@ static int pathspec_matches(const char **paths, const char *name, int max_depth) return 0; } +static void *read_sha1_file_locked(const unsigned char *sha1, enum object_type *type, unsigned long *size) +{ + void *data; + + if (use_threads) { + read_sha1_lock(); + data = read_sha1_file(sha1, type, size); + read_sha1_unlock(); + } else { + data = read_sha1_file(sha1, type, size); + } + return data; +} + static void *load_sha1(const unsigned char *sha1, unsigned long *size, const char *name) { enum object_type type; - char *data; - - read_sha1_lock(); - data = read_sha1_file(sha1, &type, size); - read_sha1_unlock(); + void *data = read_sha1_file_locked(sha1, &type, size); if (!data) error("'%s': unable to read %s", name, sha1_to_hex(sha1)); @@ -605,10 +615,7 @@ static int grep_tree(struct grep_opt *opt, const char **paths, void *data; unsigned long size; - read_sha1_lock(); - data = read_sha1_file(entry.sha1, &type, &size); - read_sha1_unlock(); - + data = read_sha1_file_locked(entry.sha1, &type, &size); if (!data) die("unable to read tree (%s)", sha1_to_hex(entry.sha1));