{"thread":{"id":"41005","subject":"[PATCH v8 0/2] Add git-grep threads param","startedAt":"2015-12-15T15:31:38Z","lastAt":"2015-12-16T00:26:46Z","messageCount":6,"participants":["Victor Leschuk","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":8,"patchTotal":2},"messages":[{"id":"274493","messageId":"1450193500-22468-1-git-send-email-vleschuk@accesssoftek.com","threadId":"41005","inReplyTo":null,"subject":"[PATCH v8 0/2] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-12-15T15:31:38Z","receivedAt":"2015-12-15T15:31:38Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"Introducing v8 of git-grep threads param patch.\nPatch is now split in 2 parts - 1/2 is actually renewed v6 version (see list of changes below),\n2/2 removes dependency on online_cpus() - as we discussed with Eric this is rather \nsignificant change in default behavior and should be placed into separate patch.\n\nHere is list of changes since v6 ($gmane/281160):\n\n  * Fixed broken t7811: moved all threads_num setup to 1 place (for -O option it was in wrong place)\n  * Fixed 'invalid number of threads' message so that it could be translated\n  * Got rid of grep_threads_config() - its too trivial to be separate function\n  * Fixed xcalloc() args (sizeof(pthread_t) -> sizeof(*threads)) to correspond to general git style\n  * Improved commit message (in 2/2) to explain why online_cpus() is now not used in threads_num setup\n  * The full param documentation was moved into single place (grep.threads description in git-grep.txt) and is referenced from other places. Also made few language improvements in documentation.\n  * Style improvements: splitted too long lines\n\nVictor Leschuk (2):\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  Number of threads now doesn't depend on online_cpus(),     e.g. if\n    specific number is not configured GREP_NUM_THREADS_DEFAULT (8)    \n    threads will be used even on 1-core CPU.\n\n Documentation/config.txt               |  4 +++\n Documentation/git-grep.txt             | 12 +++++++++\n builtin/grep.c                         | 49 +++++++++++++++++++++++-----------\n contrib/completion/git-completion.bash |  1 +\n 4 files changed, 51 insertions(+), 15 deletions(-)\n\n-- \n2.6.3.369.g3e7f205.dirty\n"},{"id":"274494","messageId":"1450193500-22468-2-git-send-email-vleschuk@accesssoftek.com","threadId":"41005","inReplyTo":"1450193500-22468-1-git-send-email-vleschuk@accesssoftek.com","subject":"[PATCH 1/2] Introduce grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-12-15T15:31:39Z","receivedAt":"2015-12-15T15:31:39Z","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\nSigned-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n---\n Documentation/config.txt               |  4 +++\n Documentation/git-grep.txt             | 12 +++++++++\n builtin/grep.c                         | 49 +++++++++++++++++++++++-----------\n contrib/completion/git-completion.bash |  1 +\n 4 files changed, 51 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 2d06b11..cbf4071 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1450,6 +1450,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.\n+\tSee `grep.threads` in linkgit:git-grep[1] for more information.\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..25e6dc5 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 <post-context>] [-B <pre-context>] [-C <context>]\n \t   [-W | --function-context]\n+\t   [--threads <num>]\n \t   [-f <file>] [-e] <pattern>\n \t   [--and|--or|--not|(|)|-e <pattern>...]\n \t   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]\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 is using 8 threads for all systems.\n+\tDefault behavior may change 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,10 @@ OPTIONS\n \teffectively showing the whole function in which the match was\n \tfound.\n \n+--threads <num>::\n+\tNumber of grep worker threads.\n+\tSee `grep.threads` in 'CONFIGURATION' for more information.\n+\n -f <file>::\n \tRead patterns from <file>, one per line.\n \ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 4229cae..e9aebab 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(*threads));\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@@ -267,6 +270,12 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)\n \tint st = grep_config(var, value, cb);\n \tif (git_color_default_config(var, value, cb) < 0)\n \t\tst = -1;\n+\n+\tif (!strcmp(var, \"grep.threads\")) {\n+\t\t/* Sanity check of value will be perfomed later */\n+\t\tnum_threads = git_config_int(var, value);\n+\t}\n+\n \treturn st;\n }\n \n@@ -294,7 +303,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 +332,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@@ -697,6 +706,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\", &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@@ -786,7 +797,6 @@ 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}\n \n \tif (!opt.pattern_list)\n@@ -817,14 +827,23 @@ 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 || online_cpus() == 1 || show_in_pager) {\n+\t\t/* Can not multi-thread object lookup */\n+\t\tnum_threads = 0;\n+\t}\n+\telse if (num_threads == 0) {\n+\t\t/* User didn't specify value, or just wants default behavior */\n+\t\tnum_threads = GREP_NUM_THREADS_DEFAULT;\n+\t}\n+\telse if (num_threads < 0) {\n+\t\tdie(_(\"invalid number of threads specified (%d)\"), num_threads);\n+\t}\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@@ -894,7 +913,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 111b053..d5c3e3f 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1311,6 +1311,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":"274495","messageId":"1450193500-22468-3-git-send-email-vleschuk@accesssoftek.com","threadId":"41005","inReplyTo":"1450193500-22468-1-git-send-email-vleschuk@accesssoftek.com","subject":"[PATCH 2/2] Get rid of online_cpus() when determining grep threads num","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-12-15T15:31:40Z","receivedAt":"2015-12-15T15:31:40Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"Number of threads now doesn't depend on online_cpus(),\ne.g. if specific number is not configured GREP_NUM_THREADS_DEFAULT (8)\nthreads will be used even on 1-core CPU.\n\nReason: multithreading can improve performance even on single core machines\nas IO is also a major factor here. Using multiple threads can significantly\nboost grep performance when working on slow filesystems (or repo isn't cached)\nor through network (for example repo is located on NFS).\n\nSigned-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n---\n builtin/grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex e9aebab..1315905 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -827,7 +827,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 || show_in_pager) {\n+\tif (list.nr || cached || show_in_pager) {\n \t\t/* Can not multi-thread object lookup */\n \t\tnum_threads = 0;\n \t}\n-- \n2.6.3.369.g3e7f205.dirty\n"},{"id":"274512","messageId":"xmqq60zzfpdz.fsf@gitster.mtv.corp.google.com","threadId":"41005","inReplyTo":"1450193500-22468-2-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH 1/2] Introduce grep threads param","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-12-15T20:06:16Z","receivedAt":"2015-12-15T20:06:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victor Leschuk <vleschuk@gmail.com> writes:\n\n> Subject: Re: [PATCH 1/2] Introduce grep threads param\n\nI'll retitle this to something like\n\n    grep: add --threads=<num> option and grep.threads configuration\n\nwhile queuing (which I did for v7 earlier).\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> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n> ---\n> ...\n> +grep.threads::\n> +\tNumber of grep worker threads.\n\n\"Number of grep worker threads to use\"?\n\n> +\tSee `grep.threads` in linkgit:git-grep[1] for more information.\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 is using 8 threads for all systems.\n> +\tDefault behavior may change in future versions\n> +\tto better suit hardware and circumstances.\n\nThe last sentence is too noisy.  Perhaps drop it and phrase it like\nthis instead?\n\n    grep.threads::\n            Number of grep worker threads to use.  If unset (or set to 0),\n            to 0), 8 threads are used by default (for now).\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 4229cae..e9aebab 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\nPlease do not initialize static to 0 (or NULL).\n\n> @@ -267,6 +270,12 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)\n>  \tint st = grep_config(var, value, cb);\n>  \tif (git_color_default_config(var, value, cb) < 0)\n>  \t\tst = -1;\n> +\n> +\tif (!strcmp(var, \"grep.threads\")) {\n> +\t\t/* Sanity check of value will be perfomed later */\n\nHmm, is that a good design?\n\nA user may hear \"invalid number of threads specified (-4)\" later,\nbut if that came from \"grep.threads\", especially when the user did\nnot say \"--threads=-4\" from the command line, would she know to\ncheck her configuration file?\n\nIf she had \"grep.threads=Yes\" in her configuration, we would\nhelpfully tell her that 'Yes' given to grep.threads is not a valid\ninteger.  Shouldn't we do the same for '-4' given to grep.threads,\ntoo?\n\n\tif (!strcmp(var, \"grep.threads\")) {\n\t\tnum_threads = git_config_int(var, value);\n\t\tif (num_threads < 0)\n\t\t\tdie(_(\"invalid number of threads specified (%d) for %s\"),\n\t\t\t    num_threads, var);\n\t}\n\nperhaps.\n\n> @@ -817,14 +827,23 @@ 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 || online_cpus() == 1 || show_in_pager) {\n> +\t\t/* Can not multi-thread object lookup */\n> +\t\tnum_threads = 0;\n\nRemoving 'use_threads = 0' from an earlier part and moving the check\nto show_in_pager is a good idea, but it invalidates this comment.\nThe earlier three (actually two and a half) are \"cannot\" cases,\ni.e. the object layer is not easily threaded without locking, and\nwhen you have a single core, you do not truly run multiple\noperations at the same time, but as [PATCH 2/2] does, threading in\n\"grep\" is not about CPU alone, so that is why I am demoting it to\njust a half ;-).  But show_in_pager is \"we do not want to\", I think.\n\nIn any case, this comment and \"User didn't specify\" below are not\ntelling the reader something very much useful.  You probably should\nremove them.\n\n> +\t}\n> +\telse if (num_threads == 0) {\n> +\t\t/* User didn't specify value, or just wants default behavior */\n> +\t\tnum_threads = GREP_NUM_THREADS_DEFAULT;\n> +\t}\n> +\telse if (num_threads < 0) {\n> +\t\tdie(_(\"invalid number of threads specified (%d)\"), num_threads);\n> +\t}\n\nMany unnecessary braces.\n\nI think [2/2] and also moving the code to disable threading when\nshow-in-pager mode should be separate \"preparatory clean-up\" patches\nbefore this main patch.  I'll push out what I think this topic\nshould be on 'pu' later today (with fixups suggested above squashed\nin); please check them and see what you think.\n\nThanks.\n"},{"id":"274513","messageId":"56707648.4050005@gmail.com","threadId":"41005","inReplyTo":"xmqq60zzfpdz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Introduce grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-12-15T20:21:28Z","receivedAt":"2015-12-15T20:21:28Z","isPatch":true,"sender":{"key":"vleschuk@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1045374?v=4"},"body":"Hello Junio,\n\nOn 12/15/2015 11:06 PM, Junio C Hamano wrote:\n> Victor Leschuk <vleschuk@gmail.com> writes:\n>\n>> Subject: Re: [PATCH 1/2] Introduce grep threads param\n> I'll retitle this to something like\n>\n>      grep: add --threads=<num> option and grep.threads configuration\n>\n> while queuing (which I did for v7 earlier).\nOk, thanks.\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>> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n>> ---\n>> ...\n>> +grep.threads::\n>> +\tNumber of grep worker threads.\n> \"Number of grep worker threads to use\"?\nAccepted, will change.\n>\n>> +\tSee `grep.threads` in linkgit:git-grep[1] for more information.\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 is using 8 threads for all systems.\n>> +\tDefault behavior may change in future versions\n>> +\tto better suit hardware and circumstances.\n> The last sentence is too noisy.  Perhaps drop it and phrase it like\n> this instead?\n>\n>      grep.threads::\n>              Number of grep worker threads to use.  If unset (or set to 0),\n>              to 0), 8 threads are used by default (for now).\nAgree.\n>\n>> diff --git a/builtin/grep.c b/builtin/grep.c\n>> index 4229cae..e9aebab 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> Please do not initialize static to 0 (or NULL).\nOk.\n>\n>> @@ -267,6 +270,12 @@ static int grep_cmd_config(const char *var, const char *value, void *cb)\n>>   \tint st = grep_config(var, value, cb);\n>>   \tif (git_color_default_config(var, value, cb) < 0)\n>>   \t\tst = -1;\n>> +\n>> +\tif (!strcmp(var, \"grep.threads\")) {\n>> +\t\t/* Sanity check of value will be perfomed later */\n> Hmm, is that a good design?\n>\n> A user may hear \"invalid number of threads specified (-4)\" later,\n> but if that came from \"grep.threads\", especially when the user did\n> not say \"--threads=-4\" from the command line, would she know to\n> check her configuration file?\n>\n> If she had \"grep.threads=Yes\" in her configuration, we would\n> helpfully tell her that 'Yes' given to grep.threads is not a valid\n> integer.  Shouldn't we do the same for '-4' given to grep.threads,\n> too?\n>\n> \tif (!strcmp(var, \"grep.threads\")) {\n> \t\tnum_threads = git_config_int(var, value);\n> \t\tif (num_threads < 0)\n> \t\t\tdie(_(\"invalid number of threads specified (%d) for %s\"),\n> \t\t\t    num_threads, var);\n> \t}\n>\n> perhaps.\nWhen I was doing this I looked at other code and saw that exact message \n\"Invalid number of threads...\" is used in other parts of git and is \npresent in 'po' translations. Thus I decided to use exactly the same \nmessage in order not to create numerous almost similar localizations \n(which we should do if we use two different messages in different \nplaces). Do you think we need to create two more different messages (and \ntranslations, I can prepare Russian and French) and create two checks \nfor cmd and config?\n>\n>> @@ -817,14 +827,23 @@ 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 || online_cpus() == 1 || show_in_pager) {\n>> +\t\t/* Can not multi-thread object lookup */\n>> +\t\tnum_threads = 0;\n> Removing 'use_threads = 0' from an earlier part and moving the check\n> to show_in_pager is a good idea, but it invalidates this comment.\n> The earlier three (actually two and a half) are \"cannot\" cases,\n> i.e. the object layer is not easily threaded without locking, and\n> when you have a single core, you do not truly run multiple\n> operations at the same time, but as [PATCH 2/2] does, threading in\n> \"grep\" is not about CPU alone, so that is why I am demoting it to\n> just a half ;-).  But show_in_pager is \"we do not want to\", I think.\n>\n> In any case, this comment and \"User didn't specify\" below are not\n> telling the reader something very much useful.  You probably should\n> remove them.\nOk, will remove comments.\n>\n>> +\t}\n>> +\telse if (num_threads == 0) {\n>> +\t\t/* User didn't specify value, or just wants default behavior */\n>> +\t\tnum_threads = GREP_NUM_THREADS_DEFAULT;\n>> +\t}\n>> +\telse if (num_threads < 0) {\n>> +\t\tdie(_(\"invalid number of threads specified (%d)\"), num_threads);\n>> +\t}\n> Many unnecessary braces.\nI put braces to make code look more unified. I had to put braces here:\n\n+\telse if (num_threads == 0) {\n+\t\t/* User didn't specify value, or just wants default behavior */\n\n\nIn order to be able to place long comment above the line. And code like:\n\nif (a)\n   do_smth();\nelse if (b) {\n   do_smth1();\n   do_smth2();\n}\nelse\n   do_smth3();\n\nLooks rather ugly to me. Usually I put braces to all 'ifs' if any part \nof block requires them.\nActually I will remove comments as I said above, and thus remove all braces.\n>\n> I think [2/2] and also moving the code to disable threading when\n> show-in-pager mode should be separate \"preparatory clean-up\" patches\n> before this main patch.  I'll push out what I think this topic\n> should be on 'pu' later today (with fixups suggested above squashed\n> in); please check them and see what you think.\n>\n> Thanks.\nOk, I will prepare v8 as soon as we finish discussion on this one.\n\nThanks for the review.\n\n--\nVictor\n"},{"id":"274538","messageId":"CAPig+cRTz=DMd6XyJ=co26d2c=PgVqhhsWQpgy53930MdC_=Rw@mail.gmail.com","threadId":"41005","inReplyTo":"xmqq60zzfpdz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Introduce grep threads param","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-12-16T00:26:46Z","receivedAt":"2015-12-16T00:26:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Dec 15, 2015 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Victor Leschuk <vleschuk@gmail.com> writes:\n>> Subject: Re: [PATCH 1/2] Introduce grep threads param\n>\n> I'll retitle this to something like\n>\n>     grep: add --threads=<num> option and grep.threads configuration\n>\n> while queuing (which I did for v7 earlier).\n>\n> I think [2/2] and also moving the code to disable threading when\n> show-in-pager mode should be separate \"preparatory clean-up\" patches\n> before this main patch.  I'll push out what I think this topic\n> should be on 'pu' later today (with fixups suggested above squashed\n> in); please check them and see what you think.\n\nI read over what was pushed to 'pu' and noticed a couple problems.\n\nFirst, the 'online_cpus() == 1' check, which was removed in patch 1/3,\naccidentally creeps back in with patch 3/3.\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 is using 8 threads for all systems.\n>> +     Default behavior may change in future versions\n>> +     to better suit hardware and circumstances.\n>\n> The last sentence is too noisy.  Perhaps drop it and phrase it like\n> this instead?\n>\n>     grep.threads::\n>             Number of grep worker threads to use.  If unset (or set to 0),\n>             to 0), 8 threads are used by default (for now).\n\nSecond, the stray \"to 0),\" on the second line needs to be dropped.\n\nOther than that, the series looks reasonable.\n"}]}