Re: [PATCH] git grep: be careful to use mutices only when they are initialized
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:52:19
Subsystem:
the rest · Maintainer:
Linus Torvalds
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH] git grep: be careful to use mutices only when they are initialized · Jeff King <hidden>
- 2016-06-15 · [PATCH] git grep: be careful to use mutices only when they are initialized · Johannes Schindelin <hidden>
- 2016-06-15 · Re: [PATCH] git grep: be careful to use mutices only when they are initialized · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH] git grep: be careful to use mutices only when they are initialized · Johannes Schindelin <hidden>
- 2016-06-15 · Re: [PATCH] git grep: be careful to use mutices only when they are initialized · Junio C Hamano <hidden>
Junio C Hamano [off-list ref] writes:
The remainder of this message are hints and random thoughts on potential follow-up patches that may want to build on top of this patch for further clean-ups (not specifically meant for Dscho but for other people on both mailing lists). ... - Could we lose "#ifndef NO_PTHREADS" inside grep_sha1(), grep_file(), and possibly cmd_grep() functions and let the compiler optimize things away under NO_PTHREADS compilation?
I suspect that the result of the conversion would look a lot cleaner if the code is first cleaned up to move global variable like skip_first_line and the mutexes into the grep_opt structure. Without such clean-up, I do not think a conversion like this does not add much value. But since I already did it,... -- >8 -- Subject: [PATCH] builtin/grep: war on #if[n]def inside function body Get rid of #if[n]def inside implementation of the function body and let the compiler optimize codepaths that are protected with "if (use_threads)" away on NO_PTHREADS builds. Signed-off-by: Junio C Hamano <redacted> --- builtin/grep.c | 37 +++++++++++++++++++------------------ 1 files changed, 19 insertions(+), 18 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 3d7329d..f24f3a7 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c@@ -24,9 +24,13 @@ static char const * const grep_usage[] = { NULL }; -static int use_threads = 1; +/* Skip the leading hunk mark of the first file. */ +static int skip_first_line; #ifndef NO_PTHREADS +static int use_threads = 1; +#define no_threads() use_threads = 0 + #define THREADS 8 static pthread_t threads[THREADS];
@@ -112,8 +116,6 @@ static pthread_cond_t cond_write; /* Signalled when we are finished with everything. */ static pthread_cond_t cond_result; -static int skip_first_line; - static void add_work(enum work_type type, char *name, void *id) { grep_lock();
@@ -181,7 +183,6 @@ static void work_done(struct work_item *w) const char *p = w->out.buf; size_t len = w->out.len; - /* Skip the leading hunk mark of the first file. */ if (skip_first_line) { while (len) { len--;
@@ -310,8 +311,18 @@ static int wait_all(void) return hit; } #else /* !NO_PTHREADS */ +#define use_threads 0 +#define no_threads() /* noop */ + #define read_sha1_lock() #define read_sha1_unlock() +#define grep_lock() +#define grep_unlock() + +#define online_cpus() 1 +#define grep_sha1_async(opt, name, sha1) +#define grep_file_async(opt, name, filename) +#define start_threads(opt) static int wait_all(void) {
@@ -407,13 +418,10 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1, name = strbuf_detach(&pathbuf, NULL); -#ifndef NO_PTHREADS if (use_threads) { grep_sha1_async(opt, name, sha1); return 0; - } else -#endif - { + } else { int hit; unsigned long sz; void *data = load_sha1(sha1, &sz, name);
@@ -469,13 +477,10 @@ static int grep_file(struct grep_opt *opt, const char *filename) strbuf_addstr(&buf, filename); name = strbuf_detach(&buf, NULL); -#ifndef NO_PTHREADS if (use_threads) { grep_file_async(opt, name, filename); return 0; - } else -#endif - { + } else { int hit; size_t sz; void *data = load_file(filename, &sz);
@@ -992,7 +997,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) opt.output_priv = &path_list; opt.output = append_path; string_list_append(&path_list, show_in_pager); - use_threads = 0; + no_threads(); } if (!opt.pattern_list)
@@ -1000,9 +1005,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix) if (!opt.fixed && opt.ignore_case) opt.regflags |= REG_ICASE; -#ifndef NO_PTHREADS if (online_cpus() == 1 || !grep_threads_ok(&opt)) - use_threads = 0; + no_threads(); if (use_threads) { if (opt.pre_context || opt.post_context || opt.file_break ||
@@ -1010,9 +1014,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix) skip_first_line = 1; start_threads(&opt); } -#else - use_threads = 0; -#endif compile_grep_patterns(&opt);
--
1.7.7.1.504.gcc718