| Message ID | 20181027071003.1347-7-pclouds@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reduce #ifdef NO_PTHREADS | expand |
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
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
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
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?)
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
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.
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
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(-)