[06/10] grep: remove #ifdef NO_PTHREADS
diff mbox series

Message ID 20181027071003.1347-7-pclouds@gmail.com
State New
Headers show
Series
  • Reduce #ifdef NO_PTHREADS
Related show

Commit Message

Duy Nguyen Oct. 27, 2018, 7:09 a.m. UTC
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c | 58 +++++++++++++++++---------------------------------
 grep.c         |  6 ------
 grep.h         |  6 ------
 3 files changed, 20 insertions(+), 50 deletions(-)

Comments

Jeff King Oct. 27, 2018, 7:44 a.m. UTC | #1
On Sat, Oct 27, 2018 at 09:09:59AM +0200, Nguyễn Thái Ngọc Duy wrote:

> diff --git a/builtin/grep.c b/builtin/grep.c
> index d8508ddf79..29221e1003 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -34,7 +34,6 @@ static int recurse_submodules;
>  #define GREP_NUM_THREADS_DEFAULT 8
>  static int num_threads;
>  
> -#ifndef NO_PTHREADS
>  static pthread_t *threads;
>  
>  /* We use one producer thread and THREADS consumer
> @@ -265,13 +264,6 @@ static int wait_all(void)
>  
>  	return hit;
>  }
> -#else /* !NO_PTHREADS */
> -
> -static int wait_all(void)
> -{
> -	return 0;
> -}
> -#endif

Cases like this are kind of weird. I'd expect to see wait_all() return
immediately when !HAVE_THREADS. But we don't need to, because later we
do this:

> -	if (num_threads)
> +	if (HAVE_THREADS && num_threads)
>  		hit |= wait_all();

Which I think works, but it feels like we're introducing some subtle
dependencies about which functions get called in which cases. I'd hoped
in general that the non-threaded code paths could mostly just collapse
down into the same as the "threads == 1" cases, and we wouldn't have to
ask "are threads even supported" in a lot of places.

I dunno. My comment isn't really an objection exactly, but mostly just
an indication that I'm still thinking through what the best approach is,
and what end state we want to end up in.

-Peff
Junio C Hamano Oct. 29, 2018, 2:16 a.m. UTC | #2
Jeff King <peff@peff.net> writes:

> Cases like this are kind of weird. I'd expect to see wait_all() return
> immediately when !HAVE_THREADS. But we don't need to, because later we
> do this:
>
>> -	if (num_threads)
>> +	if (HAVE_THREADS && num_threads)
>>  		hit |= wait_all();
>
> Which I think works, but it feels like we're introducing some subtle
> dependencies about which functions get called in which cases. I'd hoped
> in general that the non-threaded code paths could mostly just collapse
> down into the same as the "threads == 1" cases, and we wouldn't have to
> ask "are threads even supported" in a lot of places.

True, but the way "grep" code works with threads is already strange,
and I suspect that the subtle strangeness you feel mostly comes from
that.  The single threaded code signaled "hit" with return value of
individual functions like grep_file(), but the meaning of return
value from them is changed to "does not matter--we do not have
meaningful result yet at this point" when threading is used.

In the new world order where "threading is the norm but
single-threaded is still supported", I wonder if we would want to
drive the single threaded case the same way with the add_work(run)
interface and return the "did we see hits?" etc. from the same (or
at lesat "more similar than today's") interface, so that the flow of
data smells the same in both cases.

