{"thread":{"id":"29019","subject":"[PATCH] grep: load funcname patterns for -W","startedAt":"2011-11-25T14:46:18Z","lastAt":"2011-11-29T14:07:04Z","messageCount":6,"participants":["Thomas Rast","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"179968","messageId":"5e3bcf651b31b299ca411296e6e7c4d11f6ae617.1322232319.git.trast@student.ethz.ch","threadId":"29019","inReplyTo":null,"subject":"[PATCH] grep: load funcname patterns for -W","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-11-25T14:46:18Z","receivedAt":"2011-11-25T14:46:18Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"git-grep avoids loading the funcname patterns unless they are needed.\nba8ea74 (grep: add option to show whole function as context,\n2011-08-01) forgot to extend this test also to the new funcbody\nfeature.  Do so.\n\nThe catch is that we also have to disable threading when using\nuserdiff, as explained in grep_threads_ok().  So we must be careful to\nintroduce the same test there.\n---\n grep.c          |    7 ++++---\n t/t7810-grep.sh |   14 ++++++++++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex b29d09c..7a070e9 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -948,8 +948,8 @@ int grep_threads_ok(const struct grep_opt *opt)\n \t * machinery in grep_buffer_1. The attribute code is not\n \t * thread safe, so we disable the use of threads.\n \t */\n-\tif (opt->funcname && !opt->unmatch_name_only && !opt->status_only &&\n-\t    !opt->name_only)\n+\tif ((opt->funcname || opt->funcbody)\n+\t    && !opt->unmatch_name_only && !opt->status_only && !opt->name_only)\n \t\treturn 0;\n \n \treturn 1;\n@@ -1008,7 +1008,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t}\n \n \tmemset(&xecfg, 0, sizeof(xecfg));\n-\tif (opt->funcname && !opt->unmatch_name_only && !opt->status_only &&\n+\tif ((opt->funcname || opt->funcbody)\n+\t    && !opt->unmatch_name_only && !opt->status_only &&\n \t    !opt->name_only && !binary_match_only && !collect_hits) {\n \t\tstruct userdiff_driver *drv = userdiff_find_by_path(name);\n \t\tif (drv && drv->funcname.pattern) {\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 81263b7..7ba5b16 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -523,6 +523,20 @@ test_expect_success 'grep -W' '\n \ttest_cmp expected actual\n '\n \n+cat >expected <<EOF\n+hello.c=\tprintf(\"Hello world.\\n\");\n+hello.c:\treturn 0;\n+hello.c-\t/* char ?? */\n+EOF\n+\n+test_expect_success 'grep -W with userdiff' '\n+\ttest_when_finished \"rm -f .gitattributes\" &&\n+\tgit config diff.custom.xfuncname \"(printf.*|})$\" &&\n+\techo \"hello.c diff=custom\" >.gitattributes &&\n+\tgit grep -W return >actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'grep from a subdirectory to search wider area (1)' '\n \tmkdir -p s &&\n \t(\n-- \n1.7.8.rc3.415.gbcbb2\n"},{"id":"179975","messageId":"4ECFC320.4030003@lsrfire.ath.cx","threadId":"29019","inReplyTo":"5e3bcf651b31b299ca411296e6e7c4d11f6ae617.1322232319.git.trast@student.ethz.ch","subject":"Re: [PATCH] grep: load funcname patterns for -W","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-11-25T16:32:32Z","receivedAt":"2011-11-25T16:32:32Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.11.2011 15:46, schrieb Thomas Rast:\n> git-grep avoids loading the funcname patterns unless they are needed.\n> ba8ea74 (grep: add option to show whole function as context,\n> 2011-08-01) forgot to extend this test also to the new funcbody\n> feature.  Do so.\n> \n> The catch is that we also have to disable threading when using\n> userdiff, as explained in grep_threads_ok().  So we must be careful to\n> introduce the same test there.\n\nOops.  Thanks for catching this.\n\nThat reminds me to look into adding a thread-safe way to access\nattributes again..\n\nThanks,\nRené\n"},{"id":"179985","messageId":"4ED0D875.5050900@lsrfire.ath.cx","threadId":"29019","inReplyTo":"4ECFC320.4030003@lsrfire.ath.cx","subject":"[PATCH] grep: enable multi-threading for -p and -W","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-11-26T12:15:49Z","receivedAt":"2011-11-26T12:15:49Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Move attribute reading, which is not thread-safe, into add_work(), under\ngrep_mutex.  That way we can stop turning off multi-threading if -p or -W\nis given, as the diff attribute for each file is gotten safely now.\n\nSigned-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n---\nGoes on top of your patch.  Needs testing..\n\n builtin/grep.c |   33 +++++++++++++++++++++++++--------\n grep.c         |   25 +++++++------------------\n grep.h         |    5 +++--\n revision.c     |    2 +-\n 4 files changed, 36 insertions(+), 29 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3d7329d..5534111 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -18,6 +18,7 @@\n #include \"quote.h\"\n #include \"dir.h\"\n #include \"thread-utils.h\"\n+#include \"userdiff.h\"\n \n static char const * const grep_usage[] = {\n \t\"git grep [options] [-e] <pattern> [<rev>...] [[--] <path>...]\",\n@@ -43,6 +44,7 @@ enum work_type {WORK_SHA1, WORK_FILE};\n struct work_item {\n \tenum work_type type;\n \tchar *name;\n+\tstruct userdiff_driver *userdiff_driver;\n \n \t/* if type == WORK_SHA1, then 'identifier' is a SHA1,\n \t * otherwise type == WORK_FILE, and 'identifier' is a NUL\n@@ -114,7 +116,17 @@ static pthread_cond_t cond_result;\n \n static int skip_first_line;\n \n-static void add_work(enum work_type type, char *name, void *id)\n+/* Reading attributes is not thread-safe, so callers need to lock. */\n+static struct userdiff_driver *get_userdiff_driver(struct grep_opt *opt,\n+\t\t\t\t\t\t   const char *name)\n+{\n+\tif (opt->funcbody || opt->funcname)\n+\t\treturn userdiff_find_by_path(name);\n+\treturn NULL;\n+}\n+\n+static void add_work(struct grep_opt *opt, enum work_type type, char *name,\n+\t\t     void *id)\n {\n \tgrep_lock();\n \n@@ -124,6 +136,7 @@ static void add_work(enum work_type type, char *name, void *id)\n \n \ttodo[todo_end].type = type;\n \ttodo[todo_end].name = name;\n+\ttodo[todo_end].userdiff_driver = get_userdiff_driver(opt, name);\n \ttodo[todo_end].identifier = id;\n \ttodo[todo_end].done = 0;\n \tstrbuf_reset(&todo[todo_end].out);\n@@ -158,13 +171,13 @@ static void grep_sha1_async(struct grep_opt *opt, char *name,\n \tunsigned char *s;\n \ts = xmalloc(20);\n \tmemcpy(s, sha1, 20);\n-\tadd_work(WORK_SHA1, name, s);\n+\tadd_work(opt, WORK_SHA1, name, s);\n }\n \n static void grep_file_async(struct grep_opt *opt, char *name,\n \t\t\t    const char *filename)\n {\n-\tadd_work(WORK_FILE, name, xstrdup(filename));\n+\tadd_work(opt, WORK_FILE, name, xstrdup(filename));\n }\n \n static void work_done(struct work_item *w)\n@@ -212,24 +225,26 @@ static void *run(void *arg)\n \tstruct grep_opt *opt = arg;\n \n \twhile (1) {\n+\t\tstruct userdiff_driver *drv;\n \t\tstruct work_item *w = get_work();\n \t\tif (!w)\n \t\t\tbreak;\n \n \t\topt->output_priv = w;\n+\t\tdrv = w->userdiff_driver;\n \t\tif (w->type == WORK_SHA1) {\n \t\t\tunsigned long sz;\n \t\t\tvoid* data = load_sha1(w->identifier, &sz, w->name);\n \n \t\t\tif (data) {\n-\t\t\t\thit |= grep_buffer(opt, w->name, data, sz);\n+\t\t\t\thit |= grep_buffer(opt, w->name, drv, data, sz);\n \t\t\t\tfree(data);\n \t\t\t}\n \t\t} else if (w->type == WORK_FILE) {\n \t\t\tsize_t sz;\n \t\t\tvoid* data = load_file(w->identifier, &sz);\n \t\t\tif (data) {\n-\t\t\t\thit |= grep_buffer(opt, w->name, data, sz);\n+\t\t\t\thit |= grep_buffer(opt, w->name, drv, data, sz);\n \t\t\t\tfree(data);\n \t\t\t}\n \t\t} else {\n@@ -417,10 +432,11 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t\tint hit;\n \t\tunsigned long sz;\n \t\tvoid *data = load_sha1(sha1, &sz, name);\n+\t\tstruct userdiff_driver *drv = get_userdiff_driver(opt, name);\n \t\tif (!data)\n \t\t\thit = 0;\n \t\telse\n-\t\t\thit = grep_buffer(opt, name, data, sz);\n+\t\t\thit = grep_buffer(opt, name, drv, data, sz);\n \n \t\tfree(data);\n \t\tfree(name);\n@@ -479,10 +495,11 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tint hit;\n \t\tsize_t sz;\n \t\tvoid *data = load_file(filename, &sz);\n+\t\tstruct userdiff_driver *drv = get_userdiff_driver(opt, name);\n \t\tif (!data)\n \t\t\thit = 0;\n \t\telse\n-\t\t\thit = grep_buffer(opt, name, data, sz);\n+\t\t\thit = grep_buffer(opt, name, drv, data, sz);\n \n \t\tfree(data);\n \t\tfree(name);\n@@ -1001,7 +1018,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\topt.regflags |= REG_ICASE;\n \n #ifndef NO_PTHREADS\n-\tif (online_cpus() == 1 || !grep_threads_ok(&opt))\n+\tif (online_cpus() == 1)\n \t\tuse_threads = 0;\n \n \tif (use_threads) {\ndiff --git a/grep.c b/grep.c\nindex 7a070e9..ff14a98 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -942,25 +942,13 @@ static int look_ahead(struct grep_opt *opt,\n \treturn 0;\n }\n \n-int grep_threads_ok(const struct grep_opt *opt)\n-{\n-\t/* If this condition is true, then we may use the attribute\n-\t * machinery in grep_buffer_1. The attribute code is not\n-\t * thread safe, so we disable the use of threads.\n-\t */\n-\tif ((opt->funcname || opt->funcbody)\n-\t    && !opt->unmatch_name_only && !opt->status_only && !opt->name_only)\n-\t\treturn 0;\n-\n-\treturn 1;\n-}\n-\n static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n {\n \tfwrite(buf, size, 1, stdout);\n }\n \n static int grep_buffer_1(struct grep_opt *opt, const char *name,\n+\t\t\t struct userdiff_driver *drv,\n \t\t\t char *buf, unsigned long size, int collect_hits)\n {\n \tchar *bol = buf;\n@@ -1011,7 +999,6 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \tif ((opt->funcname || opt->funcbody)\n \t    && !opt->unmatch_name_only && !opt->status_only &&\n \t    !opt->name_only && !binary_match_only && !collect_hits) {\n-\t\tstruct userdiff_driver *drv = userdiff_find_by_path(name);\n \t\tif (drv && drv->funcname.pattern) {\n \t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n \t\t\txdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);\n@@ -1167,23 +1154,25 @@ static int chk_hit_marker(struct grep_expr *x)\n \t}\n }\n \n-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)\n+int grep_buffer(struct grep_opt *opt, const char *name,\n+\t\tstruct userdiff_driver *userdiff_driver,\n+\t\tchar *buf, unsigned long size)\n {\n \t/*\n \t * we do not have to do the two-pass grep when we do not check\n \t * buffer-wide \"all-match\".\n \t */\n \tif (!opt->all_match)\n-\t\treturn grep_buffer_1(opt, name, buf, size, 0);\n+\t\treturn grep_buffer_1(opt, name, userdiff_driver, buf, size, 0);\n \n \t/* Otherwise the toplevel \"or\" terms hit a bit differently.\n \t * We first clear hit markers from them.\n \t */\n \tclr_hit_marker(opt->pattern_expression);\n-\tgrep_buffer_1(opt, name, buf, size, 1);\n+\tgrep_buffer_1(opt, name, userdiff_driver, buf, size, 1);\n \n \tif (!chk_hit_marker(opt->pattern_expression))\n \t\treturn 0;\n \n-\treturn grep_buffer_1(opt, name, buf, size, 0);\n+\treturn grep_buffer_1(opt, name, userdiff_driver, buf, size, 0);\n }\ndiff --git a/grep.h b/grep.h\nindex a652800..20224b5 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -121,14 +121,15 @@ struct grep_opt {\n \tvoid *output_priv;\n };\n \n+struct userdiff_driver;\n+\n 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);\n extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t);\n extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *);\n extern void compile_grep_patterns(struct grep_opt *opt);\n extern void free_grep_patterns(struct grep_opt *opt);\n-extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);\n+extern int grep_buffer(struct grep_opt *opt, const char *name, struct userdiff_driver *userdiff_driver, char *buf, unsigned long size);\n \n extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);\n-extern int grep_threads_ok(const struct grep_opt *opt);\n \n #endif\ndiff --git a/revision.c b/revision.c\nindex 8764dde..95cb8c2 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2126,7 +2126,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)\n \tif (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)\n \t\treturn 1;\n \treturn grep_buffer(&opt->grep_filter,\n-\t\t\t   NULL, /* we say nothing, not even filename */\n+\t\t\t   NULL, NULL, /* we say nothing, not even filename */\n \t\t\t   commit->buffer, strlen(commit->buffer));\n }\n \n-- \n1.7.7\n"},{"id":"180078","messageId":"201111291054.39477.trast@student.ethz.ch","threadId":"29019","inReplyTo":"4ED0D875.5050900@lsrfire.ath.cx","subject":"Re: [PATCH] grep: enable multi-threading for -p and -W","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-11-29T09:54:39Z","receivedAt":"2011-11-29T09:54:39Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"René Scharfe wrote:\n> Move attribute reading, which is not thread-safe, into add_work(), under\n> grep_mutex.  That way we can stop turning off multi-threading if -p or -W\n> is given, as the diff attribute for each file is gotten safely now.\n> \n> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n> ---\n> Goes on top of your patch.  Needs testing..\n\nOn a random old linux-2.6 clone at v2.6.37-rc2, I'm seeing (best of 5):\n\n* none of the patches:\n  git grep --cached INITRAMFS_ROOT_UID\n    2.13user 0.14system 0:02.10elapsed\n  git grep --cached -W INITRAMFS_ROOT_UID    # note: broken!\n    2.23user 0.18system 0:02.21elapsed \n\n* my patch, but not yours:\n  git grep --cached INITRAMFS_ROOT_UID\n    2.21user 0.21system 0:02.27elapsed\n  git grep --cached -W INITRAMFS_ROOT_UID\n    3.01user 0.05system 0:03.07elapsed\n\n* my patch + your patch:\n  git grep --cached INITRAMFS_ROOT_UID\n    2.22user 0.17system 0:02.22elapsed\n  git grep --cached -W INITRAMFS_ROOT_UID\n    4.45user 0.22system 0:02.67elapsed\n\nSo while it ends up being faster overall, it also uses way more CPU\nand would presumably be *slower* on a single-processor system.\nApparently those attribute lookups really hurt.\n\nSo I had the following idea: if we load attributes only very lazily\n(that is, when match_funcname() is first called), we can ask for them\nmuch more rarely.  The revised timings:\n\n* my patch + the new patch at the end:\n  git grep --cached INITRAMFS_ROOT_UID\n    2.15user 0.16system 0:02.15elapsed 107%CPU\n  git grep --cached -W INITRAMFS_ROOT_UID\n    2.20user 0.18system 0:02.24elapsed\n\nHowever, locking on a specific lock relies on the fact that the\nattributes are only read from the tree, but never from the object\nstore.  Perhaps it would be more futureproof to lock on\nread_sha1_mutex instead.  Either way the lazy attributes lookup seems\na big win.\n\n------ 8< ------ 8< ------\nSubject: [PATCH] grep: fine-grained locking around attributes access\n\nLazily load the userdiff attributes in match_funcname().  Use a\nseparate mutex around this loading to protect the (not thread-safe)\nattributes machinery.  This lets us re-enable threading with -p and\n-W while reducing the overhead caused by looking up attributes.\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 988ea1d..822b32f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -256,6 +256,7 @@ static void start_threads(struct grep_opt *opt)\n \n \tpthread_mutex_init(&grep_mutex, NULL);\n \tpthread_mutex_init(&read_sha1_mutex, NULL);\n+\tpthread_mutex_init(&grep_attr_mutex, NULL);\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n@@ -303,6 +304,7 @@ static int wait_all(void)\n \n \tpthread_mutex_destroy(&grep_mutex);\n \tpthread_mutex_destroy(&read_sha1_mutex);\n+\tpthread_mutex_destroy(&grep_attr_mutex);\n \tpthread_cond_destroy(&cond_add);\n \tpthread_cond_destroy(&cond_write);\n \tpthread_cond_destroy(&cond_result);\n@@ -1002,9 +1004,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\topt.regflags |= REG_ICASE;\n \n #ifndef NO_PTHREADS\n-\tif (online_cpus() == 1 || !grep_threads_ok(&opt))\n+\tif (online_cpus() == 1)\n \t\tuse_threads = 0;\n+#endif\n+\n+\topt.use_threads = use_threads;\n \n+#ifndef NO_PTHREADS\n \tif (use_threads) {\n \t\tif (opt.pre_context || opt.post_context || opt.file_break ||\n \t\t    opt.funcbody)\ndiff --git a/grep.c b/grep.c\nindex 7a070e9..e9c3df3 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -2,6 +2,7 @@\n #include \"grep.h\"\n #include \"userdiff.h\"\n #include \"xdiff-interface.h\"\n+#include \"thread-utils.h\"\n \n void append_header_grep_pattern(struct grep_opt *opt, enum grep_header_field field, const char *pat)\n {\n@@ -806,10 +807,38 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n \topt->output(opt, \"\\n\", 1);\n }\n \n-static int match_funcname(struct grep_opt *opt, char *bol, char *eol)\n+#ifndef NO_PTHREADS\n+pthread_mutex_t grep_attr_mutex;\n+\n+static inline void grep_attr_lock(struct grep_opt *opt)\n+{\n+\tif (opt->use_threads)\n+\t\tpthread_mutex_lock(&grep_attr_mutex);\n+}\n+\n+static inline void grep_attr_unlock(struct grep_opt *opt)\n+{\n+\tif (opt->use_threads)\n+\t\tpthread_mutex_unlock(&grep_attr_mutex);\n+}\n+#endif\n+\n+static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)\n {\n \txdemitconf_t *xecfg = opt->priv;\n-\tif (xecfg && xecfg->find_func) {\n+\tif (xecfg && !xecfg->find_func) {\n+\t\tgrep_attr_lock(opt);\n+\t\tstruct userdiff_driver *drv = userdiff_find_by_path(name);\n+\t\tgrep_attr_unlock(opt);\n+\t\tif (drv && drv->funcname.pattern) {\n+\t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n+\t\t\txdiff_set_find_func(xecfg, pe->pattern, pe->cflags);\n+\t\t} else {\n+\t\t\txecfg = opt->priv = NULL;\n+\t\t}\n+\t}\n+\n+\tif (xecfg) {\n \t\tchar buf[1];\n \t\treturn xecfg->find_func(bol, eol - bol, buf, 1,\n \t\t\t\t\txecfg->find_func_priv) >= 0;\n@@ -835,7 +864,7 @@ static void show_funcname_line(struct grep_opt *opt, const char *name,\n \t\tif (lno <= opt->last_shown)\n \t\t\tbreak;\n \n-\t\tif (match_funcname(opt, bol, eol)) {\n+\t\tif (match_funcname(opt, name, bol, eol)) {\n \t\t\tshow_line(opt, bol, eol, name, lno, '=');\n \t\t\tbreak;\n \t\t}\n@@ -848,7 +877,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n \tunsigned cur = lno, from = 1, funcname_lno = 0;\n \tint funcname_needed = !!opt->funcname;\n \n-\tif (opt->funcbody && !match_funcname(opt, bol, end))\n+\tif (opt->funcbody && !match_funcname(opt, name, bol, end))\n \t\tfuncname_needed = 2;\n \n \tif (opt->pre_context < lno)\n@@ -864,7 +893,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n \t\twhile (bol > buf && bol[-1] != '\\n')\n \t\t\tbol--;\n \t\tcur--;\n-\t\tif (funcname_needed && match_funcname(opt, bol, eol)) {\n+\t\tif (funcname_needed && match_funcname(opt, name, bol, eol)) {\n \t\t\tfuncname_lno = cur;\n \t\t\tfuncname_needed = 0;\n \t\t}\n@@ -942,19 +971,6 @@ static int look_ahead(struct grep_opt *opt,\n \treturn 0;\n }\n \n-int grep_threads_ok(const struct grep_opt *opt)\n-{\n-\t/* If this condition is true, then we may use the attribute\n-\t * machinery in grep_buffer_1. The attribute code is not\n-\t * thread safe, so we disable the use of threads.\n-\t */\n-\tif ((opt->funcname || opt->funcbody)\n-\t    && !opt->unmatch_name_only && !opt->status_only && !opt->name_only)\n-\t\treturn 0;\n-\n-\treturn 1;\n-}\n-\n static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n {\n \tfwrite(buf, size, 1, stdout);\n@@ -1011,12 +1027,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \tif ((opt->funcname || opt->funcbody)\n \t    && !opt->unmatch_name_only && !opt->status_only &&\n \t    !opt->name_only && !binary_match_only && !collect_hits) {\n-\t\tstruct userdiff_driver *drv = userdiff_find_by_path(name);\n-\t\tif (drv && drv->funcname.pattern) {\n-\t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n-\t\t\txdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);\n-\t\t\topt->priv = &xecfg;\n-\t\t}\n+\t\topt->priv = &xecfg;\n \t}\n \ttry_lookahead = should_lookahead(opt);\n \n@@ -1093,7 +1104,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n \t\t\t\tshow_function = 1;\n \t\t\tgoto next_line;\n \t\t}\n-\t\tif (show_function && match_funcname(opt, bol, eol))\n+\t\tif (show_function && match_funcname(opt, name, bol, eol))\n \t\t\tshow_function = 0;\n \t\tif (show_function ||\n \t\t    (last_hit && lno <= last_hit + opt->post_context)) {\ndiff --git a/grep.h b/grep.h\nindex a652800..15d227c 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -115,6 +115,7 @@ struct grep_opt {\n \tint show_hunk_mark;\n \tint file_break;\n \tint heading;\n+\tint use_threads;\n \tvoid *priv;\n \n \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n@@ -131,4 +132,10 @@ struct grep_opt {\n extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);\n extern int grep_threads_ok(const struct grep_opt *opt);\n \n+#ifndef NO_PTHREADS\n+/* Mutex used around access to the attributes machinery if\n+ * opt->use_threads.  Must be initialized/destroyed by callers! */\n+extern pthread_mutex_t grep_attr_mutex;\n+#endif\n+\n #endif\n\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"180088","messageId":"4ED4E2D7.9090804@lsrfire.ath.cx","threadId":"29019","inReplyTo":"201111291054.39477.trast@student.ethz.ch","subject":"Re: [PATCH] grep: enable multi-threading for -p and -W","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-11-29T13:49:11Z","receivedAt":"2011-11-29T13:49:11Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 29.11.2011 10:54, schrieb Thomas Rast:\n> René Scharfe wrote:\n>> Move attribute reading, which is not thread-safe, into add_work(), under\n>> grep_mutex.  That way we can stop turning off multi-threading if -p or -W\n>> is given, as the diff attribute for each file is gotten safely now.\n>>\n>> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>\n>> ---\n>> Goes on top of your patch.  Needs testing..\n> \n> On a random old linux-2.6 clone at v2.6.37-rc2, I'm seeing (best of 5):\n> \n> * none of the patches:\n>   git grep --cached INITRAMFS_ROOT_UID\n>     2.13user 0.14system 0:02.10elapsed\n>   git grep --cached -W INITRAMFS_ROOT_UID    # note: broken!\n>     2.23user 0.18system 0:02.21elapsed \n\nBroken is a tad too hard a word; IIUC -W just lacks support for function\npatterns of different file types, which is unintended and surprising of\ncourse.  For the default case it works correctly (and threaded).\n\n> * my patch, but not yours:\n>   git grep --cached INITRAMFS_ROOT_UID\n>     2.21user 0.21system 0:02.27elapsed\n>   git grep --cached -W INITRAMFS_ROOT_UID\n>     3.01user 0.05system 0:03.07elapsed\n> \n> * my patch + your patch:\n>   git grep --cached INITRAMFS_ROOT_UID\n>     2.22user 0.17system 0:02.22elapsed\n>   git grep --cached -W INITRAMFS_ROOT_UID\n>     4.45user 0.22system 0:02.67elapsed\n> \n> So while it ends up being faster overall, it also uses way more CPU\n> and would presumably be *slower* on a single-processor system.\n> Apparently those attribute lookups really hurt.\n\nHmm, why are they *that* expensive?\n\ncallgrind tells me that userdiff_find_by_path() contributes only 0.18%\nto the total cost with your first patch.  Timings in my virtual machine\nare very volatile, but it seems that here the difference is in the\nsystem time while user is basically the same for all combinations of\npatches.\n\n> So I had the following idea: if we load attributes only very lazily\n> (that is, when match_funcname() is first called), we can ask for them\n> much more rarely.  The revised timings:\n> \n> * my patch + the new patch at the end:\n>   git grep --cached INITRAMFS_ROOT_UID\n>     2.15user 0.16system 0:02.15elapsed 107%CPU\n>   git grep --cached -W INITRAMFS_ROOT_UID\n>     2.20user 0.18system 0:02.24elapsed\n\nNice.\n\nI wonder what caused the slight speedup for the case without -W.  It's\nprobably just noise, though.\n\n> However, locking on a specific lock relies on the fact that the\n> attributes are only read from the tree, but never from the object\n> store.  Perhaps it would be more futureproof to lock on\n> read_sha1_mutex instead.  Either way the lazy attributes lookup seems\n> a big win.\n\nYou could lock read_sha1_mutex after grep_attr_mutex.  With lazy loading\nit should have a low impact most of the time.\n\n> ------ 8< ------ 8< ------\n> Subject: [PATCH] grep: fine-grained locking around attributes access\n> \n> Lazily load the userdiff attributes in match_funcname().  Use a\n> separate mutex around this loading to protect the (not thread-safe)\n> attributes machinery.  This lets us re-enable threading with -p and\n> -W while reducing the overhead caused by looking up attributes.\n> \n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 988ea1d..822b32f 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -256,6 +256,7 @@ static void start_threads(struct grep_opt *opt)\n>  \n>  \tpthread_mutex_init(&grep_mutex, NULL);\n>  \tpthread_mutex_init(&read_sha1_mutex, NULL);\n> +\tpthread_mutex_init(&grep_attr_mutex, NULL);\n>  \tpthread_cond_init(&cond_add, NULL);\n>  \tpthread_cond_init(&cond_write, NULL);\n>  \tpthread_cond_init(&cond_result, NULL);\n> @@ -303,6 +304,7 @@ static int wait_all(void)\n>  \n>  \tpthread_mutex_destroy(&grep_mutex);\n>  \tpthread_mutex_destroy(&read_sha1_mutex);\n> +\tpthread_mutex_destroy(&grep_attr_mutex);\n>  \tpthread_cond_destroy(&cond_add);\n>  \tpthread_cond_destroy(&cond_write);\n>  \tpthread_cond_destroy(&cond_result);\n> @@ -1002,9 +1004,13 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\topt.regflags |= REG_ICASE;\n>  \n>  #ifndef NO_PTHREADS\n> -\tif (online_cpus() == 1 || !grep_threads_ok(&opt))\n> +\tif (online_cpus() == 1)\n>  \t\tuse_threads = 0;\n> +#endif\n> +\n> +\topt.use_threads = use_threads;\n>  \n> +#ifndef NO_PTHREADS\n>  \tif (use_threads) {\n>  \t\tif (opt.pre_context || opt.post_context || opt.file_break ||\n>  \t\t    opt.funcbody)\n> diff --git a/grep.c b/grep.c\n> index 7a070e9..e9c3df3 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -2,6 +2,7 @@\n>  #include \"grep.h\"\n>  #include \"userdiff.h\"\n>  #include \"xdiff-interface.h\"\n> +#include \"thread-utils.h\"\n>  \n>  void append_header_grep_pattern(struct grep_opt *opt, enum grep_header_field field, const char *pat)\n>  {\n> @@ -806,10 +807,38 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,\n>  \topt->output(opt, \"\\n\", 1);\n>  }\n>  \n> -static int match_funcname(struct grep_opt *opt, char *bol, char *eol)\n> +#ifndef NO_PTHREADS\n> +pthread_mutex_t grep_attr_mutex;\n> +\n> +static inline void grep_attr_lock(struct grep_opt *opt)\n> +{\n> +\tif (opt->use_threads)\n> +\t\tpthread_mutex_lock(&grep_attr_mutex);\n> +}\n> +\n> +static inline void grep_attr_unlock(struct grep_opt *opt)\n> +{\n> +\tif (opt->use_threads)\n> +\t\tpthread_mutex_unlock(&grep_attr_mutex);\n> +}\n> +#endif\n\nWe'd need stubs here for the case of NO_PTHREADS being defined.\n\n> +\n> +static int match_funcname(struct grep_opt *opt, const char *name, char *bol, char *eol)\n>  {\n>  \txdemitconf_t *xecfg = opt->priv;\n> -\tif (xecfg && xecfg->find_func) {\n> +\tif (xecfg && !xecfg->find_func) {\n> +\t\tgrep_attr_lock(opt);\n> +\t\tstruct userdiff_driver *drv = userdiff_find_by_path(name);\n> +\t\tgrep_attr_unlock(opt);\n> +\t\tif (drv && drv->funcname.pattern) {\n> +\t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n> +\t\t\txdiff_set_find_func(xecfg, pe->pattern, pe->cflags);\n> +\t\t} else {\n> +\t\t\txecfg = opt->priv = NULL;\n> +\t\t}\n> +\t}\n\nPerhaps leave the thread stuff in builtin/grep.c and export a function\nfrom there that does the above, with locking and all?\n\n> +\n> +\tif (xecfg) {\n>  \t\tchar buf[1];\n>  \t\treturn xecfg->find_func(bol, eol - bol, buf, 1,\n>  \t\t\t\t\txecfg->find_func_priv) >= 0;\n> @@ -835,7 +864,7 @@ static void show_funcname_line(struct grep_opt *opt, const char *name,\n>  \t\tif (lno <= opt->last_shown)\n>  \t\t\tbreak;\n>  \n> -\t\tif (match_funcname(opt, bol, eol)) {\n> +\t\tif (match_funcname(opt, name, bol, eol)) {\n>  \t\t\tshow_line(opt, bol, eol, name, lno, '=');\n>  \t\t\tbreak;\n>  \t\t}\n> @@ -848,7 +877,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n>  \tunsigned cur = lno, from = 1, funcname_lno = 0;\n>  \tint funcname_needed = !!opt->funcname;\n>  \n> -\tif (opt->funcbody && !match_funcname(opt, bol, end))\n> +\tif (opt->funcbody && !match_funcname(opt, name, bol, end))\n>  \t\tfuncname_needed = 2;\n>  \n>  \tif (opt->pre_context < lno)\n> @@ -864,7 +893,7 @@ static void show_pre_context(struct grep_opt *opt, const char *name, char *buf,\n>  \t\twhile (bol > buf && bol[-1] != '\\n')\n>  \t\t\tbol--;\n>  \t\tcur--;\n> -\t\tif (funcname_needed && match_funcname(opt, bol, eol)) {\n> +\t\tif (funcname_needed && match_funcname(opt, name, bol, eol)) {\n>  \t\t\tfuncname_lno = cur;\n>  \t\t\tfuncname_needed = 0;\n>  \t\t}\n> @@ -942,19 +971,6 @@ static int look_ahead(struct grep_opt *opt,\n>  \treturn 0;\n>  }\n>  \n> -int grep_threads_ok(const struct grep_opt *opt)\n> -{\n> -\t/* If this condition is true, then we may use the attribute\n> -\t * machinery in grep_buffer_1. The attribute code is not\n> -\t * thread safe, so we disable the use of threads.\n> -\t */\n> -\tif ((opt->funcname || opt->funcbody)\n> -\t    && !opt->unmatch_name_only && !opt->status_only && !opt->name_only)\n> -\t\treturn 0;\n> -\n> -\treturn 1;\n> -}\n> -\n>  static void std_output(struct grep_opt *opt, const void *buf, size_t size)\n>  {\n>  \tfwrite(buf, size, 1, stdout);\n> @@ -1011,12 +1027,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n>  \tif ((opt->funcname || opt->funcbody)\n>  \t    && !opt->unmatch_name_only && !opt->status_only &&\n>  \t    !opt->name_only && !binary_match_only && !collect_hits) {\n> -\t\tstruct userdiff_driver *drv = userdiff_find_by_path(name);\n> -\t\tif (drv && drv->funcname.pattern) {\n> -\t\t\tconst struct userdiff_funcname *pe = &drv->funcname;\n> -\t\t\txdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);\n> -\t\t\topt->priv = &xecfg;\n> -\t\t}\n> +\t\topt->priv = &xecfg;\n>  \t}\n\nopt->priv can be set unconditionally here now, since we only access it\nfrom within match_funcname() and that function is not called if any of\nthe short-circuit options is set.\n\n>  \ttry_lookahead = should_lookahead(opt);\n>  \n> @@ -1093,7 +1104,7 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,\n>  \t\t\t\tshow_function = 1;\n>  \t\t\tgoto next_line;\n>  \t\t}\n> -\t\tif (show_function && match_funcname(opt, bol, eol))\n> +\t\tif (show_function && match_funcname(opt, name, bol, eol))\n>  \t\t\tshow_function = 0;\n>  \t\tif (show_function ||\n>  \t\t    (last_hit && lno <= last_hit + opt->post_context)) {\n> diff --git a/grep.h b/grep.h\n> index a652800..15d227c 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -115,6 +115,7 @@ struct grep_opt {\n>  \tint show_hunk_mark;\n>  \tint file_break;\n>  \tint heading;\n> +\tint use_threads;\n>  \tvoid *priv;\n>  \n>  \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n> @@ -131,4 +132,10 @@ struct grep_opt {\n>  extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);\n>  extern int grep_threads_ok(const struct grep_opt *opt);\n>  \n> +#ifndef NO_PTHREADS\n> +/* Mutex used around access to the attributes machinery if\n> + * opt->use_threads.  Must be initialized/destroyed by callers! */\n> +extern pthread_mutex_t grep_attr_mutex;\n> +#endif\n> +\n>  #endif\n> \n> \n"},{"id":"180090","messageId":"201111291507.04754.trast@student.ethz.ch","threadId":"29019","inReplyTo":"4ED4E2D7.9090804@lsrfire.ath.cx","subject":"Re: [PATCH] grep: enable multi-threading for -p and -W","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-11-29T14:07:04Z","receivedAt":"2011-11-29T14:07:04Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"René Scharfe wrote:\n> > * none of the patches:\n> >   git grep --cached INITRAMFS_ROOT_UID\n> >     2.13user 0.14system 0:02.10elapsed\n> >   git grep --cached -W INITRAMFS_ROOT_UID    # note: broken!\n> >     2.23user 0.18system 0:02.21elapsed \n> \n> Broken is a tad too hard a word\n\nSorry, I just wanted to mark it as: this is unattainable since we're\nnow doing extra work.\n\n> > * my patch, but not yours:\n> >   git grep --cached -W INITRAMFS_ROOT_UID\n> >     3.01user 0.05system 0:03.07elapsed\n> > \n> > * my patch + your patch:\n> >   git grep --cached -W INITRAMFS_ROOT_UID\n> >     4.45user 0.22system 0:02.67elapsed\n> > \n> > So while it ends up being faster overall, it also uses way more CPU\n> > and would presumably be *slower* on a single-processor system.\n> > Apparently those attribute lookups really hurt.\n> \n> Hmm, why are they *that* expensive?\n> \n> callgrind tells me that userdiff_find_by_path() contributes only 0.18%\n> to the total cost with your first patch.  Timings in my virtual machine\n> are very volatile, but it seems that here the difference is in the\n> system time while user is basically the same for all combinations of\n> patches.\n\nNot sure, perhaps callgrind does not model syscalls as expensive\nenough.  I also suspect (from my very cursory look at the attributes\nmachinery) that loading attributes for all paths *in random order*\n(i.e., threaded) causes a lot of discards of the existing attributes\ndata.  (The order is still random with my new patch, but we only load\nthem for files that have matches.)\n\n> I wonder what caused the slight speedup for the case without -W.  It's\n> probably just noise, though.\n\nYeah, it's very noisy, but I was too lazy for best-of-50 or something ;-)\n\n[...]\n> > +#ifndef NO_PTHREADS\n> > +static inline void grep_attr_lock(struct grep_opt *opt)\n[...]\n> We'd need stubs here for the case of NO_PTHREADS being defined.\n\nRight, thanks.  Sorry for not testing with NO_PTHREADS to begin with.\n\n> Perhaps leave the thread stuff in builtin/grep.c and export a function\n> from there that does [the userdiff lookup], with locking and all?\n\nThat seems like a layering violation to me.  Isn't builtin/grep.c\nsupposed to call out to grep.c, but not the other way around?\n\nMaybe it would be less messy if we had a global (across all of git)\nflag that says whether threads are on.  Then subsystems that are used\nfrom threaded code, but cannot handle it, can learn to lock themselves\naround their work.  But it would be pretty much the opposite of what\ngit-grep now does.\n\n\nThanks for the review.  I'll reroll as a proper patch later.\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"}]}