{"thread":{"id":"40619","subject":"[PATCH] Add git-grep threads-num param","startedAt":"2015-10-22T13:23:56Z","lastAt":"2015-10-22T20:40:34Z","messageCount":4,"participants":["Victor Leschuk","John Keeping","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"272099","messageId":"1445520236-10753-1-git-send-email-vleschuk@accesssoftek.com","threadId":"40619","inReplyTo":null,"subject":"[PATCH] Add git-grep threads-num param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-10-22T13:23:56Z","receivedAt":"2015-10-22T13:23:56Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"Hello all, I suggest we make number of git-grep worker threads a configuration\nparameter. I have run several tests on systems with different number of CPU cores.\nIt appeared that the hard-coded number 8 lowers performance on both of my systems:\non my 4-core and 8-core systems the thread number of 4 worked about 20% faster than\ndefault 8. So I think it is better to allow users tune this parameter.\n\nSigned-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n---\n Documentation/config.txt               |  4 ++++\n Documentation/git-grep.txt             |  5 +++++\n builtin/grep.c                         | 20 +++++++++++++-------\n contrib/completion/git-completion.bash |  1 +\n grep.c                                 | 15 +++++++++++++++\n grep.h                                 |  4 ++++\n 6 files changed, 42 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..c3df20c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1447,6 +1447,10 @@ grep.extendedRegexp::\n \toption is ignored when the 'grep.patternType' option is set to a value\n \tother than 'default'.\n \n+grep.threadsNum::\n+\tNumber of grep worker threads, use it to tune up performance on\n+\tmulticore machines. Default value is 8.\n+\n gpg.program::\n \tUse this custom program instead of \"gpg\" found on $PATH when\n \tmaking or verifying a PGP signature. The program must support the\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 4a44d6d..e9ca265 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -22,6 +22,7 @@ SYNOPSIS\n \t   [--color[=<when>] | --no-color]\n \t   [--break] [--heading] [-p | --show-function]\n \t   [-A <post-context>] [-B <pre-context>] [-C <context>]\n+\t   [-t <threads-num>]\n \t   [-W | --function-context]\n \t   [-f <file>] [-e] <pattern>\n \t   [--and|--or|--not|(|)|-e <pattern>...]\n@@ -220,6 +221,10 @@ OPTIONS\n \tShow <num> leading lines, and place a line containing\n \t`--` between contiguous groups of matches.\n \n+-t <num>::\n+--threads-num <num>::\n+\tSet number of worker threads to <num>. Default is 8.\n+\n -W::\n --function-context::\n \tShow the surrounding text from the previous line containing a\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d04f440..9b4fc47 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -27,8 +27,7 @@ static char const * const grep_usage[] = {\n static int use_threads = 1;\n \n #ifndef NO_PTHREADS\n-#define THREADS 8\n-static pthread_t threads[THREADS];\n+static pthread_t *threads;\n \n /* We use one producer thread and THREADS consumer\n  * threads. The producer adds struct work_items to 'todo' and the\n@@ -206,7 +205,8 @@ static void start_threads(struct grep_opt *opt)\n \t\tstrbuf_init(&todo[i].out, 0);\n \t}\n \n-\tfor (i = 0; i < ARRAY_SIZE(threads); i++) {\n+\tthreads = xmalloc(sizeof(pthread_t) * opt->num_threads);\n+\tfor (i = 0; i < opt->num_threads; i++) {\n \t\tint err;\n \t\tstruct grep_opt *o = grep_opt_dup(opt);\n \t\to->output = strbuf_out;\n@@ -220,7 +220,7 @@ static void start_threads(struct grep_opt *opt)\n \t}\n }\n \n-static int wait_all(void)\n+static int wait_all(struct grep_opt *opt)\n {\n \tint hit = 0;\n \tint i;\n@@ -238,12 +238,14 @@ static int wait_all(void)\n \tpthread_cond_broadcast(&cond_add);\n \tgrep_unlock();\n \n-\tfor (i = 0; i < ARRAY_SIZE(threads); i++) {\n+\tfor (i = 0; i < opt->num_threads; i++) {\n \t\tvoid *h;\n \t\tpthread_join(threads[i], &h);\n \t\thit |= (int) (intptr_t) h;\n \t}\n \n+\tfree(threads);\n+\n \tpthread_mutex_destroy(&grep_mutex);\n \tpthread_mutex_destroy(&grep_read_mutex);\n \tpthread_mutex_destroy(&grep_attr_mutex);\n@@ -256,7 +258,7 @@ static int wait_all(void)\n }\n #else /* !NO_PTHREADS */\n \n-static int wait_all(void)\n+static int wait_all(struct grep_opt *opt)\n {\n \treturn 0;\n }\n@@ -702,6 +704,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"show <n> context lines before matches\")),\n \t\tOPT_INTEGER('A', \"after-context\", &opt.post_context,\n \t\t\tN_(\"show <n> context lines after matches\")),\n+#ifndef NO_PTHREADS\n+\t\tOPT_INTEGER('t', \"threads-num\", &opt.num_threads,\n+\t\t\tN_(\"use <n> worker threads\")),\n+#endif /* !NO_PTHREADS */\n \t\tOPT_NUMBER_CALLBACK(&opt, N_(\"shortcut for -C NUM\"),\n \t\t\tcontext_callback),\n \t\tOPT_BOOL('p', \"show-function\", &opt.funcname,\n@@ -910,7 +916,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (use_threads)\n-\t\thit |= wait_all();\n+\t\thit |= wait_all(&opt);\n \tif (hit && show_in_pager)\n \t\trun_pager(&opt, prefix);\n \tfree_grep_patterns(&opt);\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 482ca84..6231595 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1310,6 +1310,7 @@ _git_grep ()\n \t\t\t--full-name --line-number\n \t\t\t--extended-regexp --basic-regexp --fixed-strings\n \t\t\t--perl-regexp\n+\t\t\t--threads-num\n \t\t\t--files-with-matches --name-only\n \t\t\t--files-without-match\n \t\t\t--max-depth\ndiff --git a/grep.c b/grep.c\nindex 7b2b96a..17e6a7c 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -40,6 +40,9 @@ void init_grep_defaults(void)\n \tcolor_set(opt->color_selected, \"\");\n \tcolor_set(opt->color_sep, GIT_COLOR_CYAN);\n \topt->color = -1;\n+#ifndef NO_PTHREADS\n+\topt->num_threads = GREP_NUM_THREADS_DEFAULT;\n+#endif /* !NO_PTHREADS */\n }\n \n static int parse_pattern_type_arg(const char *opt, const char *arg)\n@@ -124,6 +127,14 @@ int grep_config(const char *var, const char *value, void *cb)\n \t\t\treturn config_error_nonbool(var);\n \t\treturn color_parse(value, color);\n \t}\n+\n+#ifndef NO_PTHREADS\n+\tif (!strcmp(var, \"grep.threadsnum\")) {\n+\t\tint threads = git_config_int(var, value);\n+\t\topt->num_threads = (threads >= 0) ? threads : GREP_NUM_THREADS_DEFAULT;\n+\t\treturn 0;\n+\t}\n+#endif /* !NO_PTHREADS */\n \treturn 0;\n }\n \n@@ -150,6 +161,10 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n \topt->pathname = def->pathname;\n \topt->regflags = def->regflags;\n \topt->relative = def->relative;\n+#ifndef NO_PTHREADS\n+\tif(!opt->num_threads)\n+\t\topt->num_threads = def->num_threads;\n+#endif /* !NO_PTHREADS */\n \n \tcolor_set(opt->color_context, def->color_context);\n \tcolor_set(opt->color_filename, def->color_filename);\ndiff --git a/grep.h b/grep.h\nindex 95f197a..e4a296b 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -132,6 +132,10 @@ struct grep_opt {\n \tunsigned pre_context;\n \tunsigned post_context;\n \tunsigned last_shown;\n+#ifndef NO_PTHREADS\n+#define GREP_NUM_THREADS_DEFAULT 8\n+\tunsigned num_threads;\n+#endif /* !NO_PTHREADS */\n \tint show_hunk_mark;\n \tint file_break;\n \tint heading;\n-- \n2.6.2.281.gd4b1c9f\n"},{"id":"272101","messageId":"20151022142302.GL19802@serenity.lan","threadId":"40619","inReplyTo":"1445520236-10753-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH] Add git-grep threads-num param","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-10-22T14:23:02Z","receivedAt":"2015-10-22T14:23:02Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Oct 22, 2015 at 04:23:56PM +0300, Victor Leschuk wrote:\n> Hello all, I suggest we make number of git-grep worker threads a configuration\n> parameter. I have run several tests on systems with different number of CPU cores.\n> It appeared that the hard-coded number 8 lowers performance on both of my systems:\n> on my 4-core and 8-core systems the thread number of 4 worked about 20% faster than\n> default 8. So I think it is better to allow users tune this parameter.\n\nFor git-pack-objects we call the command-line parameter \"--threads\" and\nthe config variable \"pack.threads\".  Is there a reason not to use the\nsame name here (i.e. \"--threads\" and \"grep.threads\")?\n\n> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n> ---\n>  Documentation/config.txt               |  4 ++++\n>  Documentation/git-grep.txt             |  5 +++++\n>  builtin/grep.c                         | 20 +++++++++++++-------\n>  contrib/completion/git-completion.bash |  1 +\n>  grep.c                                 | 15 +++++++++++++++\n>  grep.h                                 |  4 ++++\n>  6 files changed, 42 insertions(+), 7 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 391a0c3..c3df20c 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1447,6 +1447,10 @@ grep.extendedRegexp::\n>  \toption is ignored when the 'grep.patternType' option is set to a value\n>  \tother than 'default'.\n>  \n> +grep.threadsNum::\n> +\tNumber of grep worker threads, use it to tune up performance on\n> +\tmulticore machines. Default value is 8.\n> +\n>  gpg.program::\n>  \tUse this custom program instead of \"gpg\" found on $PATH when\n>  \tmaking or verifying a PGP signature. The program must support the\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index 4a44d6d..e9ca265 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -22,6 +22,7 @@ SYNOPSIS\n>  \t   [--color[=<when>] | --no-color]\n>  \t   [--break] [--heading] [-p | --show-function]\n>  \t   [-A <post-context>] [-B <pre-context>] [-C <context>]\n> +\t   [-t <threads-num>]\n>  \t   [-W | --function-context]\n>  \t   [-f <file>] [-e] <pattern>\n>  \t   [--and|--or|--not|(|)|-e <pattern>...]\n> @@ -220,6 +221,10 @@ OPTIONS\n>  \tShow <num> leading lines, and place a line containing\n>  \t`--` between contiguous groups of matches.\n>  \n> +-t <num>::\n> +--threads-num <num>::\n> +\tSet number of worker threads to <num>. Default is 8.\n> +\n>  -W::\n>  --function-context::\n>  \tShow the surrounding text from the previous line containing a\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index d04f440..9b4fc47 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -27,8 +27,7 @@ static char const * const grep_usage[] = {\n>  static int use_threads = 1;\n>  \n>  #ifndef NO_PTHREADS\n> -#define THREADS 8\n> -static pthread_t threads[THREADS];\n> +static pthread_t *threads;\n>  \n>  /* We use one producer thread and THREADS consumer\n>   * threads. The producer adds struct work_items to 'todo' and the\n> @@ -206,7 +205,8 @@ static void start_threads(struct grep_opt *opt)\n>  \t\tstrbuf_init(&todo[i].out, 0);\n>  \t}\n>  \n> -\tfor (i = 0; i < ARRAY_SIZE(threads); i++) {\n> +\tthreads = xmalloc(sizeof(pthread_t) * opt->num_threads);\n> +\tfor (i = 0; i < opt->num_threads; i++) {\n>  \t\tint err;\n>  \t\tstruct grep_opt *o = grep_opt_dup(opt);\n>  \t\to->output = strbuf_out;\n> @@ -220,7 +220,7 @@ static void start_threads(struct grep_opt *opt)\n>  \t}\n>  }\n>  \n> -static int wait_all(void)\n> +static int wait_all(struct grep_opt *opt)\n>  {\n>  \tint hit = 0;\n>  \tint i;\n> @@ -238,12 +238,14 @@ static int wait_all(void)\n>  \tpthread_cond_broadcast(&cond_add);\n>  \tgrep_unlock();\n>  \n> -\tfor (i = 0; i < ARRAY_SIZE(threads); i++) {\n> +\tfor (i = 0; i < opt->num_threads; i++) {\n>  \t\tvoid *h;\n>  \t\tpthread_join(threads[i], &h);\n>  \t\thit |= (int) (intptr_t) h;\n>  \t}\n>  \n> +\tfree(threads);\n> +\n>  \tpthread_mutex_destroy(&grep_mutex);\n>  \tpthread_mutex_destroy(&grep_read_mutex);\n>  \tpthread_mutex_destroy(&grep_attr_mutex);\n> @@ -256,7 +258,7 @@ static int wait_all(void)\n>  }\n>  #else /* !NO_PTHREADS */\n>  \n> -static int wait_all(void)\n> +static int wait_all(struct grep_opt *opt)\n>  {\n>  \treturn 0;\n>  }\n> @@ -702,6 +704,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t\t\tN_(\"show <n> context lines before matches\")),\n>  \t\tOPT_INTEGER('A', \"after-context\", &opt.post_context,\n>  \t\t\tN_(\"show <n> context lines after matches\")),\n> +#ifndef NO_PTHREADS\n> +\t\tOPT_INTEGER('t', \"threads-num\", &opt.num_threads,\n> +\t\t\tN_(\"use <n> worker threads\")),\n> +#endif /* !NO_PTHREADS */\n>  \t\tOPT_NUMBER_CALLBACK(&opt, N_(\"shortcut for -C NUM\"),\n>  \t\t\tcontext_callback),\n>  \t\tOPT_BOOL('p', \"show-function\", &opt.funcname,\n> @@ -910,7 +916,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  \tif (use_threads)\n> -\t\thit |= wait_all();\n> +\t\thit |= wait_all(&opt);\n>  \tif (hit && show_in_pager)\n>  \t\trun_pager(&opt, prefix);\n>  \tfree_grep_patterns(&opt);\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 482ca84..6231595 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1310,6 +1310,7 @@ _git_grep ()\n>  \t\t\t--full-name --line-number\n>  \t\t\t--extended-regexp --basic-regexp --fixed-strings\n>  \t\t\t--perl-regexp\n> +\t\t\t--threads-num\n>  \t\t\t--files-with-matches --name-only\n>  \t\t\t--files-without-match\n>  \t\t\t--max-depth\n> diff --git a/grep.c b/grep.c\n> index 7b2b96a..17e6a7c 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -40,6 +40,9 @@ void init_grep_defaults(void)\n>  \tcolor_set(opt->color_selected, \"\");\n>  \tcolor_set(opt->color_sep, GIT_COLOR_CYAN);\n>  \topt->color = -1;\n> +#ifndef NO_PTHREADS\n> +\topt->num_threads = GREP_NUM_THREADS_DEFAULT;\n> +#endif /* !NO_PTHREADS */\n>  }\n>  \n>  static int parse_pattern_type_arg(const char *opt, const char *arg)\n> @@ -124,6 +127,14 @@ int grep_config(const char *var, const char *value, void *cb)\n>  \t\t\treturn config_error_nonbool(var);\n>  \t\treturn color_parse(value, color);\n>  \t}\n> +\n> +#ifndef NO_PTHREADS\n> +\tif (!strcmp(var, \"grep.threadsnum\")) {\n> +\t\tint threads = git_config_int(var, value);\n> +\t\topt->num_threads = (threads >= 0) ? threads : GREP_NUM_THREADS_DEFAULT;\n> +\t\treturn 0;\n> +\t}\n> +#endif /* !NO_PTHREADS */\n>  \treturn 0;\n>  }\n>  \n> @@ -150,6 +161,10 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n>  \topt->pathname = def->pathname;\n>  \topt->regflags = def->regflags;\n>  \topt->relative = def->relative;\n> +#ifndef NO_PTHREADS\n> +\tif(!opt->num_threads)\n> +\t\topt->num_threads = def->num_threads;\n> +#endif /* !NO_PTHREADS */\n>  \n>  \tcolor_set(opt->color_context, def->color_context);\n>  \tcolor_set(opt->color_filename, def->color_filename);\n> diff --git a/grep.h b/grep.h\n> index 95f197a..e4a296b 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -132,6 +132,10 @@ struct grep_opt {\n>  \tunsigned pre_context;\n>  \tunsigned post_context;\n>  \tunsigned last_shown;\n> +#ifndef NO_PTHREADS\n> +#define GREP_NUM_THREADS_DEFAULT 8\n> +\tunsigned num_threads;\n> +#endif /* !NO_PTHREADS */\n>  \tint show_hunk_mark;\n>  \tint file_break;\n>  \tint heading;\n> -- \n> 2.6.2.281.gd4b1c9f\n"},{"id":"272121","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9F7@mail.accesssoftek.com","threadId":"40619","inReplyTo":"20151022142302.GL19802@serenity.lan","subject":"RE: [PATCH] Add git-grep threads-num param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-10-22T20:39:59Z","receivedAt":"2015-10-22T20:39:59Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello John,\n\nI haven't noticed the --threads option in pack-objects, I will fix the patch to make naming more uniform. Do you have any comments regarding the functionality?\n\n--\nBest Regards,\nVictor\n________________________________________\nFrom: John Keeping [john@keeping.me.uk]\nSent: Thursday, October 22, 2015 7:23 AM\nTo: Victor Leschuk\nCc: git@vger.kernel.org; Victor Leschuk\nSubject: Re: [PATCH] Add git-grep threads-num param\n\nOn Thu, Oct 22, 2015 at 04:23:56PM +0300, Victor Leschuk wrote:\n> Hello all, I suggest we make number of git-grep worker threads a configuration\n> parameter. I have run several tests on systems with different number of CPU cores.\n> It appeared that the hard-coded number 8 lowers performance on both of my systems:\n> on my 4-core and 8-core systems the thread number of 4 worked about 20% faster than\n> default 8. So I think it is better to allow users tune this parameter.\n\nFor git-pack-objects we call the command-line parameter \"--threads\" and\nthe config variable \"pack.threads\".  Is there a reason not to use the\nsame name here (i.e. \"--threads\" and \"grep.threads\")?\n\n> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n> ---\n>  Documentation/config.txt               |  4 ++++\n>  Documentation/git-grep.txt             |  5 +++++\n>  builtin/grep.c                         | 20 +++++++++++++-------\n>  contrib/completion/git-completion.bash |  1 +\n>  grep.c                                 | 15 +++++++++++++++\n>  grep.h                                 |  4 ++++\n>  6 files changed, 42 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 391a0c3..c3df20c 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1447,6 +1447,10 @@ grep.extendedRegexp::\n>       option is ignored when the 'grep.patternType' option is set to a value\n>       other than 'default'.\n>\n> +grep.threadsNum::\n> +     Number of grep worker threads, use it to tune up performance on\n> +     multicore machines. Default value is 8.\n> +\n>  gpg.program::\n>       Use this custom program instead of \"gpg\" found on $PATH when\n>       making or verifying a PGP signature. The program must support the\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index 4a44d6d..e9ca265 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> @@ -22,6 +22,7 @@ SYNOPSIS\n>          [--color[=<when>] | --no-color]\n>          [--break] [--heading] [-p | --show-function]\n>          [-A <post-context>] [-B <pre-context>] [-C <context>]\n> +        [-t <threads-num>]\n>          [-W | --function-context]\n>          [-f <file>] [-e] <pattern>\n>          [--and|--or|--not|(|)|-e <pattern>...]\n> @@ -220,6 +221,10 @@ OPTIONS\n>       Show <num> leading lines, and place a line containing\n>       `--` between contiguous groups of matches.\n>\n> +-t <num>::\n> +--threads-num <num>::\n> +     Set number of worker threads to <num>. Default is 8.\n> +\n>  -W::\n>  --function-context::\n>       Show the surrounding text from the previous line containing a\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index d04f440..9b4fc47 100644\n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -27,8 +27,7 @@ static char const * const grep_usage[] = {\n>  static int use_threads = 1;\n>\n>  #ifndef NO_PTHREADS\n> -#define THREADS 8\n> -static pthread_t threads[THREADS];\n> +static pthread_t *threads;\n>\n>  /* We use one producer thread and THREADS consumer\n>   * threads. The producer adds struct work_items to 'todo' and the\n> @@ -206,7 +205,8 @@ static void start_threads(struct grep_opt *opt)\n>               strbuf_init(&todo[i].out, 0);\n>       }\n>\n> -     for (i = 0; i < ARRAY_SIZE(threads); i++) {\n> +     threads = xmalloc(sizeof(pthread_t) * opt->num_threads);\n> +     for (i = 0; i < opt->num_threads; i++) {\n>               int err;\n>               struct grep_opt *o = grep_opt_dup(opt);\n>               o->output = strbuf_out;\n> @@ -220,7 +220,7 @@ static void start_threads(struct grep_opt *opt)\n>       }\n>  }\n>\n> -static int wait_all(void)\n> +static int wait_all(struct grep_opt *opt)\n>  {\n>       int hit = 0;\n>       int i;\n> @@ -238,12 +238,14 @@ static int wait_all(void)\n>       pthread_cond_broadcast(&cond_add);\n>       grep_unlock();\n>\n> -     for (i = 0; i < ARRAY_SIZE(threads); i++) {\n> +     for (i = 0; i < opt->num_threads; i++) {\n>               void *h;\n>               pthread_join(threads[i], &h);\n>               hit |= (int) (intptr_t) h;\n>       }\n>\n> +     free(threads);\n> +\n>       pthread_mutex_destroy(&grep_mutex);\n>       pthread_mutex_destroy(&grep_read_mutex);\n>       pthread_mutex_destroy(&grep_attr_mutex);\n> @@ -256,7 +258,7 @@ static int wait_all(void)\n>  }\n>  #else /* !NO_PTHREADS */\n>\n> -static int wait_all(void)\n> +static int wait_all(struct grep_opt *opt)\n>  {\n>       return 0;\n>  }\n> @@ -702,6 +704,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>                       N_(\"show <n> context lines before matches\")),\n>               OPT_INTEGER('A', \"after-context\", &opt.post_context,\n>                       N_(\"show <n> context lines after matches\")),\n> +#ifndef NO_PTHREADS\n> +             OPT_INTEGER('t', \"threads-num\", &opt.num_threads,\n> +                     N_(\"use <n> worker threads\")),\n> +#endif /* !NO_PTHREADS */\n>               OPT_NUMBER_CALLBACK(&opt, N_(\"shortcut for -C NUM\"),\n>                       context_callback),\n>               OPT_BOOL('p', \"show-function\", &opt.funcname,\n> @@ -910,7 +916,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>       }\n>\n>       if (use_threads)\n> -             hit |= wait_all();\n> +             hit |= wait_all(&opt);\n>       if (hit && show_in_pager)\n>               run_pager(&opt, prefix);\n>       free_grep_patterns(&opt);\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 482ca84..6231595 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -1310,6 +1310,7 @@ _git_grep ()\n>                       --full-name --line-number\n>                       --extended-regexp --basic-regexp --fixed-strings\n>                       --perl-regexp\n> +                     --threads-num\n>                       --files-with-matches --name-only\n>                       --files-without-match\n>                       --max-depth\n> diff --git a/grep.c b/grep.c\n> index 7b2b96a..17e6a7c 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -40,6 +40,9 @@ void init_grep_defaults(void)\n>       color_set(opt->color_selected, \"\");\n>       color_set(opt->color_sep, GIT_COLOR_CYAN);\n>       opt->color = -1;\n> +#ifndef NO_PTHREADS\n> +     opt->num_threads = GREP_NUM_THREADS_DEFAULT;\n> +#endif /* !NO_PTHREADS */\n>  }\n>\n>  static int parse_pattern_type_arg(const char *opt, const char *arg)\n> @@ -124,6 +127,14 @@ int grep_config(const char *var, const char *value, void *cb)\n>                       return config_error_nonbool(var);\n>               return color_parse(value, color);\n>       }\n> +\n> +#ifndef NO_PTHREADS\n> +     if (!strcmp(var, \"grep.threadsnum\")) {\n> +             int threads = git_config_int(var, value);\n> +             opt->num_threads = (threads >= 0) ? threads : GREP_NUM_THREADS_DEFAULT;\n> +             return 0;\n> +     }\n> +#endif /* !NO_PTHREADS */\n>       return 0;\n>  }\n>\n> @@ -150,6 +161,10 @@ void grep_init(struct grep_opt *opt, const char *prefix)\n>       opt->pathname = def->pathname;\n>       opt->regflags = def->regflags;\n>       opt->relative = def->relative;\n> +#ifndef NO_PTHREADS\n> +     if(!opt->num_threads)\n> +             opt->num_threads = def->num_threads;\n> +#endif /* !NO_PTHREADS */\n>\n>       color_set(opt->color_context, def->color_context);\n>       color_set(opt->color_filename, def->color_filename);\n> diff --git a/grep.h b/grep.h\n> index 95f197a..e4a296b 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -132,6 +132,10 @@ struct grep_opt {\n>       unsigned pre_context;\n>       unsigned post_context;\n>       unsigned last_shown;\n> +#ifndef NO_PTHREADS\n> +#define GREP_NUM_THREADS_DEFAULT 8\n> +     unsigned num_threads;\n> +#endif /* !NO_PTHREADS */\n>       int show_hunk_mark;\n>       int file_break;\n>       int heading;\n> --\n> 2.6.2.281.gd4b1c9f\n"},{"id":"272122","messageId":"xmqqbnbqr60t.fsf@gitster.mtv.corp.google.com","threadId":"40619","inReplyTo":"20151022142302.GL19802@serenity.lan","subject":"Re: [PATCH] Add git-grep threads-num param","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-22T20:40:34Z","receivedAt":"2015-10-22T20:40:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> On Thu, Oct 22, 2015 at 04:23:56PM +0300, Victor Leschuk wrote:\n>> Hello all, I suggest we make number of git-grep worker threads a configuration\n>> parameter. I have run several tests on systems with different number of CPU cores.\n>> It appeared that the hard-coded number 8 lowers performance on both of my systems:\n>> on my 4-core and 8-core systems the thread number of 4 worked about 20% faster than\n>> default 8. So I think it is better to allow users tune this parameter.\n>\n> For git-pack-objects we call the command-line parameter \"--threads\" and\n> the config variable \"pack.threads\".  Is there a reason not to use the\n> same name here (i.e. \"--threads\" and \"grep.threads\")?\n\nGood suggestion.\n\n>> +-t <num>::\n>> +--threads-num <num>::\n>> +\tSet number of worker threads to <num>. Default is 8.\n\nIt is not like you would want to specify different degree of\nparallelism for every invocation of \"grep\".  The command line option\nfor this feature is expected to be used extremely infrequently, only\nto defeat a configuration variable that is misconfigured to a wrong\nvalue.  Squatting on a short-and-sweet single-letter option name is\nunwarranted for an option like this.\n\n>> -\tfor (i = 0; i < ARRAY_SIZE(threads); i++) {\n>> +\tthreads = xmalloc(sizeof(pthread_t) * opt->num_threads);\n\nxcalloc(nmemb, size)?\n\n>> @@ -702,6 +704,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>  \t\t\tN_(\"show <n> context lines before matches\")),\n>>  \t\tOPT_INTEGER('A', \"after-context\", &opt.post_context,\n>>  \t\t\tN_(\"show <n> context lines after matches\")),\n>> +#ifndef NO_PTHREADS\n>> +\t\tOPT_INTEGER('t', \"threads-num\", &opt.num_threads,\n>> +\t\t\tN_(\"use <n> worker threads\")),\n>> +#endif /* !NO_PTHREADS */\n>>  \t\tOPT_NUMBER_CALLBACK(&opt, N_(\"shortcut for -C NUM\"),\n>>  \t\t\tcontext_callback),\n>>  \t\tOPT_BOOL('p', \"show-function\", &opt.funcname,\n>> @@ -910,7 +916,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>>  \t}\n>>  \n>>  \tif (use_threads)\n>> -\t\thit |= wait_all();\n>> +\t\thit |= wait_all(&opt);\n\nI do not think anybody checked if opt.num_threads is a sensible\nnumber after parse_options() returned at this point.  There is a\ncode outside the context before this hunk that decides if it makes\nsense to use threads and turns use_threads to zero, which should\nbe the right place to do the missing sanity check, I thinnk.\n\n>> diff --git a/grep.c b/grep.c\n>> index 7b2b96a..17e6a7c 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -40,6 +40,9 @@ void init_grep_defaults(void)\n>>  \tcolor_set(opt->color_selected, \"\");\n>>  \tcolor_set(opt->color_sep, GIT_COLOR_CYAN);\n>>  \topt->color = -1;\n>> +#ifndef NO_PTHREADS\n>> +\topt->num_threads = GREP_NUM_THREADS_DEFAULT;\n>> +#endif /* !NO_PTHREADS */\n>>  }\n\nThese #ifndef/#endif's are unnecessary distraction.  Just set them\nunconditionally, even in NO_PTHREADS build (and of course include\nthe field even in NO_PTHREADS build in grep.h).\n"}]}