Re: [PATCH] git grep: be careful to use mutices only when they are initialized
Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> 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 <gitster@pobox.com>
---
builtin/grep.c | 37 +++++++++++++++++++------------------
1 files changed, 19 insertions(+), 18 deletions(-)
Show changes to builtin/grep.c +19 −18
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
Johannes Schindelin
