# [PATCH v3 3/3] grep: disable threading in non-worktree case 9 messages from 2011-12-12 to 2011-12-16. Participants: Thomas Rast, René Scharfe, Junio C Hamano, Johannes Sixt. Thread: https://gitlist.dev/t/29142 ## Thomas Rast, 2011-12-12 21:16 Subject: [PATCH v3 0/3] grep attributes and multithreading Message-ID: URL: https://gitlist.dev/e/cover.1323723759.git.trast%40student.ethz.ch In-Reply-To: <4ED8F9AE.8030605@lsrfire.ath.cx> ``` I think we should finish up these three patches for the next release. The first two are unchanged from last time; nobody seemed to have any objections. On the third one I'm following René's argument: > Am 02.12.2011 14:07, schrieb Thomas Rast: > > So disable threading entirely when not scanning the worktree, to get > > the NO_PTHREADS performance in that case. This obsoletes all code > > related to grep_sha1_async. The thread startup must be delayed until > > after all arguments have been parsed, but this does not have a > > measurable effect. > > This is a bit radical. I think the underlying issue that > read_sha1_file() is not thread-safe can be solved eventually and then > we'd need to readd that code. I already posted a bunch of POC patches, but doing the timing manually has been getting on my nerves lately. So I would first like to formalize some of the performance testing, perhaps along lines already drawn up by Michael Hagger, perhaps not. Then we can revisit the issue of grep performance. But I would prefer not to block the -W fix and two easy and confirmed speedups on that. I dropped this part entirely: > How about adding a parameter to control the number of threads > (--threads?) instead that defaults to eight (or five) for the worktree > and one for the rest? That would also make benchmarking easier. It does make testing a lot easier, but the interface is IMHO not fit for users and I have a feeling that the "right" for-debugging interface will end up falling out of the performance testing work (probably an environment variable). The end-user option should be a config setting, if any. Thomas Rast (3): grep: load funcname patterns for -W grep: enable threading with -p and -W using lazy attribute lookup grep: disable threading in non-worktree case builtin/grep.c | 34 +++++++++++++++---------- grep.c | 73 ++++++++++++++++++++++++++++++++++--------------------- grep.h | 7 +++++ t/t7810-grep.sh | 14 ++++++++++ 4 files changed, 86 insertions(+), 42 deletions(-) -- 1.7.8.431.g2abf2 ``` ## Thomas Rast, 2011-12-12 21:16 Subject: [PATCH v3 1/3] grep: load funcname patterns for -W Message-ID: <5abfcd11512c0118465459dd8cf43980aec7ceee.1323723759.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/5abfcd11512c0118465459dd8cf43980aec7ceee.1323723759.git.trast%40student.ethz.ch In-Reply-To: ``` git-grep avoids loading the funcname patterns unless they are needed. ba8ea74 (grep: add option to show whole function as context, 2011-08-01) forgot to extend this test also to the new funcbody feature. Do so. The catch is that we also have to disable threading when using userdiff, as explained in grep_threads_ok(). So we must be careful to introduce the same test there. --- grep.c | 7 ++++--- t/t7810-grep.sh | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/grep.c b/grep.c index b29d09c..7a070e9 100644 --- a/grep.c +++ b/grep.c @@ -948,8 +948,8 @@ int grep_threads_ok(const struct grep_opt *opt) * machinery in grep_buffer_1. The attribute code is not * thread safe, so we disable the use of threads. */ - if (opt->funcname && !opt->unmatch_name_only && !opt->status_only && - !opt->name_only) + if ((opt->funcname || opt->funcbody) + && !opt->unmatch_name_only && !opt->status_only && !opt->name_only) return 0; return 1; @@ -1008,7 +1008,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, } memset(&xecfg, 0, sizeof(xecfg)); - if (opt->funcname && !opt->unmatch_name_only && !opt->status_only && + if ((opt->funcname || opt->funcbody) + && !opt->unmatch_name_only && !opt->status_only && !opt->name_only && !binary_match_only && !collect_hits) { struct userdiff_driver *drv = userdiff_find_by_path(name); if (drv && drv->funcname.pattern) { diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index 81263b7..7ba5b16 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -523,6 +523,20 @@ test_expect_success 'grep -W' ' test_cmp expected actual ' +cat >expected <.gitattributes && + git grep -W return >actual && + test_cmp expected actual +' + test_expect_success 'grep from a subdirectory to search wider area (1)' ' mkdir -p s && ( -- 1.7.8.431.g2abf2 ``` ## Thomas Rast, 2011-12-12 21:16 Subject: [PATCH v3 2/3] grep: enable threading with -p and -W using lazy attribute lookup Message-ID: <138b930c0c96029f2fb9a816e73e70a4d5bce1de.1323723759.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/138b930c0c96029f2fb9a816e73e70a4d5bce1de.1323723759.git.trast%40student.ethz.ch In-Reply-To: ``` Lazily load the userdiff attributes in match_funcname(). Use a separate mutex around this loading to protect the (not thread-safe) attributes machinery. This lets us re-enable threading with -p and -W while reducing the overhead caused by looking up attributes. Signed-off-by: Thomas Rast --- builtin/grep.c | 12 +++++++-- grep.c | 74 ++++++++++++++++++++++++++++++++++---------------------- grep.h | 7 +++++ 3 files changed, 61 insertions(+), 32 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 988ea1d..bc23c3c 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -256,6 +256,7 @@ static void start_threads(struct grep_opt *opt) pthread_mutex_init(&grep_mutex, NULL); pthread_mutex_init(&read_sha1_mutex, NULL); + pthread_mutex_init(&grep_attr_mutex, NULL); pthread_cond_init(&cond_add, NULL); pthread_cond_init(&cond_write, NULL); pthread_cond_init(&cond_result, NULL); @@ -303,6 +304,7 @@ static int wait_all(void) pthread_mutex_destroy(&grep_mutex); pthread_mutex_destroy(&read_sha1_mutex); + pthread_mutex_destroy(&grep_attr_mutex); pthread_cond_destroy(&cond_add); pthread_cond_destroy(&cond_write); pthread_cond_destroy(&cond_result); @@ -1002,17 +1004,21 @@ int cmd_grep(int argc, const char **argv, const char *prefix) opt.regflags |= REG_ICASE; #ifndef NO_PTHREADS - if (online_cpus() == 1 || !grep_threads_ok(&opt)) + if (online_cpus() == 1) use_threads = 0; +#else + use_threads = 0; +#endif + + opt.use_threads = use_threads; +#ifndef NO_PTHREADS if (use_threads) { if (opt.pre_context || opt.post_context || opt.file_break || opt.funcbody) skip_first_line = 1; start_threads(&opt); } -#else - use_threads = 0; #endif compile_grep_patterns(&opt); diff --git a/grep.c b/grep.c index 7a070e9..4dd7da2 100644 --- a/grep.c +++ b/grep.c @@ -2,6 +2,7 @@ #include "grep.h" #include "userdiff.h" #include "xdiff-interface.h" +#include "thread-utils.h" void append_header_grep_pattern(struct grep_opt *opt, enum grep_header_field field, const char *pat) { @@ -806,10 +807,46 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol, opt->output(opt, "\n", 1); } -static int match_funcname(struct grep_opt *opt, char *bol, char *eol) +#ifndef NO_PTHREADS +/* + * This lock protects access to the gitattributes machinery, which is + * not thread-safe. + */ +pthread_mutex_t grep_attr_mutex; + +static inline void grep_attr_lock(struct grep_opt *opt) +{ + if (opt->use_threads) + pthread_mutex_lock(&grep_attr_mutex); +} + +static inline void grep_attr_unlock(struct grep_opt *opt) +{ + if (opt->use_threads) + pthread_mutex_unlock(&grep_attr_mutex); +} +#else +#define grep_attr_lock(opt) +#define grep_attr_unlock(opt) +#endif + +static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol) { xdemitconf_t *xecfg = opt->priv; - if (xecfg && xecfg->find_func) { + if (xecfg && !xecfg->find_func) { + struct userdiff_driver *drv; + grep_attr_lock(opt); + drv = userdiff_find_by_path(name); + grep_attr_unlock(opt); + if (drv && drv->funcname.pattern) { + const struct userdiff_funcname *pe = &drv->funcname; + xdiff_set_find_func(xecfg, pe->pattern, pe->cflags); + } else { + xecfg = opt->priv = NULL; + } + } + + if (xecfg) { char buf[1]; return xecfg->find_func(bol, eol - bol, buf, 1, xecfg->find_func_priv) >= 0; @@ -835,7 +872,7 @@ static void show_funcname_line(struct grep_opt *opt, const char *name, if (lno <= opt->last_shown) break; - if (match_funcname(opt, bol, eol)) { + if (match_funcname(opt, name, bol, eol)) { show_line(opt, bol, eol, name, lno, '='); break; } @@ -848,7 +885,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf, unsigned cur = lno, from = 1, funcname_lno = 0; int funcname_needed = !!opt->funcname; - if (opt->funcbody && !match_funcname(opt, bol, end)) + if (opt->funcbody && !match_funcname(opt, name, bol, end)) funcname_needed = 2; if (opt->pre_context < lno) @@ -864,7 +901,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf, while (bol > buf && bol[-1] != '\n') bol--; cur--; - if (funcname_needed && match_funcname(opt, bol, eol)) { + if (funcname_needed && match_funcname(opt, name, bol, eol)) { funcname_lno = cur; funcname_needed = 0; } @@ -942,19 +979,6 @@ static int look_ahead(struct grep_opt *opt, return 0; } -int grep_threads_ok(const struct grep_opt *opt) -{ - /* If this condition is true, then we may use the attribute - * machinery in grep_buffer_1. The attribute code is not - * thread safe, so we disable the use of threads. - */ - if ((opt->funcname || opt->funcbody) - && !opt->unmatch_name_only && !opt->status_only && !opt->name_only) - return 0; - - return 1; -} - static void std_output(struct grep_opt *opt, const void *buf, size_t size) { fwrite(buf, size, 1, stdout); @@ -1008,16 +1032,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, } memset(&xecfg, 0, sizeof(xecfg)); - if ((opt->funcname || opt->funcbody) - && !opt->unmatch_name_only && !opt->status_only && - !opt->name_only && !binary_match_only && !collect_hits) { - struct userdiff_driver *drv = userdiff_find_by_path(name); - if (drv && drv->funcname.pattern) { - const struct userdiff_funcname *pe = &drv->funcname; - xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags); - opt->priv = &xecfg; - } - } + opt->priv = &xecfg; + try_lookahead = should_lookahead(opt); while (left) { @@ -1093,7 +1109,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, show_function = 1; goto next_line; } - if (show_function && match_funcname(opt, bol, eol)) + if (show_function && match_funcname(opt, name, bol, eol)) show_function = 0; if (show_function || (last_hit && lno <= last_hit + opt->post_context)) { diff --git a/grep.h b/grep.h index a652800..15d227c 100644 --- a/grep.h +++ b/grep.h @@ -115,6 +115,7 @@ struct grep_opt { int show_hunk_mark; int file_break; int heading; + int use_threads; void *priv; void (*output)(struct grep_opt *opt, const void *data, size_t size); @@ -131,4 +132,10 @@ struct grep_opt { 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! */ +extern pthread_mutex_t grep_attr_mutex; +#endif + #endif -- 1.7.8.431.g2abf2 ``` ## Thomas Rast, 2011-12-12 21:16 Subject: [PATCH v3 3/3] grep: disable threading in non-worktree case Message-ID: URL: https://gitlist.dev/e/faa76319c7863852eb6848614c193b56af5789f9.1323723759.git.trast%40student.ethz.ch In-Reply-To: ``` Measurements by various people have shown that grepping in parallel is not beneficial when the object store is involved. For example, with a simple regex: Threads | --cached case | worktree case ---------------------------------------------------------------- 8 (default) | 2.88u 0.21s 0:02.94real | 0.19u 0.32s 0:00.16real 4 | 2.89u 0.29s 0:02.99real | 0.16u 0.34s 0:00.17real 2 | 2.83u 0.36s 0:02.87real | 0.18u 0.32s 0:00.26real NO_PTHREADS | 2.16u 0.08s 0:02.25real | 0.12u 0.17s 0:00.31real This happens because all the threads contend on read_sha1_mutex almost all of the time. A more complex regex allows the threads to do more work in parallel, but as Jeff King found out, the "super boost" (much higher clock when only one core is active) feature of recent CPUs still causes the unthreaded case to win by a large margin. So until the pack machinery allows unthreaded access, we disable grep's threading in all but the worktree case. Helped-by: René Scharfe Helped-by: Jeff King Signed-off-by: Thomas Rast --- builtin/grep.c | 36 ++++++++++++++++++------------------ 1 files changed, 18 insertions(+), 18 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index bc23c3c..7affbda 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -1003,24 +1003,6 @@ 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) - use_threads = 0; -#else - use_threads = 0; -#endif - - opt.use_threads = use_threads; - -#ifndef NO_PTHREADS - if (use_threads) { - if (opt.pre_context || opt.post_context || opt.file_break || - opt.funcbody) - skip_first_line = 1; - start_threads(&opt); - } -#endif - compile_grep_patterns(&opt); /* Check revs and then paths */ @@ -1042,6 +1024,24 @@ int cmd_grep(int argc, const char **argv, const char *prefix) break; } +#ifndef NO_PTHREADS + if (list.nr || cached || online_cpus() == 1) + use_threads = 0; +#else + use_threads = 0; +#endif + + opt.use_threads = use_threads; + +#ifndef NO_PTHREADS + if (use_threads) { + if (opt.pre_context || opt.post_context || opt.file_break || + opt.funcbody) + skip_first_line = 1; + start_threads(&opt); + } +#endif + /* The rest are paths */ if (!seen_dashdash) { int j; -- 1.7.8.431.g2abf2 ``` ## René Scharfe, 2011-12-12 22:37 Subject: Re: [PATCH v3 0/3] grep attributes and multithreading Message-ID: <4EE6820E.7010605@lsrfire.ath.cx> URL: https://gitlist.dev/e/4EE6820E.7010605%40lsrfire.ath.cx In-Reply-To: ``` Am 12.12.2011 22:16, schrieb Thomas Rast: > I think we should finish up these three patches for the next release. > I already posted a bunch of POC patches, but doing the timing manually > has been getting on my nerves lately. So I would first like to > formalize some of the performance testing, perhaps along lines already > drawn up by Michael Hagger, perhaps not. Then we can revisit the > issue of grep performance. But I would prefer not to block the -W fix > and two easy and confirmed speedups on that. Yes, that's a good idea. The three patches are uncontroversial incremental improvements. > I dropped this part entirely: > >> How about adding a parameter to control the number of threads >> (--threads?) instead that defaults to eight (or five) for the worktree >> and one for the rest? That would also make benchmarking easier. > > It does make testing a lot easier, but the interface is IMHO not fit > for users and I have a feeling that the "right" for-debugging > interface will end up falling out of the performance testing work > (probably an environment variable). The end-user option should be a > config setting, if any. Agreed; users shouldn't need to specify such a parameter -- our heuristic should be good enough. René ``` ## Junio C Hamano, 2011-12-12 23:44 Subject: Re: [PATCH v3 0/3] grep attributes and multithreading Message-ID: <7vsjkpz763.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsjkpz763.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Thanks; sign-off? ``` ## Thomas Rast, 2011-12-13 08:44 Subject: Re: [PATCH v3 0/3] grep attributes and multithreading Message-ID: <201112130944.17429.trast@student.ethz.ch> URL: https://gitlist.dev/e/201112130944.17429.trast%40student.ethz.ch In-Reply-To: <7vsjkpz763.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Thanks; sign-off? Ooops, not again! > [PATCH v3 1/3] grep: load funcname patterns for -W Signed-off-by: Thomas Rast -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## Johannes Sixt, 2011-12-16 08:22 Subject: Re: [PATCH v3 2/3] grep: enable threading with -p and -W using lazy attribute lookup Message-ID: <4EEAFFAF.8030003@viscovery.net> URL: https://gitlist.dev/e/4EEAFFAF.8030003%40viscovery.net In-Reply-To: <138b930c0c96029f2fb9a816e73e70a4d5bce1de.1323723759.git.trast@student.ethz.ch> ``` Am 12/12/2011 22:16, schrieb Thomas Rast: > diff --git a/grep.h b/grep.h > index a652800..15d227c 100644 > --- a/grep.h > +++ b/grep.h > @@ -115,6 +115,7 @@ struct grep_opt { > int show_hunk_mark; > int file_break; > int heading; > + int use_threads; > void *priv; > > void (*output)(struct grep_opt *opt, const void *data, size_t size); > @@ -131,4 +132,10 @@ struct grep_opt { > 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! */ > +extern pthread_mutex_t grep_attr_mutex; > +#endif > + > #endif This is the first time we use pthread_mutex_t in a header file. We need at least the following squashed in. An alternative would be to include "thread-utils.h", but thread-utils is really more about implementation helpers functions, not about types, and therefore does not look right to be #included in a header. --- grep.h | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/grep.h b/grep.h index 15d227c..754b270 100644 --- a/grep.h +++ b/grep.h @@ -133,6 +133,7 @@ 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 +#include /* Mutex used around access to the attributes machinery if * opt->use_threads. Must be initialized/destroyed by callers! */ extern pthread_mutex_t grep_attr_mutex; -- 1.7.8.1436.gb3021 ``` ## Junio C Hamano, 2011-12-16 17:34 Subject: Re: [PATCH v3 2/3] grep: enable threading with -p and -W using lazy attribute lookup Message-ID: <7vvcpgifow.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vvcpgifow.fsf%40alter.siamese.dyndns.org In-Reply-To: <4EEAFFAF.8030003@viscovery.net> ``` Johannes Sixt writes: > This is the first time we use pthread_mutex_t in a header file. We need at > least the following squashed in. An alternative would be to include > "thread-utils.h", but thread-utils is really more about implementation > helpers functions, not about types,... builtin/grep.c already uses thread-utils.h since 5b594f4 (Threaded grep, 2010-01-25), so does builtin/pack-objects.c since 833e3df (pack-objects: Add runtime detection of online CPU's, 2008-02-22), so it may be simpler to do so in grep.h instead. diff --git a/builtin/grep.c b/builtin/grep.c index bc23c3c..6474eed 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -17,7 +17,6 @@ #include "grep.h" #include "quote.h" #include "dir.h" -#include "thread-utils.h" static char const * const grep_usage[] = { "git grep [options] [-e] [...] [[--] ...]", diff --git a/grep.h b/grep.h index 15d227c..dd4c65e 100644 --- a/grep.h +++ b/grep.h @@ -133,8 +133,11 @@ 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! */ +#include "thread-utils.h" +/* + * Mutex used around access to the attributes machinery if + * opt->use_threads. Must be initialized/destroyed by callers! + */ extern pthread_mutex_t grep_attr_mutex; #endif ```