{"thread":{"id":"40644","subject":"[PATCH v3] Add git-grep threads param","startedAt":"2015-10-26T12:32:13Z","lastAt":"2015-10-27T14:11:00Z","messageCount":7,"participants":["Victor Leschuk","John Keeping"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"272233","messageId":"1445862733-838-1-git-send-email-vleschuk@accesssoftek.com","threadId":"40644","inReplyTo":null,"subject":"[PATCH v3] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-10-26T12:32:13Z","receivedAt":"2015-10-26T12:32:13Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"Make number of git-grep worker threads a configuration parameter.\nAccording to several tests on systems with different number of CPU cores\nthe hard-coded number of 8 threads is not optimal for all systems:\ntuning this parameter can significantly speed up grep performance.\n\nSigned-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n---\n Documentation/config.txt               |  4 ++++\n Documentation/git-grep.txt             |  4 ++++\n builtin/grep.c                         | 34 ++++++++++++++++++++++++++--------\n contrib/completion/git-completion.bash |  1 +\n grep.c                                 | 10 ++++++++++\n grep.h                                 |  2 ++\n 6 files changed, 47 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..1c95587 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.threads::\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..fbd4f83 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   [--threads <num>]\n \t   [-W | --function-context]\n \t   [-f <file>] [-e] <pattern>\n \t   [--and|--or|--not|(|)|-e <pattern>...]\n@@ -220,6 +221,9 @@ OPTIONS\n \tShow <num> leading lines, and place a line containing\n \t`--` between contiguous groups of matches.\n \n+--threads <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..5ef1b07 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 = xcalloc(opt->num_threads, sizeof(pthread_t));\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,8 @@ 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+\t\tOPT_INTEGER(0, \"threads\", &opt.num_threads,\n+\t\t\tN_(\"use <n> worker threads\")),\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@@ -832,8 +836,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t}\n \n #ifndef NO_PTHREADS\n-\tif (list.nr || cached || online_cpus() == 1)\n+\tif (!opt.num_threads) {\n+\t\tuse_threads = 0; /* User explicitely told not to use threads */\n+\t}\n+\telse if (list.nr || cached) {\n+\t\tuse_threads = 0; /* Can not multi-thread object lookup */\n+\t}\n+\telse if (opt.num_threads >= 0) {\n+\t\tuse_threads = 1; /* User explicitely set the number of threads */\n+\t}\n+\telse if (online_cpus() <= 1) {\n \t\tuse_threads = 0;\n+\t}\n+\telse {\n+\t\tuse_threads = 1;\n+\t\topt.num_threads = GREP_NUM_THREADS_DEFAULT;\n+\t}\n #else\n \tuse_threads = 0;\n #endif\n@@ -910,7 +928,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..390d9c0 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\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..b53fb14 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -40,6 +40,7 @@ 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+\topt->num_threads = -1;\n }\n \n static int parse_pattern_type_arg(const char *opt, const char *arg)\n@@ -124,6 +125,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+\tif (!strcmp(var, \"grep.threads\")) {\n+\t\tint threads = git_config_int(var, value);\n+\t\tif (threads < 0)\n+\t\t\tdie(\"invalid number of threads specified (%d)\", threads);\n+\t\topt->num_threads = threads;\n+\t\treturn 0;\n+\t}\n \treturn 0;\n }\n \n@@ -150,6 +159,7 @@ 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+\topt->num_threads = def->num_threads;\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..bb20456 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -132,6 +132,8 @@ struct grep_opt {\n \tunsigned pre_context;\n \tunsigned post_context;\n \tunsigned last_shown;\n+#define GREP_NUM_THREADS_DEFAULT 8\n+\tint num_threads;\n \tint show_hunk_mark;\n \tint file_break;\n \tint heading;\n-- \n2.6.2.281.g222e106.dirty\n"},{"id":"272256","messageId":"20151026193241.GO19802@serenity.lan","threadId":"40644","inReplyTo":"1445862733-838-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH v3] Add git-grep threads param","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-10-26T19:32:41Z","receivedAt":"2015-10-26T19:32:41Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Oct 26, 2015 at 03:32:13PM +0300, Victor Leschuk wrote:\n> Make number of git-grep worker threads a configuration parameter.\n> According to several tests on systems with different number of CPU cores\n> the hard-coded number of 8 threads is not optimal for all systems:\n> tuning this parameter can significantly speed up grep performance.\n> \n> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n> ---\n>  Documentation/config.txt               |  4 ++++\n>  Documentation/git-grep.txt             |  4 ++++\n>  builtin/grep.c                         | 34 ++++++++++++++++++++++++++--------\n>  contrib/completion/git-completion.bash |  1 +\n>  grep.c                                 | 10 ++++++++++\n>  grep.h                                 |  2 ++\n>  6 files changed, 47 insertions(+), 8 deletions(-)\n> \n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 391a0c3..1c95587 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.threads::\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..fbd4f83 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   [--threads <num>]\n\nIs this the best place for this option?  I know the current list isn't\nsorted in any particular way, but here you're splitting up the set of\ncontext options (`-A`, `-B`, `-C` and `-W`).\n\n>  \t   [-W | --function-context]\n>  \t   [-f <file>] [-e] <pattern>\n>  \t   [--and|--or|--not|(|)|-e <pattern>...]\n> @@ -220,6 +221,9 @@ OPTIONS\n>  \tShow <num> leading lines, and place a line containing\n>  \t`--` between contiguous groups of matches.\n>  \n> +--threads <num>::\n> +\tSet number of worker threads to <num>. Default is 8.\n\nThe same comment as above applies here.\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..5ef1b07 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 = xcalloc(opt->num_threads, sizeof(pthread_t));\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\nI'm not sure passing a grep_opt in here is the cleanest way to do this.\nOptions are a UI concept and all we care about here is the number of\nthreads.\n\nSince `threads` is a global, shouldn't the number of threads be a global\nas well?  Could we reuse `use_threads` here (possibly renaming it\n`num_threads`)?\n\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,8 @@ 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> +\t\tOPT_INTEGER(0, \"threads\", &opt.num_threads,\n> +\t\t\tN_(\"use <n> worker threads\")),\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> @@ -832,8 +836,22 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n>  \t}\n>  \n>  #ifndef NO_PTHREADS\n> -\tif (list.nr || cached || online_cpus() == 1)\n> +\tif (!opt.num_threads) {\n> +\t\tuse_threads = 0; /* User explicitely told not to use threads */\n> +\t}\n> +\telse if (list.nr || cached) {\n> +\t\tuse_threads = 0; /* Can not multi-thread object lookup */\n> +\t}\n> +\telse if (opt.num_threads >= 0) {\n> +\t\tuse_threads = 1; /* User explicitely set the number of threads */\n> +\t}\n> +\telse if (online_cpus() <= 1) {\n>  \t\tuse_threads = 0;\n> +\t}\n> +\telse {\n> +\t\tuse_threads = 1;\n> +\t\topt.num_threads = GREP_NUM_THREADS_DEFAULT;\n> +\t}\n>  #else\n>  \tuse_threads = 0;\n>  #endif\n> @@ -910,7 +928,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..390d9c0 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\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..b53fb14 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -40,6 +40,7 @@ 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> +\topt->num_threads = -1;\n>  }\n>  \n>  static int parse_pattern_type_arg(const char *opt, const char *arg)\n> @@ -124,6 +125,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> +\tif (!strcmp(var, \"grep.threads\")) {\n> +\t\tint threads = git_config_int(var, value);\n> +\t\tif (threads < 0)\n> +\t\t\tdie(\"invalid number of threads specified (%d)\", threads);\n> +\t\topt->num_threads = threads;\n> +\t\treturn 0;\n> +\t}\n>  \treturn 0;\n>  }\n>  \n> @@ -150,6 +159,7 @@ 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> +\topt->num_threads = def->num_threads;\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..bb20456 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -132,6 +132,8 @@ struct grep_opt {\n>  \tunsigned pre_context;\n>  \tunsigned post_context;\n>  \tunsigned last_shown;\n> +#define GREP_NUM_THREADS_DEFAULT 8\n> +\tint num_threads;\n>  \tint show_hunk_mark;\n>  \tint file_break;\n>  \tint heading;\n> -- \n> 2.6.2.281.g222e106.dirty\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"272290","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9FC@mail.accesssoftek.com","threadId":"40644","inReplyTo":"20151026193241.GO19802@serenity.lan","subject":"RE: [PATCH v3] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-10-27T05:25:41Z","receivedAt":"2015-10-27T05:25:41Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello John,\n\nsee comments inline.\n\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>> +        [--threads <num>]\n\n> Is this the best place for this option?  I know the current list isn't\n> sorted in any particular way, but here you're splitting up the set of\n> context options (`-A`, `-B`, `-C` and `-W`).\n\nAgree, I'll move the option both here and in documentation.\n\n>> -static int wait_all(void)\n>> +static int wait_all(struct grep_opt *opt)\n\n> I'm not sure passing a grep_opt in here is the cleanest way to do this.\n> Options are a UI concept and all we care about here is the number of\n> threads.\n\n> Since `threads` is a global, shouldn't the number of threads be a global\n> as well?  Could we reuse `use_threads` here (possibly renaming it\n> `num_threads`)?\n\nThis thought also crossed my mind, however we already pass grep_opt to start_threads() function,\nso I think passing it to wait_all() is not that ugly, and kind of symmetric. And I do not like the idea\nof duplicating same information in different places. What do you think?\n\n--\nBest Regards,\nVictor\n"},{"id":"272293","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9FE@mail.accesssoftek.com","threadId":"40644","inReplyTo":"CA+55aFwrU25x25XrRODgS1oRXqN60rmYPiXLgfs3mqRco4Oi9A@mail.gmail.com","subject":"RE: [PATCH v3] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-10-27T09:14:25Z","receivedAt":"2015-10-27T09:14:25Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello Linus, \n\n>> According to several tests on systems with different number of CPU cores\n>> the hard-coded number of 8 threads is not optimal for all systems:\n\n> Did you also compare cold-cache filesystem performance?\n\n>  One of the reasons for doing threaded grep is for CPU scaling. But another is for IO scaling. If your git tree is over NFS, doing grep eight threads at a time if likely going to make things much faster even if you are on a single CPU.\n\nYes, I have performed tests on cold-cache FS and it looks like number of threads affects performance. Here are the results for grepping linux kernel repo on a 4-core machine (similar test was conducted on 8-core machine):\n\nThreads: 4 Time: 39.13\nThreads: 8 Time: 34.39\nThreads: 16 Time: 31.46\nThreads: 32 Time: 27.40\n\nHere is test scenario:\n\n#!/bin/bash\nTIMEFORMAT=%R\nGIT=/home/del/git-dev/bin/git\nTESTS=10\nfor n in 4 8 16 32; do\n    echo -n \"Threads: $n Time: \"\n    for i in $(seq 1 $TESTS); do\n        echo 3 > /proc/sys/vm/drop_caches\n        time $GIT grep --threads $n -e '#define' --and \\( -e MAX_PATH -e PATH_MAX \\)  >/dev/null\n    done 2>&1 | awk -v ntests=${TESTS} '{sum+=$1} END{printf \"%.2f\\n\", sum/ntests}'\ndone\n\nNote: With hot-cache grepping with 4 threads gives fastest results on both 4-core and 8-core machines.\n\nThus I think it can be useful for users to be able to tune the threads number according to their needs.\n"},{"id":"272306","messageId":"20151027115256.GQ19802@serenity.lan","threadId":"40644","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9FC@mail.accesssoftek.com","subject":"Re: [PATCH v3] Add git-grep threads param","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-10-27T11:52:56Z","receivedAt":"2015-10-27T11:52:56Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Mon, Oct 26, 2015 at 10:25:41PM -0700, Victor Leschuk wrote:\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> >> +        [--threads <num>]\n> \n> > Is this the best place for this option?  I know the current list isn't\n> > sorted in any particular way, but here you're splitting up the set of\n> > context options (`-A`, `-B`, `-C` and `-W`).\n> \n> Agree, I'll move the option both here and in documentation.\n> \n> >> -static int wait_all(void)\n> >> +static int wait_all(struct grep_opt *opt)\n> \n> > I'm not sure passing a grep_opt in here is the cleanest way to do this.\n> > Options are a UI concept and all we care about here is the number of\n> > threads.\n> \n> > Since `threads` is a global, shouldn't the number of threads be a global\n> > as well?  Could we reuse `use_threads` here (possibly renaming it\n> > `num_threads`)?\n> \n> This thought also crossed my mind, however we already pass grep_opt to\n> start_threads() function, so I think passing it to wait_all() is not\n> that ugly, and kind of symmetric. And I do not like the idea of\n> duplicating same information in different places. What do you think?\n\nThe grep_opt in start_threads() is being passed through to run(), so it\nseems slightly different to me.  If the threads were being setup in\ngrep.c (as opposed to builtin/grep.c) then I'd agree that it belongs in\ngrep_opt, but since this is local to this particular user of the grep\ninfrastructure adding num_threads to the grep_opt structure at all feels\nwrong to me.\n\nNote that I wasn't suggesting passing num_threads as a parameter to\nwait_all(), but rather having it as global state that is accessed by\nwait_all() in the same way as the `threads` array.\n\nIf we rename use_threads to num_threads and just use that, then we only\nhave the information in one place don't we?\n"},{"id":"272308","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9FF@mail.accesssoftek.com","threadId":"40644","inReplyTo":"20151027115256.GQ19802@serenity.lan","subject":"RE: [PATCH v3] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-10-27T13:54:16Z","receivedAt":"2015-10-27T13:54:16Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello John,\n\n>> This thought also crossed my mind, however we already pass grep_opt to\n>> start_threads() function, so I think passing it to wait_all() is not\n>> that ugly, and kind of symmetric. And I do not like the idea of\n>> duplicating same information in different places. What do you think?\n\n> The grep_opt in start_threads() is being passed through to run(), so it\n> seems slightly different to me.  If the threads were being setup in\n> grep.c (as opposed to builtin/grep.c) then I'd agree that it belongs in\n> grep_opt, but since this is local to this particular user of the grep\n> infrastructure adding num_threads to the grep_opt structure at all feels\n> wrong to me.\n\n> Note that I wasn't suggesting passing num_threads as a parameter to\n> wait_all(), but rather having it as global state that is accessed by\n> wait_all() in the same way as the `threads` array.\n\n> If we rename use_threads to num_threads and just use that, then we only\n> have the information in one place don't we?\n\nYeah, I understood your idea. So we parse config_value directly to \n\nstatic int num_threads; /* old use_threads */\n\nAnd use it internally in builtin/grep.c. I think you are right.\n\nLooks like grep_cmd_config() is the right place to parse it. Something like:\n\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -267,6 +267,8 @@ static int wait_all(struct grep_opt *opt)\n static int grep_cmd_config(const char *var, const char *value, void *cb)\n {\n        int st = grep_config(var, value, cb);\n+       if (thread_config(var, value, cb) < 0)\n+               st = -1;\n        if (git_color_default_config(var, value, cb) < 0)\n                st = -1;\n        return st;\n\nWhat do you think?\n\n--\nBest Regards,\nVictor\n"},{"id":"272309","messageId":"20151027141100.GR19802@serenity.lan","threadId":"40644","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDAB9FF@mail.accesssoftek.com","subject":"Re: [PATCH v3] Add git-grep threads param","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-10-27T14:11:00Z","receivedAt":"2015-10-27T14:11:00Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Oct 27, 2015 at 06:54:16AM -0700, Victor Leschuk wrote:\n> Hello John,\n> \n> >> This thought also crossed my mind, however we already pass grep_opt to\n> >> start_threads() function, so I think passing it to wait_all() is not\n> >> that ugly, and kind of symmetric. And I do not like the idea of\n> >> duplicating same information in different places. What do you think?\n> \n> > The grep_opt in start_threads() is being passed through to run(), so it\n> > seems slightly different to me.  If the threads were being setup in\n> > grep.c (as opposed to builtin/grep.c) then I'd agree that it belongs in\n> > grep_opt, but since this is local to this particular user of the grep\n> > infrastructure adding num_threads to the grep_opt structure at all feels\n> > wrong to me.\n> \n> > Note that I wasn't suggesting passing num_threads as a parameter to\n> > wait_all(), but rather having it as global state that is accessed by\n> > wait_all() in the same way as the `threads` array.\n> \n> > If we rename use_threads to num_threads and just use that, then we only\n> > have the information in one place don't we?\n> \n> Yeah, I understood your idea. So we parse config_value directly to \n> \n> static int num_threads; /* old use_threads */\n\nPresumably this is:\n\n\tstatic int num_threads = -1;\n\nso that the default behaviour continues to work correctly.\n\n> And use it internally in builtin/grep.c. I think you are right.\n> \n> Looks like grep_cmd_config() is the right place to parse it. Something like:\n> \n> --- a/builtin/grep.c\n> +++ b/builtin/grep.c\n> @@ -267,6 +267,8 @@ static int wait_all(struct grep_opt *opt)\n>  static int grep_cmd_config(const char *var, const char *value, void *cb)\n>  {\n>         int st = grep_config(var, value, cb);\n> +       if (thread_config(var, value, cb) < 0)\n> +               st = -1;\n>         if (git_color_default_config(var, value, cb) < 0)\n>                 st = -1;\n>         return st;\n> \n> What do you think?\n\nI'd be tempted to open code the \"grep.threads\" case in this function\nrather than introducing a helper for a single variable, but I don't\nthink it matters either way.  This looks good.\n"}]}