# [PATCH] grep: load funcname patterns for -W 6 messages from 2011-11-25 to 2011-11-29. Participants: Thomas Rast, René Scharfe. Thread: https://gitlist.dev/t/29019 ## Thomas Rast, 2011-11-25 14:46 Subject: [PATCH] grep: load funcname patterns for -W Message-ID: <5e3bcf651b31b299ca411296e6e7c4d11f6ae617.1322232319.git.trast@student.ethz.ch> URL: https://gitlist.dev/e/5e3bcf651b31b299ca411296e6e7c4d11f6ae617.1322232319.git.trast%40student.ethz.ch ``` 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.rc3.415.gbcbb2 ``` ## René Scharfe, 2011-11-25 16:32 Subject: Re: [PATCH] grep: load funcname patterns for -W Message-ID: <4ECFC320.4030003@lsrfire.ath.cx> URL: https://gitlist.dev/e/4ECFC320.4030003%40lsrfire.ath.cx In-Reply-To: <5e3bcf651b31b299ca411296e6e7c4d11f6ae617.1322232319.git.trast@student.ethz.ch> ``` Am 25.11.2011 15:46, schrieb Thomas Rast: > 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. Oops. Thanks for catching this. That reminds me to look into adding a thread-safe way to access attributes again.. Thanks, René ``` ## René Scharfe, 2011-11-26 12:15 Subject: [PATCH] grep: enable multi-threading for -p and -W Message-ID: <4ED0D875.5050900@lsrfire.ath.cx> URL: https://gitlist.dev/e/4ED0D875.5050900%40lsrfire.ath.cx In-Reply-To: <4ECFC320.4030003@lsrfire.ath.cx> ``` Move attribute reading, which is not thread-safe, into add_work(), under grep_mutex. That way we can stop turning off multi-threading if -p or -W is given, as the diff attribute for each file is gotten safely now. Signed-off-by: Rene Scharfe --- Goes on top of your patch. Needs testing.. builtin/grep.c | 33 +++++++++++++++++++++++++-------- grep.c | 25 +++++++------------------ grep.h | 5 +++-- revision.c | 2 +- 4 files changed, 36 insertions(+), 29 deletions(-) diff --git a/builtin/grep.c b/builtin/grep.c index 3d7329d..5534111 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -18,6 +18,7 @@ #include "quote.h" #include "dir.h" #include "thread-utils.h" +#include "userdiff.h" static char const * const grep_usage[] = { "git grep [options] [-e] [...] [[--] ...]", @@ -43,6 +44,7 @@ enum work_type {WORK_SHA1, WORK_FILE}; struct work_item { enum work_type type; char *name; + struct userdiff_driver *userdiff_driver; /* if type == WORK_SHA1, then 'identifier' is a SHA1, * otherwise type == WORK_FILE, and 'identifier' is a NUL @@ -114,7 +116,17 @@ static pthread_cond_t cond_result; static int skip_first_line; -static void add_work(enum work_type type, char *name, void *id) +/* Reading attributes is not thread-safe, so callers need to lock. */ +static struct userdiff_driver *get_userdiff_driver(struct grep_opt *opt, + const char *name) +{ + if (opt->funcbody || opt->funcname) + return userdiff_find_by_path(name); + return NULL; +} + +static void add_work(struct grep_opt *opt, enum work_type type, char *name, + void *id) { grep_lock(); @@ -124,6 +136,7 @@ static void add_work(enum work_type type, char *name, void *id) todo[todo_end].type = type; todo[todo_end].name = name; + todo[todo_end].userdiff_driver = get_userdiff_driver(opt, name); todo[todo_end].identifier = id; todo[todo_end].done = 0; strbuf_reset(&todo[todo_end].out); @@ -158,13 +171,13 @@ static void grep_sha1_async(struct grep_opt *opt, char *name, unsigned char *s; s = xmalloc(20); memcpy(s, sha1, 20); - add_work(WORK_SHA1, name, s); + add_work(opt, WORK_SHA1, name, s); } static void grep_file_async(struct grep_opt *opt, char *name, const char *filename) { - add_work(WORK_FILE, name, xstrdup(filename)); + add_work(opt, WORK_FILE, name, xstrdup(filename)); } static void work_done(struct work_item *w) @@ -212,24 +225,26 @@ static void *run(void *arg) struct grep_opt *opt = arg; while (1) { + struct userdiff_driver *drv; struct work_item *w = get_work(); if (!w) break; opt->output_priv = w; + drv = w->userdiff_driver; if (w->type == WORK_SHA1) { unsigned long sz; void* data = load_sha1(w->identifier, &sz, w->name); if (data) { - hit |= grep_buffer(opt, w->name, data, sz); + hit |= grep_buffer(opt, w->name, drv, data, sz); free(data); } } else if (w->type == WORK_FILE) { size_t sz; void* data = load_file(w->identifier, &sz); if (data) { - hit |= grep_buffer(opt, w->name, data, sz); + hit |= grep_buffer(opt, w->name, drv, data, sz); free(data); } } else { @@ -417,10 +432,11 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1, int hit; unsigned long sz; void *data = load_sha1(sha1, &sz, name); + struct userdiff_driver *drv = get_userdiff_driver(opt, name); if (!data) hit = 0; else - hit = grep_buffer(opt, name, data, sz); + hit = grep_buffer(opt, name, drv, data, sz); free(data); free(name); @@ -479,10 +495,11 @@ static int grep_file(struct grep_opt *opt, const char *filename) int hit; size_t sz; void *data = load_file(filename, &sz); + struct userdiff_driver *drv = get_userdiff_driver(opt, name); if (!data) hit = 0; else - hit = grep_buffer(opt, name, data, sz); + hit = grep_buffer(opt, name, drv, data, sz); free(data); free(name); @@ -1001,7 +1018,7 @@ 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; if (use_threads) { diff --git a/grep.c b/grep.c index 7a070e9..ff14a98 100644 --- a/grep.c +++ b/grep.c @@ -942,25 +942,13 @@ 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); } static int grep_buffer_1(struct grep_opt *opt, const char *name, + struct userdiff_driver *drv, char *buf, unsigned long size, int collect_hits) { char *bol = buf; @@ -1011,7 +999,6 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, 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); @@ -1167,23 +1154,25 @@ static int chk_hit_marker(struct grep_expr *x) } } -int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size) +int grep_buffer(struct grep_opt *opt, const char *name, + struct userdiff_driver *userdiff_driver, + char *buf, unsigned long size) { /* * we do not have to do the two-pass grep when we do not check * buffer-wide "all-match". */ if (!opt->all_match) - return grep_buffer_1(opt, name, buf, size, 0); + return grep_buffer_1(opt, name, userdiff_driver, buf, size, 0); /* Otherwise the toplevel "or" terms hit a bit differently. * We first clear hit markers from them. */ clr_hit_marker(opt->pattern_expression); - grep_buffer_1(opt, name, buf, size, 1); + grep_buffer_1(opt, name, userdiff_driver, buf, size, 1); if (!chk_hit_marker(opt->pattern_expression)) return 0; - return grep_buffer_1(opt, name, buf, size, 0); + return grep_buffer_1(opt, name, userdiff_driver, buf, size, 0); } diff --git a/grep.h b/grep.h index a652800..20224b5 100644 --- a/grep.h +++ b/grep.h @@ -121,14 +121,15 @@ struct grep_opt { void *output_priv; }; +struct userdiff_driver; + extern void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t); extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t); extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *); extern void compile_grep_patterns(struct grep_opt *opt); extern void free_grep_patterns(struct grep_opt *opt); -extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size); +extern int grep_buffer(struct grep_opt *opt, const char *name, struct userdiff_driver *userdiff_driver, char *buf, unsigned long size); extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt); -extern int grep_threads_ok(const struct grep_opt *opt); #endif diff --git a/revision.c b/revision.c index 8764dde..95cb8c2 100644 --- a/revision.c +++ b/revision.c @@ -2126,7 +2126,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt) if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list) return 1; return grep_buffer(&opt->grep_filter, - NULL, /* we say nothing, not even filename */ + NULL, NULL, /* we say nothing, not even filename */ commit->buffer, strlen(commit->buffer)); } -- 1.7.7 ``` ## Thomas Rast, 2011-11-29 09:54 Subject: Re: [PATCH] grep: enable multi-threading for -p and -W Message-ID: <201111291054.39477.trast@student.ethz.ch> URL: https://gitlist.dev/e/201111291054.39477.trast%40student.ethz.ch In-Reply-To: <4ED0D875.5050900@lsrfire.ath.cx> ``` René Scharfe wrote: > Move attribute reading, which is not thread-safe, into add_work(), under > grep_mutex. That way we can stop turning off multi-threading if -p or -W > is given, as the diff attribute for each file is gotten safely now. > > Signed-off-by: Rene Scharfe > --- > Goes on top of your patch. Needs testing.. On a random old linux-2.6 clone at v2.6.37-rc2, I'm seeing (best of 5): * none of the patches: git grep --cached INITRAMFS_ROOT_UID 2.13user 0.14system 0:02.10elapsed git grep --cached -W INITRAMFS_ROOT_UID # note: broken! 2.23user 0.18system 0:02.21elapsed * my patch, but not yours: git grep --cached INITRAMFS_ROOT_UID 2.21user 0.21system 0:02.27elapsed git grep --cached -W INITRAMFS_ROOT_UID 3.01user 0.05system 0:03.07elapsed * my patch + your patch: git grep --cached INITRAMFS_ROOT_UID 2.22user 0.17system 0:02.22elapsed git grep --cached -W INITRAMFS_ROOT_UID 4.45user 0.22system 0:02.67elapsed So while it ends up being faster overall, it also uses way more CPU and would presumably be *slower* on a single-processor system. Apparently those attribute lookups really hurt. So I had the following idea: if we load attributes only very lazily (that is, when match_funcname() is first called), we can ask for them much more rarely. The revised timings: * my patch + the new patch at the end: git grep --cached INITRAMFS_ROOT_UID 2.15user 0.16system 0:02.15elapsed 107%CPU git grep --cached -W INITRAMFS_ROOT_UID 2.20user 0.18system 0:02.24elapsed However, locking on a specific lock relies on the fact that the attributes are only read from the tree, but never from the object store. Perhaps it would be more futureproof to lock on read_sha1_mutex instead. Either way the lazy attributes lookup seems a big win. ------ 8< ------ 8< ------ Subject: [PATCH] grep: fine-grained locking around attributes access 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. diff --git a/builtin/grep.c b/builtin/grep.c index 988ea1d..822b32f 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,9 +1004,13 @@ 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; +#endif + + opt.use_threads = use_threads; +#ifndef NO_PTHREADS if (use_threads) { if (opt.pre_context || opt.post_context || opt.file_break || opt.funcbody) diff --git a/grep.c b/grep.c index 7a070e9..e9c3df3 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,38 @@ 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 +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); +} +#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) { + grep_attr_lock(opt); + struct userdiff_driver *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 +864,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 +877,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 +893,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 +971,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); @@ -1011,12 +1027,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, 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); @@ -1093,7 +1104,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 -- Thomas Rast trast@{inf,student}.ethz.ch ``` ## René Scharfe, 2011-11-29 13:49 Subject: Re: [PATCH] grep: enable multi-threading for -p and -W Message-ID: <4ED4E2D7.9090804@lsrfire.ath.cx> URL: https://gitlist.dev/e/4ED4E2D7.9090804%40lsrfire.ath.cx In-Reply-To: <201111291054.39477.trast@student.ethz.ch> ``` Am 29.11.2011 10:54, schrieb Thomas Rast: > René Scharfe wrote: >> Move attribute reading, which is not thread-safe, into add_work(), under >> grep_mutex. That way we can stop turning off multi-threading if -p or -W >> is given, as the diff attribute for each file is gotten safely now. >> >> Signed-off-by: Rene Scharfe >> --- >> Goes on top of your patch. Needs testing.. > > On a random old linux-2.6 clone at v2.6.37-rc2, I'm seeing (best of 5): > > * none of the patches: > git grep --cached INITRAMFS_ROOT_UID > 2.13user 0.14system 0:02.10elapsed > git grep --cached -W INITRAMFS_ROOT_UID # note: broken! > 2.23user 0.18system 0:02.21elapsed Broken is a tad too hard a word; IIUC -W just lacks support for function patterns of different file types, which is unintended and surprising of course. For the default case it works correctly (and threaded). > * my patch, but not yours: > git grep --cached INITRAMFS_ROOT_UID > 2.21user 0.21system 0:02.27elapsed > git grep --cached -W INITRAMFS_ROOT_UID > 3.01user 0.05system 0:03.07elapsed > > * my patch + your patch: > git grep --cached INITRAMFS_ROOT_UID > 2.22user 0.17system 0:02.22elapsed > git grep --cached -W INITRAMFS_ROOT_UID > 4.45user 0.22system 0:02.67elapsed > > So while it ends up being faster overall, it also uses way more CPU > and would presumably be *slower* on a single-processor system. > Apparently those attribute lookups really hurt. Hmm, why are they *that* expensive? callgrind tells me that userdiff_find_by_path() contributes only 0.18% to the total cost with your first patch. Timings in my virtual machine are very volatile, but it seems that here the difference is in the system time while user is basically the same for all combinations of patches. > So I had the following idea: if we load attributes only very lazily > (that is, when match_funcname() is first called), we can ask for them > much more rarely. The revised timings: > > * my patch + the new patch at the end: > git grep --cached INITRAMFS_ROOT_UID > 2.15user 0.16system 0:02.15elapsed 107%CPU > git grep --cached -W INITRAMFS_ROOT_UID > 2.20user 0.18system 0:02.24elapsed Nice. I wonder what caused the slight speedup for the case without -W. It's probably just noise, though. > However, locking on a specific lock relies on the fact that the > attributes are only read from the tree, but never from the object > store. Perhaps it would be more futureproof to lock on > read_sha1_mutex instead. Either way the lazy attributes lookup seems > a big win. You could lock read_sha1_mutex after grep_attr_mutex. With lazy loading it should have a low impact most of the time. > ------ 8< ------ 8< ------ > Subject: [PATCH] grep: fine-grained locking around attributes access > > 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. > > diff --git a/builtin/grep.c b/builtin/grep.c > index 988ea1d..822b32f 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,9 +1004,13 @@ 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; > +#endif > + > + opt.use_threads = use_threads; > > +#ifndef NO_PTHREADS > if (use_threads) { > if (opt.pre_context || opt.post_context || opt.file_break || > opt.funcbody) > diff --git a/grep.c b/grep.c > index 7a070e9..e9c3df3 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,38 @@ 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 > +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); > +} > +#endif We'd need stubs here for the case of NO_PTHREADS being defined. > + > +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) { > + grep_attr_lock(opt); > + struct userdiff_driver *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; > + } > + } Perhaps leave the thread stuff in builtin/grep.c and export a function from there that does the above, with locking and all? > + > + if (xecfg) { > char buf[1]; > return xecfg->find_func(bol, eol - bol, buf, 1, > xecfg->find_func_priv) >= 0; > @@ -835,7 +864,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 +877,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 +893,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 +971,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); > @@ -1011,12 +1027,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name, > 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; > } opt->priv can be set unconditionally here now, since we only access it from within match_funcname() and that function is not called if any of the short-circuit options is set. > try_lookahead = should_lookahead(opt); > > @@ -1093,7 +1104,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 > > ``` ## Thomas Rast, 2011-11-29 14:07 Subject: Re: [PATCH] grep: enable multi-threading for -p and -W Message-ID: <201111291507.04754.trast@student.ethz.ch> URL: https://gitlist.dev/e/201111291507.04754.trast%40student.ethz.ch In-Reply-To: <4ED4E2D7.9090804@lsrfire.ath.cx> ``` René Scharfe wrote: > > * none of the patches: > > git grep --cached INITRAMFS_ROOT_UID > > 2.13user 0.14system 0:02.10elapsed > > git grep --cached -W INITRAMFS_ROOT_UID # note: broken! > > 2.23user 0.18system 0:02.21elapsed > > Broken is a tad too hard a word Sorry, I just wanted to mark it as: this is unattainable since we're now doing extra work. > > * my patch, but not yours: > > git grep --cached -W INITRAMFS_ROOT_UID > > 3.01user 0.05system 0:03.07elapsed > > > > * my patch + your patch: > > git grep --cached -W INITRAMFS_ROOT_UID > > 4.45user 0.22system 0:02.67elapsed > > > > So while it ends up being faster overall, it also uses way more CPU > > and would presumably be *slower* on a single-processor system. > > Apparently those attribute lookups really hurt. > > Hmm, why are they *that* expensive? > > callgrind tells me that userdiff_find_by_path() contributes only 0.18% > to the total cost with your first patch. Timings in my virtual machine > are very volatile, but it seems that here the difference is in the > system time while user is basically the same for all combinations of > patches. Not sure, perhaps callgrind does not model syscalls as expensive enough. I also suspect (from my very cursory look at the attributes machinery) that loading attributes for all paths *in random order* (i.e., threaded) causes a lot of discards of the existing attributes data. (The order is still random with my new patch, but we only load them for files that have matches.) > I wonder what caused the slight speedup for the case without -W. It's > probably just noise, though. Yeah, it's very noisy, but I was too lazy for best-of-50 or something ;-) [...] > > +#ifndef NO_PTHREADS > > +static inline void grep_attr_lock(struct grep_opt *opt) [...] > We'd need stubs here for the case of NO_PTHREADS being defined. Right, thanks. Sorry for not testing with NO_PTHREADS to begin with. > Perhaps leave the thread stuff in builtin/grep.c and export a function > from there that does [the userdiff lookup], with locking and all? That seems like a layering violation to me. Isn't builtin/grep.c supposed to call out to grep.c, but not the other way around? Maybe it would be less messy if we had a global (across all of git) flag that says whether threads are on. Then subsystems that are used from threaded code, but cannot handle it, can learn to lock themselves around their work. But it would be pretty much the opposite of what git-grep now does. Thanks for the review. I'll reroll as a proper patch later. -- Thomas Rast trast@{inf,student}.ethz.ch ```