> I dunno. My comment isn't really an objection exactly, but mostly just
> an indication that I'm still thinking through what the best approach is,
> and what end state we want to end up in.
>
> -Peff
Jeff King Oct. 29, 2018, 2:25 p.m. UTC | #3
On Mon, Oct 29, 2018 at 11:16:39AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Cases like this are kind of weird. I'd expect to see wait_all() return
> > immediately when !HAVE_THREADS. But we don't need to, because later we
> > do this:
> >
> >> -	if (num_threads)
> >> +	if (HAVE_THREADS && num_threads)
> >>  		hit |= wait_all();
> >
> > Which I think works, but it feels like we're introducing some subtle
> > dependencies about which functions get called in which cases. I'd hoped
> > in general that the non-threaded code paths could mostly just collapse
> > down into the same as the "threads == 1" cases, and we wouldn't have to
> > ask "are threads even supported" in a lot of places.
> 
> True, but the way "grep" code works with threads is already strange,
> and I suspect that the subtle strangeness you feel mostly comes from
> that.  The single threaded code signaled "hit" with return value of
> individual functions like grep_file(), but the meaning of return
> value from them is changed to "does not matter--we do not have
> meaningful result yet at this point" when threading is used.
> 
> In the new world order where "threading is the norm but
> single-threaded is still supported", I wonder if we would want to
> drive the single threaded case the same way with the add_work(run)
> interface and return the "did we see hits?" etc. from the same (or
> at lesat "more similar than today's") interface, so that the flow of
> data smells the same in both cases.

Right, your second paragraph here is a better statement of what I was
trying to get at. ;)

But if the problem is simply that we are not quite there yet in the grep
code, I am OK with taking this as the first pass, and knowing that there
is more cleanup to be done later (though that sort of thing is IMHO very
useful in a commit message).

-Peff
Duy Nguyen Oct. 29, 2018, 4:01 p.m. UTC | #4
On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
> But if the problem is simply that we are not quite there yet in the grep
> code, I am OK with taking this as the first pass, and knowing that there
> is more cleanup to be done later (though that sort of thing is IMHO very
> useful in a commit message).

Since the problem pops up now, I'm ok with updating/cleaning up all
this in this series, unless there's benefits in keeping this series
simple and merging it early (probably not?)
Jeff King Oct. 29, 2018, 4:20 p.m. UTC | #5
On Mon, Oct 29, 2018 at 05:01:41PM +0100, Duy Nguyen wrote:

> On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
> > But if the problem is simply that we are not quite there yet in the grep
> > code, I am OK with taking this as the first pass, and knowing that there
> > is more cleanup to be done later (though that sort of thing is IMHO very
> > useful in a commit message).
> 
> Since the problem pops up now, I'm ok with updating/cleaning up all
> this in this series, unless there's benefits in keeping this series
> simple and merging it early (probably not?)

Mostly I did not want to tax you. I would rather have this series and
some cleanup left over, than to not have anything. But if you are
interested in moving it further, I will not say no. :)

-Peff
Junio C Hamano Oct. 30, 2018, 1:27 a.m. UTC | #6
Jeff King <peff@peff.net> writes:

> On Mon, Oct 29, 2018 at 05:01:41PM +0100, Duy Nguyen wrote:
>
>> On Mon, Oct 29, 2018 at 3:25 PM Jeff King <peff@peff.net> wrote:
>> > But if the problem is simply that we are not quite there yet in the grep
>> > code, I am OK with taking this as the first pass, and knowing that there
>> > is more cleanup to be done later (though that sort of thing is IMHO very
>> > useful in a commit message).
>> 
>> Since the problem pops up now, I'm ok with updating/cleaning up all
>> this in this series, unless there's benefits in keeping this series
>> simple and merging it early (probably not?)
>
> Mostly I did not want to tax you. I would rather have this series and
> some cleanup left over, than to not have anything. But if you are
> interested in moving it further, I will not say no. :)

Likewise.

Patch
diff mbox series

diff --git a/builtin/grep.c b/builtin/grep.c
index d8508ddf79..29221e1003 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -34,7 +34,6 @@  static int recurse_submodules;
 #define GREP_NUM_THREADS_DEFAULT 8
 static int num_threads;
 
