{"thread":{"id":"29142","subject":"[PATCH v3 3/3] grep: disable threading in non-worktree case","startedAt":"2011-12-12T21:16:05Z","lastAt":"2011-12-16T17:34:07Z","messageCount":9,"participants":["Thomas Rast","René Scharfe","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":3,"patchTotal":3},"messages":[{"id":"180969","messageId":"cover.1323723759.git.trast@student.ethz.ch","threadId":"29142","inReplyTo":"4ED8F9AE.8030605@lsrfire.ath.cx","subject":"[PATCH v3 0/3] grep attributes and multithreading","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T21:16:05Z","receivedAt":"2011-12-12T21:16:05Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"I think we should finish up these three patches for the next release.\nThe first two are unchanged from last time; nobody seemed to have any\nobjections.\n\nOn the third one I'm following René's argument:\n\n> Am 02.12.2011 14:07, schrieb Thomas Rast:\n> > So disable threading entirely when not scanning the worktree, to get\n> > the NO_PTHREADS performance in that case.  This obsoletes all code\n> > related to grep_sha1_async.  The thread startup must be delayed until\n> > after all arguments have been parsed, but this does not have a\n> > measurable effect.\n> \n> This is a bit radical.  I think the underlying issue that \n> read_sha1_file() is not thread-safe can be solved eventually and then \n> we'd need to readd that code.\n\nI already posted a bunch of POC patches, but doing the timing manually\nhas been getting on my nerves lately.  So I would first like to\nformalize some of the performance testing, perhaps along lines already\ndrawn up by Michael Hagger, perhaps not.  Then we can revisit the\nissue of grep performance.  But I would prefer not to block the -W fix\nand two easy and confirmed speedups on that.\n\nI dropped this part entirely:\n\n> How about adding a parameter to control the number of threads \n> (--threads?) instead that defaults to eight (or five) for the worktree \n> and one for the rest?  That would also make benchmarking easier.\n\nIt does make testing a lot easier, but the interface is IMHO not fit\nfor users and I have a feeling that the \"right\" for-debugging\ninterface will end up falling out of the performance testing work\n(probably an environment variable).  The end-user option should be a\nconfig setting, if any.\n\n\nThomas Rast (3):\n  grep: load funcname patterns for -W\n  grep: enable threading with -p and -W using lazy attribute lookup\n  grep: disable threading in non-worktree case\n\n builtin/grep.c  |   34 +++++++++++++++----------\n grep.c          |   73 ++++++++++++++++++++++++++++++++++---------------------\n grep.h          |    7 +++++\n t/t7810-grep.sh |   14 ++++++++++\n 4 files changed, 86 insertions(+), 42 deletions(-)\n\n-- \n1.7.8.431.g2abf2\n"},{"id":"180968","messageId":"5abfcd11512c0118465459dd8cf43980aec7ceee.1323723759.git.trast@student.ethz.ch","threadId":"29142","inReplyTo":"cover.1323723759.git.trast@student.ethz.ch","subject":"[PATCH v3 1/3] grep: load funcname patterns for -W","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T21:16:06Z","receivedAt":"2011-12-12T21:16:06Z","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.431.g2abf2\n"},{"id":"180970","messageId":"138b930c0c96029f2fb9a816e73e70a4d5bce1de.1323723759.git.trast@student.ethz.ch","threadId":"29142","inReplyTo":"cover.1323723759.git.trast@student.ethz.ch","subject":"[PATCH v3 2/3] grep: enable threading with -p and -W using lazy attribute lookup","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T21:16:07Z","receivedAt":"2011-12-12T21:16:07Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Lazily 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\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n builtin/grep.c |   12 +++++++--\n grep.c         |   74 ++++++++++++++++++++++++++++++++++----------------------\n grep.h         |    7 +++++\n 3 files changed, 61 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 988ea1d..bc23c3c 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,17 +1004,21 @@ 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+#else\n+\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 \t\t\tskip_first_line = 1;\n \t\tstart_threads(&opt);\n \t}\n-#else\n-\tuse_threads = 0;\n #endif\n \n \tcompile_grep_patterns(&opt);\ndiff --git a/grep.c b/grep.c\nindex 7a070e9..4dd7da2 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,46 @@ 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+/*\n+ * This lock protects access to the gitattributes machinery, which is\n+ * not thread-safe.\n+ */\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+#else\n+#define grep_attr_lock(opt)\n+#define grep_attr_unlock(opt)\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\tstruct userdiff_driver *drv;\n+\t\tgrep_attr_lock(opt);\n+\t\tdrv = 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 +872,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 +885,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 +901,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 +979,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@@ -1008,16 +1032,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->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}\n+\topt->priv = &xecfg;\n+\n \ttry_lookahead = should_lookahead(opt);\n \n \twhile (left) {\n@@ -1093,7 +1109,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-- \n1.7.8.431.g2abf2\n"},{"id":"180967","messageId":"faa76319c7863852eb6848614c193b56af5789f9.1323723759.git.trast@student.ethz.ch","threadId":"29142","inReplyTo":"cover.1323723759.git.trast@student.ethz.ch","subject":"[PATCH v3 3/3] grep: disable threading in non-worktree case","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-12T21:16:08Z","receivedAt":"2011-12-12T21:16:08Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Measurements by various people have shown that grepping in parallel is\nnot beneficial when the object store is involved.  For example, with a\nsimple regex:\n\n  Threads     | --cached case            | worktree case\n  ----------------------------------------------------------------\n  8 (default) | 2.88u 0.21s 0:02.94real  | 0.19u 0.32s 0:00.16real\n  4           | 2.89u 0.29s 0:02.99real  | 0.16u 0.34s 0:00.17real\n  2           | 2.83u 0.36s 0:02.87real  | 0.18u 0.32s 0:00.26real\n  NO_PTHREADS | 2.16u 0.08s 0:02.25real  | 0.12u 0.17s 0:00.31real\n\nThis happens because all the threads contend on read_sha1_mutex almost\nall of the time.  A more complex regex allows the threads to do more\nwork in parallel, but as Jeff King found out, the \"super boost\" (much\nhigher clock when only one core is active) feature of recent CPUs\nstill causes the unthreaded case to win by a large margin.\n\nSo until the pack machinery allows unthreaded access, we disable\ngrep's threading in all but the worktree case.\n\nHelped-by: René Scharfe <rene.scharfe@lsrfire.ath.cx>\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n builtin/grep.c |   36 ++++++++++++++++++------------------\n 1 files changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex bc23c3c..7affbda 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -1003,24 +1003,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \tif (!opt.fixed && opt.ignore_case)\n \t\topt.regflags |= REG_ICASE;\n \n-#ifndef NO_PTHREADS\n-\tif (online_cpus() == 1)\n-\t\tuse_threads = 0;\n-#else\n-\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-\t\t\tskip_first_line = 1;\n-\t\tstart_threads(&opt);\n-\t}\n-#endif\n-\n \tcompile_grep_patterns(&opt);\n \n \t/* Check revs and then paths */\n@@ -1042,6 +1024,24 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\tbreak;\n \t}\n \n+#ifndef NO_PTHREADS\n+\tif (list.nr || cached || online_cpus() == 1)\n+\t\tuse_threads = 0;\n+#else\n+\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+\t\t\tskip_first_line = 1;\n+\t\tstart_threads(&opt);\n+\t}\n+#endif\n+\n \t/* The rest are paths */\n \tif (!seen_dashdash) {\n \t\tint j;\n-- \n1.7.8.431.g2abf2\n"},{"id":"180985","messageId":"4EE6820E.7010605@lsrfire.ath.cx","threadId":"29142","inReplyTo":"cover.1323723759.git.trast@student.ethz.ch","subject":"Re: [PATCH v3 0/3] grep attributes and multithreading","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-12-12T22:37:02Z","receivedAt":"2011-12-12T22:37:02Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 12.12.2011 22:16, schrieb Thomas Rast:\n> I think we should finish up these three patches for the next release.\n\n> I already posted a bunch of POC patches, but doing the timing manually\n> has been getting on my nerves lately.  So I would first like to\n> formalize some of the performance testing, perhaps along lines already\n> drawn up by Michael Hagger, perhaps not.  Then we can revisit the\n> issue of grep performance.  But I would prefer not to block the -W fix\n> and two easy and confirmed speedups on that.\n\nYes, that's a good idea.  The three patches are uncontroversial \nincremental improvements.\n\n> I dropped this part entirely:\n>\n>> How about adding a parameter to control the number of threads\n>> (--threads?) instead that defaults to eight (or five) for the worktree\n>> and one for the rest?  That would also make benchmarking easier.\n>\n> It does make testing a lot easier, but the interface is IMHO not fit\n> for users and I have a feeling that the \"right\" for-debugging\n> interface will end up falling out of the performance testing work\n> (probably an environment variable).  The end-user option should be a\n> config setting, if any.\n\nAgreed; users shouldn't need to specify such a parameter -- our \nheuristic should be good enough.\n\nRené\n"},{"id":"180994","messageId":"7vsjkpz763.fsf@alter.siamese.dyndns.org","threadId":"29142","inReplyTo":"cover.1323723759.git.trast@student.ethz.ch","subject":"Re: [PATCH v3 0/3] grep attributes and multithreading","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-12T23:44:52Z","receivedAt":"2011-12-12T23:44:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; sign-off?\n"},{"id":"181027","messageId":"201112130944.17429.trast@student.ethz.ch","threadId":"29142","inReplyTo":"7vsjkpz763.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 0/3] grep attributes and multithreading","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2011-12-13T08:44:17Z","receivedAt":"2011-12-13T08:44:17Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano wrote:\n> Thanks; sign-off?\n\nOoops, not again!\n\n> [PATCH v3 1/3] grep: load funcname patterns for -W\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n\n-- \nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"181284","messageId":"4EEAFFAF.8030003@viscovery.net","threadId":"29142","inReplyTo":"138b930c0c96029f2fb9a816e73e70a4d5bce1de.1323723759.git.trast@student.ethz.ch","subject":"Re: [PATCH v3 2/3] grep: enable threading with -p and -W using lazy attribute lookup","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-12-16T08:22:07Z","receivedAt":"2011-12-16T08:22:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 12/12/2011 22:16, schrieb Thomas Rast:\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\nThis is the first time we use pthread_mutex_t in a header file. We need at\nleast the following squashed in. An alternative would be to include\n\"thread-utils.h\", but thread-utils is really more about implementation\nhelpers functions, not about types, and therefore does not look right to\nbe #included in a header.\n\n---\n grep.h |    1 +\n 1 files changed, 1 insertions(+), 0 deletions(-)\n\ndiff --git a/grep.h b/grep.h\nindex 15d227c..754b270 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -133,6 +133,7 @@ extern struct grep_opt *grep_opt_dup(const struct\ngrep_opt *opt);\n extern int grep_threads_ok(const struct grep_opt *opt);\n\n #ifndef NO_PTHREADS\n+#include <pthread.h>\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-- \n1.7.8.1436.gb3021\n"},{"id":"181306","messageId":"7vvcpgifow.fsf@alter.siamese.dyndns.org","threadId":"29142","inReplyTo":"4EEAFFAF.8030003@viscovery.net","subject":"Re: [PATCH v3 2/3] grep: enable threading with -p and -W using lazy attribute lookup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-12-16T17:34:07Z","receivedAt":"2011-12-16T17:34:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> This is the first time we use pthread_mutex_t in a header file. We need at\n> least the following squashed in. An alternative would be to include\n> \"thread-utils.h\", but thread-utils is really more about implementation\n> helpers functions, not about types,...\n\nbuiltin/grep.c already uses thread-utils.h since 5b594f4 (Threaded grep,\n2010-01-25), so does builtin/pack-objects.c since 833e3df (pack-objects:\nAdd runtime detection of online CPU's, 2008-02-22), so it may be simpler\nto do so in grep.h instead.\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex bc23c3c..6474eed 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -17,7 +17,6 @@\n #include \"grep.h\"\n #include \"quote.h\"\n #include \"dir.h\"\n-#include \"thread-utils.h\"\n \n static char const * const grep_usage[] = {\n \t\"git grep [options] [-e] <pattern> [<rev>...] [[--] <path>...]\",\ndiff --git a/grep.h b/grep.h\nindex 15d227c..dd4c65e 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -133,8 +133,11 @@ 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+#include \"thread-utils.h\"\n+/*\n+ * Mutex used around access to the attributes machinery if\n+ * opt->use_threads.  Must be initialized/destroyed by callers!\n+ */\n extern pthread_mutex_t grep_attr_mutex;\n #endif\n \n"}]}