From fd6263fb733e50cfbcf857136388bf47d9c5151f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Sat, 3 Nov 2018 09:48:44 +0100 Subject: [PATCH] grep: clean up num_threads handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When NO_PTHREADS is still used in this file, we have two separate code paths for thread and no thread support. The latter will always have num_threads remain zero while the former uses num_threads zero as "default number of threads". With recent changes blur the line between thread and no-thread support, this num_threads handling becomes a bit strange so let's redefine it like this: - num_threads == 0 means default number of threads and should become positive after all configuration and option parsing is done if multithread is supported. - num_threads <= 1 runs no threads. It does not matter if the platform supports threading or not. - num_threads > 1 will run multiple threads and is invalid if HAVE_THREADS is false. pthread API is only used in this case. PS. a new warning is also added when num_threads is forced back to one because a thread-incompatible option is specified. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- builtin/grep.c | 60 +++++++++++++++++++++++--------------------------- 1 file changed, 28 insertions(+), 32 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 6dd15dbaa2..de3f568cee 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -69,13 +69,11 @@ static pthread_mutex_t grep_mutex; static inline void grep_lock(void) { - assert(num_threads); pthread_mutex_lock(&grep_mutex); } static inline void grep_unlock(void) { - assert(num_threads); pthread_mutex_unlock(&grep_mutex); } @@ -234,7 +232,7 @@ static int wait_all(void) int i; if (!HAVE_THREADS) - return 0; + BUG("Never call this function unless you have started threads"); grep_lock(); all_work_added = 1; @@ -279,14 +277,14 @@ static int grep_cmd_config(const char *var, const char *value, void *cb) if (num_threads < 0) die(_("invalid number of threads specified (%d) for %s"), num_threads, var); - else if (!HAVE_THREADS && num_threads && num_threads != 1) { + else if (!HAVE_THREADS && num_threads > 1) { /* * TRANSLATORS: %s is the configuration * variable for tweaking threads, currently * grep.threads */ warning(_("no threads support, ignoring %s"), var); - num_threads = 0; + num_threads = 1; } } @@ -323,7 +321,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid, grep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid); strbuf_release(&pathbuf); - if (HAVE_THREADS && num_threads) { + if (num_threads > 1) { /* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear() @@ -353,7 +351,7 @@ static int grep_file(struct grep_opt *opt, const char *filename) grep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename); strbuf_release(&buf); - if (HAVE_THREADS && num_threads) { + if (num_threads > 1) { /* * add_work() copies gs and thus assumes ownership of * its fields, so do not call grep_source_clear() @@ -1025,36 +1023,34 @@ int cmd_grep(int argc, const char **argv, const char *prefix) pathspec.recursive = 1; pathspec.recurse_submodules = !!recurse_submodules; - if (HAVE_THREADS) { - if (list.nr || cached || show_in_pager) - num_threads = 0; - else if (num_threads == 0) - num_threads = GREP_NUM_THREADS_DEFAULT; - else if (num_threads < 0) - die(_("invalid number of threads specified (%d)"), num_threads); - if (num_threads == 1) - num_threads = 0; - } else { - if (num_threads) - warning(_("no threads support, ignoring --threads")); - num_threads = 0; - } + if (list.nr || cached || show_in_pager) { + if (num_threads > 1) + warning(_("invalid option combination, ignoring --threads")); + num_threads = 1; + } else if (!HAVE_THREADS && num_threads > 1) { + warning(_("no threads support, ignoring --threads")); + num_threads = 1; + } else if (num_threads < 0) + die(_("invalid number of threads specified (%d)"), num_threads); + else if (num_threads == 0) + num_threads = HAVE_THREADS ? GREP_NUM_THREADS_DEFAULT : 1; - if (!num_threads) - /* - * The compiled patterns on the main path are only - * used when not using threading. Otherwise - * start_threads() below calls compile_grep_patterns() - * for each thread. - */ - compile_grep_patterns(&opt); - - if (HAVE_THREADS && num_threads) { + if (num_threads > 1) { + if (!HAVE_THREADS) + BUG("Somebody got num_threads calculation wrong!"); if (!(opt.name_only || opt.unmatch_name_only || opt.count) && (opt.pre_context || opt.post_context || opt.file_break || opt.funcbody)) skip_first_line = 1; start_threads(&opt); + } else { + /* + * The compiled patterns on the main path are only + * used when not using threading. Otherwise + * start_threads() above calls compile_grep_patterns() + * for each thread. + */ + compile_grep_patterns(&opt); } if (show_in_pager && (cached || list.nr)) @@ -1106,7 +1102,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix) hit = grep_objects(&opt, &pathspec, &list); } - if (num_threads) + if (num_threads > 1) hit |= wait_all(); if (hit && show_in_pager) run_pager(&opt, prefix);