-#ifndef NO_PTHREADS
 static pthread_t *threads;
 
 /* We use one producer thread and THREADS consumer
@@ -265,13 +264,6 @@  static int wait_all(void)
 
 	return hit;
 }
-#else /* !NO_PTHREADS */
-
-static int wait_all(void)
-{
-	return 0;
-}
-#endif
 
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
@@ -284,8 +276,7 @@  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);
-#ifdef NO_PTHREADS
-		else if (num_threads && num_threads != 1) {
+		else if (!HAVE_THREADS && num_threads && num_threads != 1) {
 			/*
 			 * TRANSLATORS: %s is the configuration
 			 * variable for tweaking threads, currently
@@ -294,7 +285,6 @@  static int grep_cmd_config(const char *var, const char *value, void *cb)
 			warning(_("no threads support, ignoring %s"), var);
 			num_threads = 0;
 		}
-#endif
 	}
 
 	if (!strcmp(var, "submodule.recurse"))
@@ -330,17 +320,14 @@  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);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
 		 */
 		add_work(opt, &gs);
 		return 0;
-	} else
-#endif
-	{
+	} else {
 		int hit;
 
 		hit = grep_source(opt, &gs);
@@ -363,17 +350,14 @@  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);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		/*
 		 * add_work() copies gs and thus assumes ownership of
 		 * its fields, so do not call grep_source_clear()
 		 */
 		add_work(opt, &gs);
 		return 0;
-	} else
-#endif
-	{
+	} else {
 		int hit;
 
 		hit = grep_source(opt, &gs);
@@ -1038,20 +1022,20 @@  int cmd_grep(int argc, const char **argv, const char *prefix)
 	pathspec.recursive = 1;
 	pathspec.recurse_submodules = !!recurse_submodules;
 
-#ifndef NO_PTHREADS
-	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)
+	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;
-#else
-	if (num_threads)
-		warning(_("no threads support, ignoring --threads"));
-	num_threads = 0;
-#endif
+	}
 
 	if (!num_threads)
 		/*
@@ -1062,15 +1046,13 @@  int cmd_grep(int argc, const char **argv, const char *prefix)
 		 */
 		compile_grep_patterns(&opt);
 
-#ifndef NO_PTHREADS
-	if (num_threads) {
+	if (HAVE_THREADS && num_threads) {
 		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);
 	}
-#endif
 
 	if (show_in_pager && (cached || list.nr))
 		die(_("--open-files-in-pager only works on the worktree"));
@@ -1121,7 +1103,7 @@  int cmd_grep(int argc, const char **argv, const char *prefix)
 		hit = grep_objects(&opt, &pathspec, &list);
 	}
 
-	if (num_threads)
+	if (HAVE_THREADS && num_threads)
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
diff --git a/grep.c b/grep.c
index f6bd89e40b..4db1510d16 100644
--- a/grep.c
+++ b/grep.c
@@ -1513,7 +1513,6 @@  static void show_line(struct grep_opt *opt, char *bol, char *eol,
 	}
 }
 
-#ifndef NO_PTHREADS
 int grep_use_locks;
 
 /*
@@ -1539,11 +1538,6 @@  static inline void grep_attr_unlock(void)
  */
 pthread_mutex_t grep_read_mutex;
 
-#else
-#define grep_attr_lock()
-#define grep_attr_unlock()
-#endif
-
 static int match_funcname(struct grep_opt *opt, struct grep_source *gs, char *bol, char *eol)
 {
 	xdemitconf_t *xecfg = opt->priv;
diff --git a/grep.h b/grep.h
index 1a57d12b90..fb04893721 100644
--- a/grep.h
+++ b/grep.h
@@ -229,7 +229,6 @@  int grep_source(struct grep_opt *opt, struct grep_source *gs);
 extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
 extern int grep_threads_ok(const struct grep_opt *opt);
 
-#ifndef NO_PTHREADS
 /*
  * Mutex used around access to the attributes machinery if
  * opt->use_threads.  Must be initialized/destroyed by callers!
@@ -250,9 +249,4 @@  static inline void grep_read_unlock(void)
 		pthread_mutex_unlock(&grep_read_mutex);
 }
 
-#else
-#define grep_read_lock()
-#define grep_read_unlock()
-#endif
-
 #endif