{"thread":{"id":"40622","subject":"[PATCH v2] Add git-grep threads-num param","startedAt":"2015-10-23T09:15:17Z","lastAt":"2015-10-23T22:40:53Z","messageCount":2,"participants":["Victor Leschuk","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"272135","messageId":"1445591717-5998-1-git-send-email-vleschuk@accesssoftek.com","threadId":"40622","inReplyTo":null,"subject":"[PATCH v2] Add git-grep threads-num param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-10-23T09:15:17Z","receivedAt":"2015-10-23T09:15:17Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"It's a follow up to \"[PATCH] Add git-grep threads-num param\":\n\nMake number of git-grep worker threads a configuration parameter.\nI 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             |  4 ++++\n builtin/grep.c                         | 20 ++++++++++++--------\n contrib/completion/git-completion.bash |  1 +\n grep.c                                 | 11 +++++++++++\n grep.h                                 |  2 ++\n 6 files changed, 34 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..3950725 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,7 +836,7 @@ 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 (list.nr || cached || online_cpus() == 1 || opt.num_threads <= 1)\n \t\tuse_threads = 0;\n #else\n \tuse_threads = 0;\n@@ -910,7 +914,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..9914fe9 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 = GREP_NUM_THREADS_DEFAULT;\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,8 @@ 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+\tif(!opt->num_threads)\n+\t\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..f05874c 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+\tunsigned num_threads;\n \tint show_hunk_mark;\n \tint file_break;\n \tint heading;\n-- \n2.6.2.281.g1c71ee1.dirty\n"},{"id":"272156","messageId":"xmqqbnbpnr7u.fsf@gitster.mtv.corp.google.com","threadId":"40622","inReplyTo":"1445591717-5998-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH v2] Add git-grep threads-num param","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-10-23T22:40:53Z","receivedAt":"2015-10-23T22:40:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Please pay attention to your title.  It no longer matches what the\npatch does.\n\nIt may also be beneficial to study recent titles and log messages\nfrom other developers (you can find them in \"git log --no-merges\"\nand \"git shortlog --no-merges\") and learn and imitate their format,\nstyle and tone.  We want our log to tell a story in a consistent\nvoice, no matter who the authors of individual commits are.\n\nVictor Leschuk <vleschuk@gmail.com> writes:\n\n> It's a follow up to \"[PATCH] Add git-grep threads-num param\":\n\nDo you think anybody wants to see this line in the output from \"git\nlog\" six months from now?  I doubt it.  The previous one will not be\ncommitted to my tree anyway, so the readers would not know (nor\ncare) what other patch you are talking about.\n\n> @@ -832,7 +836,7 @@ 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 (list.nr || cached || online_cpus() == 1 || opt.num_threads <= 1)\n>  \t\tuse_threads = 0;\n\nThis avoid --threads=0 to take the threading codepath and spawning\nno threads, which would have happend in the previous patch.\n\nBut it makes me wonder if the logic should be more like this:\n    \n - Because the code is not prepared to go multi-thread when\n   searching in the object data (not working tree), we always\n   disable threading if 'list' is not empty or 'cached' is given;\n   otherwise\n\n - If the user explicitly said that she wants N threads, we use that\n   many threads; otherwise\n\n - If there is only one CPU, we do not do multi-thread; otherwise\n\n - We use the default number of threads.\n\nIOW, I'd suggest making opt.num_threads an \"int\" (not \"unsigned\"),\ninitialize it to -1 (unspecified), and then make this part more like\nthis, perhaps?\n\n\tif (!opt.num_threads)\n        \tuse_threads = 0; /* the user tells us not to use threads */\n\telse if (list.nr || cached)\n        \tuse_threads = 0; /* cannot multi-thread object lookup */\n\telse if (opt.num_threads >= 1)\n\t\tuse_threads = 1; /* the user explicitly wants this many */\n\telse if (online_cpus() <= 1)\n        \tuse_threads = 0;\n\telse {\n        \tuse_threads = 1;\n                opt.num_threads = GREP_NUM_THREADS_DEFAULT;\n\t}\n\nSomething like this code structure makes it very clear what needs to\nbe changed when we want to add some sort of auto-scaling (instead of\nassigning the DEFAULT constant, you'd see how many cores you have,\nhow many files you will be grepping in, etc. and come up with a good\nnumber dynamically).\n\n> @@ -150,6 +159,8 @@ 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> +\tif(!opt->num_threads)\n\nYou forgot a required SP between a keyword for a syntactic construct\nand its open parenthesis.\n\nThanks.\n"}]}