{"thread":{"id":"40769","subject":"[PATCH v6] Add git-grep threads param","startedAt":"2015-11-11T11:52:50Z","lastAt":"2015-12-04T20:10:43Z","messageCount":8,"participants":["Victor Leschuk","Jeff King","Eric Sunshine","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":6,"patchTotal":null},"messages":[{"id":"273187","messageId":"1447242770-20753-1-git-send-email-vleschuk@accesssoftek.com","threadId":"40769","inReplyTo":null,"subject":"[PATCH v6] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-11-11T11:52:50Z","receivedAt":"2015-11-11T11:52:50Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"\"git grep\" can now be configured (or told from the command line)\n how many threads to use when searching in the working tree files.\n\n Changes to default behavior: number of threads now doesn't depend\n on online_cpus(), e.g. if specific number is not configured\n GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU. \n\nSigned-off-by: Victor Leschuk \n---\nHistory of changes from the first version ($gmane/280053/:\n\t* Param renamed from threads-num to threads\n\t* Short version of '--threads' cmd key was removed\n\t* Made num_threads 'decision-tree' more obvious \n\t and easy to edit for future use ($gmane/280089)\n\t* Moved option description to more suitable place in documentation ($gmane/280188)\n\t* Hid threads param from 'external' grep.c, made it private for builtin/grep.c ($gmane/280188)\n\t* Improved num_threads 'decision-tree', got rid of dependency on online_cpus ($gmane/280299)\n\t* Improved param documentation ($gmane/280299)\n\n\n Documentation/config.txt | 7 +++++\n Documentation/git-grep.txt | 15 ++++++++++\n builtin/grep.c | 50 +++++++++++++++++++++++-----------\n contrib/completion/git-completion.bash | 1 +\n 4 files changed, 57 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..5084e36 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1447,6 +1447,13 @@ 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+\tyour machines. Leave it unset (or set to 0) for default behavior,\n+\twhich for now is using 8 threads for all systems.\n+\tDefault behavior can be changed in future versions\n+\tto better suit hardware and circumstances.\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..8222a83 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -23,6 +23,7 @@ SYNOPSIS\n \t [--break] [--heading] [-p | --show-function]\n \t [-A ] [-B ] [-C ]\n \t [-W | --function-context]\n+\t [--threads ]\n \t [-f ] [-e] \n \t [--and|--or|--not|(|)|-e ...]\n \t [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | ...]\n@@ -53,6 +54,13 @@ 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+\tyour machines. Leave it unset (or set to 0) for default behavior,\n+\twhich for now is using 8 threads for all systems.\n+\tDefault behavior can be changed in future versions\n+\tto better suit hardware and circumstances.\n+\n grep.fullName::\n \tIf set to true, enable '--full-name' option by default.\n \n@@ -227,6 +235,13 @@ OPTIONS\n \teffectively showing the whole function in which the match was\n \tfound.\n \n+--threads ::\n+\tNumber of grep worker threads, use it to tune up performance on\n+\tyour machines. Leave it unset (or set to 0) for default behavior,\n+\twhich for now is using 8 threads for all systems.\n+\tDefault behavior can be changed in future versions\n+\tto better suit hardware and circumstances.\n+\n -f ::\n \tRead patterns from , one per line.\n \ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d04f440..f0e3dfb 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -24,11 +24,11 @@ static char const * const grep_usage[] = {\n \tNULL\n };\n \n-static int use_threads = 1;\n+#define GREP_NUM_THREADS_DEFAULT 8\n+static int num_threads = 0;\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@@ -63,13 +63,13 @@ static pthread_mutex_t grep_mutex;\n \n static inline void grep_lock(void)\n {\n-\tif (use_threads)\n+\tif (num_threads)\n \t\tpthread_mutex_lock(&grep_mutex);\n }\n \n static inline void grep_unlock(void)\n {\n-\tif (use_threads)\n+\tif (num_threads)\n \t\tpthread_mutex_unlock(&grep_mutex);\n }\n \n@@ -206,7 +206,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(num_threads, sizeof(pthread_t));\n+\tfor (i = 0; i < num_threads; i++) {\n \t\tint err;\n \t\tstruct grep_opt *o = grep_opt_dup(opt);\n \t\to->output = strbuf_out;\n@@ -238,12 +239,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 < 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@@ -262,10 +265,19 @@ static int wait_all(void)\n }\n #endif\n \n+static int grep_threads_config(const char *var, const char *value, void *cb)\n+{\n+\tif (!strcmp(var, \"grep.threads\"))\n+\t\tnum_threads = git_config_int(var, value); /* Sanity check of value will be perfomed later */\n+\treturn 0;\n+}\n+\n static int grep_cmd_config(const char *var, const char *value, void *cb)\n {\n \tint st = grep_config(var, value, cb);\n-\tif (git_color_default_config(var, value, cb) < 0)\n+\tif (grep_threads_config(var, value, cb) < 0)\n+\t\tst = -1;\n+\telse if (git_color_default_config(var, value, cb) < 0)\n \t\tst = -1;\n \treturn st;\n }\n@@ -294,7 +306,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n \t}\n \n #ifndef NO_PTHREADS\n-\tif (use_threads) {\n+\tif (num_threads) {\n \t\tadd_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n@@ -323,7 +335,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tstrbuf_addstr(&buf, filename);\n \n #ifndef NO_PTHREADS\n-\tif (use_threads) {\n+\tif (num_threads) {\n \t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n@@ -702,6 +714,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"show context lines before matches\")),\n \t\tOPT_INTEGER('A', \"after-context\", &opt.post_context,\n \t\t\tN_(\"show context lines after matches\")),\n+\t\tOPT_INTEGER(0, \"threads\", &num_threads,\n+\t\t\tN_(\"use 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@@ -801,7 +815,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\topt.output_priv = &path_list;\n \t\topt.output = append_path;\n \t\tstring_list_append(&path_list, show_in_pager);\n-\t\tuse_threads = 0;\n+\t\tnum_threads = 0;\n \t}\n \n \tif (!opt.pattern_list)\n@@ -832,14 +846,18 @@ 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-\t\tuse_threads = 0;\n+\tif (list.nr || cached)\n+\t\tnum_threads = 0; /* Can not multi-thread object lookup */\n+\telse if (num_threads == 0)\n+\t\tnum_threads = GREP_NUM_THREADS_DEFAULT; /* User didn't specify value, or just wants default behavior */\n+\telse if (num_threads < 0)\n+\t\tdie(\"Invalid number of threads specified (%d)\", num_threads);\n #else\n-\tuse_threads = 0;\n+\tnum_threads = 0;\n #endif\n \n #ifndef NO_PTHREADS\n-\tif (use_threads) {\n+\tif (num_threads) {\n \t\tif (!(opt.name_only || opt.unmatch_name_only || opt.count)\n \t\t && (opt.pre_context || opt.post_context ||\n \t\t\topt.file_break || opt.funcbody))\n@@ -909,7 +927,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t\thit = grep_objects(&opt, &pathspec, &list);\n \t}\n \n-\tif (use_threads)\n+\tif (num_threads)\n \t\thit |= wait_all();\n \tif (hit && show_in_pager)\n \t\trun_pager(&opt, prefix);\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\n-- \n2.6.3.369.g3e7f205.dirty\n"},{"id":"273363","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA1F@mail.accesssoftek.com","threadId":"40769","inReplyTo":"1447242770-20753-1-git-send-email-vleschuk@accesssoftek.com","subject":"RE: [PATCH v6] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-16T13:11:16Z","receivedAt":"2015-11-16T13:11:16Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello all,\n\nThe earlier version of this patch is already included in /pu branch, however as we all agreed ($gmane/280299) we have changed the default behavior and the meaning of \"0\". The question is: what is the right way to include changes from patch v6 (this one) into already merged patch to pu?\n\nThanks.\n--\nBest Regards,\nVictor\n________________________________________\nFrom: Victor Leschuk [vleschuk@gmail.com]\nSent: Wednesday, November 11, 2015 03:52\nTo: git@vger.kernel.org\nCc: Victor Leschuk\nSubject: [PATCH v6] Add git-grep threads param\n\n\"git grep\" can now be configured (or told from the command line)\n how many threads to use when searching in the working tree files.\n\n Changes to default behavior: number of threads now doesn't depend\n on online_cpus(), e.g. if specific number is not configured\n GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU.\n\nSigned-off-by: Victor Leschuk \n---\nHistory of changes from the first version ($gmane/280053/:\n * Param renamed from threads-num to threads\n * Short version of '--threads' cmd key was removed\n * Made num_threads 'decision-tree' more obvious\n and easy to edit for future use ($gmane/280089)\n * Moved option description to more suitable place in documentation ($gmane/280188)\n * Hid threads param from 'external' grep.c, made it private for builtin/grep.c ($gmane/280188)\n * Improved num_threads 'decision-tree', got rid of dependency on online_cpus ($gmane/280299)\n * Improved param documentation ($gmane/280299)\n\n\n Documentation/config.txt | 7 +++++\n Documentation/git-grep.txt | 15 ++++++++++\n builtin/grep.c | 50 +++++++++++++++++++++++-----------\n contrib/completion/git-completion.bash | 1 +\n 4 files changed, 57 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..5084e36 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1447,6 +1447,13 @@ grep.extendedRegexp::\n option is ignored when the 'grep.patternType' option is set to a value\n other than 'default'.\n\n+grep.threads::\n+ Number of grep worker threads, use it to tune up performance on\n+ your machines. Leave it unset (or set to 0) for default behavior,\n+ which for now is using 8 threads for all systems.\n+ Default behavior can be changed in future versions\n+ to better suit hardware and circumstances.\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\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 4a44d6d..8222a83 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -23,6 +23,7 @@ SYNOPSIS\n [--break] [--heading] [-p | --show-function]\n [-A ] [-B ] [-C ]\n [-W | --function-context]\n+ [--threads ]\n [-f ] [-e] \n [--and|--or|--not|(|)|-e ...]\n [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | ...]\n@@ -53,6 +54,13 @@ grep.extendedRegexp::\n option is ignored when the 'grep.patternType' option is set to a value\n other than 'default'.\n\n+grep.threads::\n+ Number of grep worker threads, use it to tune up performance on\n+ your machines. Leave it unset (or set to 0) for default behavior,\n+ which for now is using 8 threads for all systems.\n+ Default behavior can be changed in future versions\n+ to better suit hardware and circumstances.\n+\n grep.fullName::\n If set to true, enable '--full-name' option by default.\n\n@@ -227,6 +235,13 @@ OPTIONS\n effectively showing the whole function in which the match was\n found.\n\n+--threads ::\n+ Number of grep worker threads, use it to tune up performance on\n+ your machines. Leave it unset (or set to 0) for default behavior,\n+ which for now is using 8 threads for all systems.\n+ Default behavior can be changed in future versions\n+ to better suit hardware and circumstances.\n+\n -f ::\n Read patterns from , one per line.\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d04f440..f0e3dfb 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -24,11 +24,11 @@ static char const * const grep_usage[] = {\n NULL\n };\n\n-static int use_threads = 1;\n+#define GREP_NUM_THREADS_DEFAULT 8\n+static int num_threads = 0;\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@@ -63,13 +63,13 @@ static pthread_mutex_t grep_mutex;\n\n static inline void grep_lock(void)\n {\n- if (use_threads)\n+ if (num_threads)\n pthread_mutex_lock(&grep_mutex);\n }\n\n static inline void grep_unlock(void)\n {\n- if (use_threads)\n+ if (num_threads)\n pthread_mutex_unlock(&grep_mutex);\n }\n\n@@ -206,7 +206,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 = xcalloc(num_threads, sizeof(pthread_t));\n+ for (i = 0; i < num_threads; i++) {\n int err;\n struct grep_opt *o = grep_opt_dup(opt);\n o->output = strbuf_out;\n@@ -238,12 +239,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 < 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@@ -262,10 +265,19 @@ static int wait_all(void)\n }\n #endif\n\n+static int grep_threads_config(const char *var, const char *value, void *cb)\n+{\n+ if (!strcmp(var, \"grep.threads\"))\n+ num_threads = git_config_int(var, value); /* Sanity check of value will be perfomed later */\n+ return 0;\n+}\n+\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 (git_color_default_config(var, value, cb) < 0)\n+ if (grep_threads_config(var, value, cb) < 0)\n+ st = -1;\n+ else if (git_color_default_config(var, value, cb) < 0)\n st = -1;\n return st;\n }\n@@ -294,7 +306,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n }\n\n #ifndef NO_PTHREADS\n- if (use_threads) {\n+ if (num_threads) {\n add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n strbuf_release(&pathbuf);\n return 0;\n@@ -323,7 +335,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n strbuf_addstr(&buf, filename);\n\n #ifndef NO_PTHREADS\n- if (use_threads) {\n+ if (num_threads) {\n add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n strbuf_release(&buf);\n return 0;\n@@ -702,6 +714,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n N_(\"show context lines before matches\")),\n OPT_INTEGER('A', \"after-context\", &opt.post_context,\n N_(\"show context lines after matches\")),\n+ OPT_INTEGER(0, \"threads\", &num_threads,\n+ N_(\"use worker threads\")),\n OPT_NUMBER_CALLBACK(&opt, N_(\"shortcut for -C NUM\"),\n context_callback),\n OPT_BOOL('p', \"show-function\", &opt.funcname,\n@@ -801,7 +815,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n opt.output_priv = &path_list;\n opt.output = append_path;\n string_list_append(&path_list, show_in_pager);\n- use_threads = 0;\n+ num_threads = 0;\n }\n\n if (!opt.pattern_list)\n@@ -832,14 +846,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n }\n\n #ifndef NO_PTHREADS\n- if (list.nr || cached || online_cpus() == 1)\n- use_threads = 0;\n+ if (list.nr || cached)\n+ num_threads = 0; /* Can not multi-thread object lookup */\n+ else if (num_threads == 0)\n+ num_threads = GREP_NUM_THREADS_DEFAULT; /* User didn't specify value, or just wants default behavior */\n+ else if (num_threads < 0)\n+ die(\"Invalid number of threads specified (%d)\", num_threads);\n #else\n- use_threads = 0;\n+ num_threads = 0;\n #endif\n\n #ifndef NO_PTHREADS\n- if (use_threads) {\n+ if (num_threads) {\n if (!(opt.name_only || opt.unmatch_name_only || opt.count)\n && (opt.pre_context || opt.post_context ||\n opt.file_break || opt.funcbody))\n@@ -909,7 +927,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n hit = grep_objects(&opt, &pathspec, &list);\n }\n\n- if (use_threads)\n+ if (num_threads)\n hit |= wait_all();\n if (hit && show_in_pager)\n run_pager(&opt, prefix);\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 --full-name --line-number\n --extended-regexp --basic-regexp --fixed-strings\n --perl-regexp\n+ --threads\n --files-with-matches --name-only\n --files-without-match\n --max-depth\n--\n2.6.3.369.g3e7f205.dirty\n"},{"id":"273365","messageId":"20151116135614.GA13471@sigill.intra.peff.net","threadId":"40769","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA1F@mail.accesssoftek.com","subject":"Re: [PATCH v6] Add git-grep threads param","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-16T13:56:14Z","receivedAt":"2015-11-16T13:56:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 16, 2015 at 05:11:16AM -0800, Victor Leschuk wrote:\n\n> The earlier version of this patch is already included in /pu branch,\n> however as we all agreed ($gmane/280299) we have changed the default\n> behavior and the meaning of \"0\". The question is: what is the right\n> way to include changes from patch v6 (this one) into already merged\n> patch to pu?\n\nMerging to \"pu\" does not really mean anything; it is simply that the\nmaintainer has picked it up as a possible topic of interest. Patches can\n(and often are) still re-written in that state.\n\nJunio is on vacation for a few weeks, and I'm acting as maintainer in\nthe interim. I've added your v6 to my pile of patches to look at, but I\nhaven't gone over it carefully yet.\n\n-Peff\n"},{"id":"273382","messageId":"CAPig+cQMV-VJDff=VCeBqRLZC7Q42mu7T78_NK2TWLjEN-=cpw@mail.gmail.com","threadId":"40769","inReplyTo":"20151116135614.GA13471@sigill.intra.peff.net","subject":"Re: [PATCH v6] Add git-grep threads param","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-16T21:17:31Z","receivedAt":"2015-11-16T21:17:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Nov 16, 2015 at 8:56 AM, Jeff King wrote:\n> On Mon, Nov 16, 2015 at 05:11:16AM -0800, Victor Leschuk wrote:\n>\n>> The earlier version of this patch is already included in /pu branch,\n>> however as we all agreed ($gmane/280299) we have changed the default\n>> behavior and the meaning of \"0\". The question is: what is the right\n>> way to include changes from patch v6 (this one) into already merged\n>> patch to pu?\n>\n> Merging to \"pu\" does not really mean anything; it is simply that the\n> maintainer has picked it up as a possible topic of interest. Patches can\n> (and often are) still re-written in that state.\n>\n> Junio is on vacation for a few weeks, and I'm acting as maintainer in\n> the interim. I've added your v6 to my pile of patches to look at, but I\n> haven't gone over it carefully yet.\n\nTo be perfectly explicit: Patches in 'pu' get replaced wholesale by\nnewer versions, so you would simply send the new version of the patch\nin its entirety (as you did with v6).\n"},{"id":"273824","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5FC82F55CD@mail.accesssoftek.com","threadId":"40769","inReplyTo":"1447242770-20753-1-git-send-email-vleschuk@accesssoftek.com","subject":"RE: [PATCH v6] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-30T09:48:50Z","receivedAt":"2015-11-30T09:48:50Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello all, \n\ndoes anybody have time to review this patch?\n\nPS Sorry for bothering =)\n\n--\nBest Regards,\nVictor\n________________________________________\nFrom: Victor Leschuk [vleschuk@gmail.com]\nSent: Wednesday, November 11, 2015 03:52\nTo: git@vger.kernel.org\nCc: Victor Leschuk\nSubject: [PATCH v6] Add git-grep threads param\n\n\"git grep\" can now be configured (or told from the command line)\n how many threads to use when searching in the working tree files.\n\n Changes to default behavior: number of threads now doesn't depend\n on online_cpus(), e.g. if specific number is not configured\n GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU.\n\nSigned-off-by: Victor Leschuk \n---\nHistory of changes from the first version ($gmane/280053/:\n * Param renamed from threads-num to threads\n * Short version of '--threads' cmd key was removed\n * Made num_threads 'decision-tree' more obvious\n and easy to edit for future use ($gmane/280089)\n * Moved option description to more suitable place in documentation ($gmane/280188)\n * Hid threads param from 'external' grep.c, made it private for builtin/grep.c ($gmane/280188)\n * Improved num_threads 'decision-tree', got rid of dependency on online_cpus ($gmane/280299)\n * Improved param documentation ($gmane/280299)\n\n\n Documentation/config.txt | 7 +++++\n Documentation/git-grep.txt | 15 ++++++++++\n builtin/grep.c | 50 +++++++++++++++++++++++-----------\n contrib/completion/git-completion.bash | 1 +\n 4 files changed, 57 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..5084e36 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1447,6 +1447,13 @@ grep.extendedRegexp::\n option is ignored when the 'grep.patternType' option is set to a value\n other than 'default'.\n\n+grep.threads::\n+ Number of grep worker threads, use it to tune up performance on\n+ your machines. Leave it unset (or set to 0) for default behavior,\n+ which for now is using 8 threads for all systems.\n+ Default behavior can be changed in future versions\n+ to better suit hardware and circumstances.\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\ndiff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\nindex 4a44d6d..8222a83 100644\n--- a/Documentation/git-grep.txt\n+++ b/Documentation/git-grep.txt\n@@ -23,6 +23,7 @@ SYNOPSIS\n [--break] [--heading] [-p | --show-function]\n [-A ] [-B ] [-C ]\n [-W | --function-context]\n+ [--threads ]\n [-f ] [-e] \n [--and|--or|--not|(|)|-e ...]\n [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | ...]\n@@ -53,6 +54,13 @@ grep.extendedRegexp::\n option is ignored when the 'grep.patternType' option is set to a value\n other than 'default'.\n\n+grep.threads::\n+ Number of grep worker threads, use it to tune up performance on\n+ your machines. Leave it unset (or set to 0) for default behavior,\n+ which for now is using 8 threads for all systems.\n+ Default behavior can be changed in future versions\n+ to better suit hardware and circumstances.\n+\n grep.fullName::\n If set to true, enable '--full-name' option by default.\n\n@@ -227,6 +235,13 @@ OPTIONS\n effectively showing the whole function in which the match was\n found.\n\n+--threads ::\n+ Number of grep worker threads, use it to tune up performance on\n+ your machines. Leave it unset (or set to 0) for default behavior,\n+ which for now is using 8 threads for all systems.\n+ Default behavior can be changed in future versions\n+ to better suit hardware and circumstances.\n+\n -f ::\n Read patterns from , one per line.\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d04f440..f0e3dfb 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -24,11 +24,11 @@ static char const * const grep_usage[] = {\n NULL\n };\n\n-static int use_threads = 1;\n+#define GREP_NUM_THREADS_DEFAULT 8\n+static int num_threads = 0;\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@@ -63,13 +63,13 @@ static pthread_mutex_t grep_mutex;\n\n static inline void grep_lock(void)\n {\n- if (use_threads)\n+ if (num_threads)\n pthread_mutex_lock(&grep_mutex);\n }\n\n static inline void grep_unlock(void)\n {\n- if (use_threads)\n+ if (num_threads)\n pthread_mutex_unlock(&grep_mutex);\n }\n\n@@ -206,7 +206,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 = xcalloc(num_threads, sizeof(pthread_t));\n+ for (i = 0; i < num_threads; i++) {\n int err;\n struct grep_opt *o = grep_opt_dup(opt);\n o->output = strbuf_out;\n@@ -238,12 +239,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 < 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@@ -262,10 +265,19 @@ static int wait_all(void)\n }\n #endif\n\n+static int grep_threads_config(const char *var, const char *value, void *cb)\n+{\n+ if (!strcmp(var, \"grep.threads\"))\n+ num_threads = git_config_int(var, value); /* Sanity check of value will be perfomed later */\n+ return 0;\n+}\n+\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 (git_color_default_config(var, value, cb) < 0)\n+ if (grep_threads_config(var, value, cb) < 0)\n+ st = -1;\n+ else if (git_color_default_config(var, value, cb) < 0)\n st = -1;\n return st;\n }\n@@ -294,7 +306,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,\n }\n\n #ifndef NO_PTHREADS\n- if (use_threads) {\n+ if (num_threads) {\n add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);\n strbuf_release(&pathbuf);\n return 0;\n@@ -323,7 +335,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n strbuf_addstr(&buf, filename);\n\n #ifndef NO_PTHREADS\n- if (use_threads) {\n+ if (num_threads) {\n add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n strbuf_release(&buf);\n return 0;\n@@ -702,6 +714,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n N_(\"show context lines before matches\")),\n OPT_INTEGER('A', \"after-context\", &opt.post_context,\n N_(\"show context lines after matches\")),\n+ OPT_INTEGER(0, \"threads\", &num_threads,\n+ N_(\"use worker threads\")),\n OPT_NUMBER_CALLBACK(&opt, N_(\"shortcut for -C NUM\"),\n context_callback),\n OPT_BOOL('p', \"show-function\", &opt.funcname,\n@@ -801,7 +815,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n opt.output_priv = &path_list;\n opt.output = append_path;\n string_list_append(&path_list, show_in_pager);\n- use_threads = 0;\n+ num_threads = 0;\n }\n\n if (!opt.pattern_list)\n@@ -832,14 +846,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n }\n\n #ifndef NO_PTHREADS\n- if (list.nr || cached || online_cpus() == 1)\n- use_threads = 0;\n+ if (list.nr || cached)\n+ num_threads = 0; /* Can not multi-thread object lookup */\n+ else if (num_threads == 0)\n+ num_threads = GREP_NUM_THREADS_DEFAULT; /* User didn't specify value, or just wants default behavior */\n+ else if (num_threads < 0)\n+ die(\"Invalid number of threads specified (%d)\", num_threads);\n #else\n- use_threads = 0;\n+ num_threads = 0;\n #endif\n\n #ifndef NO_PTHREADS\n- if (use_threads) {\n+ if (num_threads) {\n if (!(opt.name_only || opt.unmatch_name_only || opt.count)\n && (opt.pre_context || opt.post_context ||\n opt.file_break || opt.funcbody))\n@@ -909,7 +927,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n hit = grep_objects(&opt, &pathspec, &list);\n }\n\n- if (use_threads)\n+ if (num_threads)\n hit |= wait_all();\n if (hit && show_in_pager)\n run_pager(&opt, prefix);\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 --full-name --line-number\n --extended-regexp --basic-regexp --fixed-strings\n --perl-regexp\n+ --threads\n --files-with-matches --name-only\n --files-without-match\n --max-depth\n--\n2.6.3.369.g3e7f205.dirty\n"},{"id":"273838","messageId":"CACsJy8AdRnW88uy+U-Q0TKf05KvDQLf3bDcKmqoLTDT3sAzg+w@mail.gmail.com","threadId":"40769","inReplyTo":"1447242770-20753-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH v6] Add git-grep threads param","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-11-30T19:31:08Z","receivedAt":"2015-11-30T19:31:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 11, 2015 at 12:52 PM, Victor Leschuk wrote:\n> \"git grep\" can now be configured (or told from the command line)\n> how many threads to use when searching in the working tree files.\n>\n> Changes to default behavior: number of threads now doesn't depend\n> on online_cpus(), e.g. if specific number is not configured\n> GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU.\n\nWhy? (I'm asking for an explanation in the commit message so that I\nwill not have to ask again in future)\n\n> @@ -206,7 +206,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 = xcalloc(num_threads, sizeof(pthread_t));\n\nI think we usually go with sizeof(*threads), but not sure if it's just\na personal taste or the preferred style for git.\n\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 (git_color_default_config(var, value, cb) < 0)\n> + if (grep_threads_config(var, value, cb) < 0)\n> + st = -1;\n> + else if (git_color_default_config(var, value, cb) < 0)\n> st = -1;\n> return st;\n\nHm... isn't it simpler to just return -1 instead of assigning to st\nfirst? I think you could just merge grep_threads_config() in this\nfunction because it's not that complex to stay separate..\n\n> }\n-- \nDuy\n"},{"id":"273841","messageId":"CACsJy8CyV9K8Kxwd-nOugjsTXN09afJFnXwR9mOE5FpA_hWacg@mail.gmail.com","threadId":"40769","inReplyTo":"1447242770-20753-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH v6] Add git-grep threads param","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-11-30T19:45:24Z","receivedAt":"2015-11-30T19:45:24Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"I forgot..\n\nOn Wed, Nov 11, 2015 at 12:52 PM, Victor Leschuk wrote:\n> + else if (num_threads < 0)\n> + die(\"Invalid number of threads specified (%d)\", num_threads);\n\nPlease wrap this string with _() so it can be translated\n-- \nDuy\n"},{"id":"274019","messageId":"xmqqh9jyrn3w.fsf@gitster.mtv.corp.google.com","threadId":"40769","inReplyTo":"1447242770-20753-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH v6] Add git-grep threads param","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-04T20:10:43Z","receivedAt":"2015-12-04T20:10:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This version seems to break t7811 when applied on top of 37023ba3\n(Seventh batch for 2.7, 2015-10-26).\n\nI'll eject it from 'pu' for today's integration.\n"}]}