{"thread":{"id":"40658","subject":"[PATCH v4] Add git-grep threads param","startedAt":"2015-10-27T21:22:24Z","lastAt":"2015-11-09T21:51:48Z","messageCount":19,"participants":["Victor Leschuk","Junio C Hamano","Jeff King","Linus Torvalds","Stefan Beller"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"272343","messageId":"1445980944-24000-1-git-send-email-vleschuk@accesssoftek.com","threadId":"40658","inReplyTo":null,"subject":"[PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-10-27T21:22:24Z","receivedAt":"2015-10-27T21:22:24Z","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 \n---\n Documentation/config.txt | 4 +++\n Documentation/git-grep.txt | 9 ++++++\n builtin/grep.c | 56 ++++++++++++++++++++++++----------\n contrib/completion/git-completion.bash | 1 +\n 4 files changed, 54 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..1dd2a61 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. Set to 0 to disable threading.\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..e766596 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,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. Set to 0 to disable threading.\n+\n grep.fullName::\n \tIf set to true, enable '--full-name' option by default.\n \n@@ -227,6 +232,10 @@ OPTIONS\n \teffectively showing the whole function in which the match was\n \tfound.\n \n+--threads ::\n+\tSet number of worker threads to . Default is 8.\n+\tSet to 0 to disable threading.\n+\n -f ::\n \tRead patterns from , one per line.\n \ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d04f440..694553e 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 = -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@@ -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,22 @@ 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);\n+\t\tif (num_threads < 0)\n+\t\t\tdie(\"Invalid number of threads specified (%d)\", num_threads);\n+\t}\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 +309,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 +338,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 +717,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 +818,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 +849,21 @@ 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+\t}\n+\telse if (num_threads < 0 && online_cpus() <= 1) {\n+\t\tnum_threads = 0; /* User didn't set threading option and we have <= 1 of hardware cores */\n+\t}\n+\telse if (num_threads < 0) {\n+\t\tnum_threads = GREP_NUM_THREADS_DEFAULT;\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@@ -909,7 +933,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.2.308.g3b8f10c.dirty\n"},{"id":"272717","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA0C@mail.accesssoftek.com","threadId":"40658","inReplyTo":"1445980944-24000-1-git-send-email-vleschuk@accesssoftek.com","subject":"RE: [PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-02T04:43:39Z","receivedAt":"2015-11-02T04:43:39Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"Hello all,\n\ndo we have any objections on this patch?\n\n--\nBest Regards,\nVictor\n________________________________________\nFrom: Victor Leschuk [vleschuk@gmail.com]\nSent: Tuesday, October 27, 2015 14:22\nTo: git@vger.kernel.org\nCc: Victor Leschuk\nSubject: [PATCH v4] Add git-grep threads param\n\nMake 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 \n---\n Documentation/config.txt | 4 +++\n Documentation/git-grep.txt | 9 ++++++\n builtin/grep.c | 56 ++++++++++++++++++++++++----------\n contrib/completion/git-completion.bash | 1 +\n 4 files changed, 54 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 391a0c3..1dd2a61 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1447,6 +1447,10 @@ grep.extendedRegexp::\n option is ignored when the 'grep.patternType' option is set to a value\n other than 'default'.\n\n+grep.threads::\n+ Number of grep worker threads, use it to tune up performance on\n+ multicore machines. Default value is 8. Set to 0 to disable threading.\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..e766596 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,10 @@ 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+ multicore machines. Default value is 8. Set to 0 to disable threading.\n+\n grep.fullName::\n If set to true, enable '--full-name' option by default.\n\n@@ -227,6 +232,10 @@ OPTIONS\n effectively showing the whole function in which the match was\n found.\n\n+--threads ::\n+ Set number of worker threads to . Default is 8.\n+ Set to 0 to disable threading.\n+\n -f ::\n Read patterns from , one per line.\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d04f440..694553e 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 = -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@@ -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,22 @@ 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);\n+ if (num_threads < 0)\n+ die(\"Invalid number of threads specified (%d)\", num_threads);\n+ }\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 +309,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 +338,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 +717,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 +818,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 +849,21 @@ 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+ }\n+ else if (num_threads < 0 && online_cpus() <= 1) {\n+ num_threads = 0; /* User didn't set threading option and we have <= 1 of hardware cores */\n+ }\n+ else if (num_threads < 0) {\n+ num_threads = GREP_NUM_THREADS_DEFAULT;\n+ }\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 +933,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.2.308.g3b8f10c.dirty\n"},{"id":"272732","messageId":"xmqqlhagfn95.fsf@gitster.mtv.corp.google.com","threadId":"40658","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA0C@mail.accesssoftek.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-02T15:13:10Z","receivedAt":"2015-11-02T15:13:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victor Leschuk writes:\n\n> do we have any objections on this patch?\n\nThe question you should be asking is \"do we have any support\".\n\nIt is not like the default for any series is to be included; it is\nquite the opposite. \"Is this worth having in our tree?\" is the\nquestion we all ask ourselves.\n\nAlso I think you should have CC'ed those who gave review comments on\nthis topic in the earlier rounds, if you wanted to receive answers\nto that question (whichever one it is ;-).\n\nHaving said that, I didn't immediately spot anything objectionable.\n\nThanks.\n"},{"id":"272821","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA10@mail.accesssoftek.com","threadId":"40658","inReplyTo":"xmqqlhagfn95.fsf@gitster.mtv.corp.google.com","subject":"RE: [PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-03T08:36:33Z","receivedAt":"2015-11-03T08:36:33Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":">> do we have any objections on this patch?\n\n> The question you should be asking is \"do we have any support\".\n\nHello all, CCing participated reviewers. As Junio has correctly mentioned: \"Do we have any support for including this functionality?\"\n\nI think this kind of customization can be useful in tuning up performance as confirmed by conducted tests. And in most cases having anything hard-coded is worse than giving users opportunity to change it, isn't it?\n"},{"id":"272838","messageId":"xmqqvb9jc81q.fsf@gitster.mtv.corp.google.com","threadId":"40658","inReplyTo":"1445980944-24000-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-11-03T17:22:09Z","receivedAt":"2015-11-03T17:22:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Victor Leschuk writes:\n\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 \n> ---\n> Documentation/config.txt | 4 +++\n> Documentation/git-grep.txt | 9 ++++++\n> builtin/grep.c | 56 ++++++++++++++++++++++++----------\n> contrib/completion/git-completion.bash | 1 +\n> 4 files changed, 54 insertions(+), 16 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 391a0c3..1dd2a61 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. Set to 0 to disable threading.\n> +\n\nI am not enthused by this \"Set to 0 to disable\". As Zero is\nmagical, it would be more useful if 1 meant that threading is not\nused (i.e. there is only 1 worker), and 0 meant that we would\nautomatically pick some reasonable parallelism for you (and we\npromise that the our choice would not be outrageously wrong), or\nsomething like that.\n\nOf course, we can do grep.threads=auto to make it even more\nexplicit, but I'd imagine that the in-core code to parse the option\nand config to the num_threads variable would need one \"int\" value to\nrepresent that \"auto\", and 0 would be a natural choice for that.\n\nThanks.\n"},{"id":"272888","messageId":"20151104064021.GB16605@sigill.intra.peff.net","threadId":"40658","inReplyTo":"xmqqvb9jc81q.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-04T06:40:21Z","receivedAt":"2015-11-04T06:40:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 03, 2015 at 09:22:09AM -0800, Junio C Hamano wrote:\n\n> > +grep.threads::\n> > +\tNumber of grep worker threads, use it to tune up performance on\n> > +\tmulticore machines. Default value is 8. Set to 0 to disable threading.\n> > +\n> \n> I am not enthused by this \"Set to 0 to disable\". As Zero is\n> magical, it would be more useful if 1 meant that threading is not\n> used (i.e. there is only 1 worker), and 0 meant that we would\n> automatically pick some reasonable parallelism for you (and we\n> promise that the our choice would not be outrageously wrong), or\n> something like that.\n\nNot just useful, but consistent with other parts of git, like\npack.threads, where \"0\" already means \"autodetect\".\n\n-Peff\n"},{"id":"273076","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA15@mail.accesssoftek.com","threadId":"40658","inReplyTo":"20151104064021.GB16605@sigill.intra.peff.net","subject":"RE: [PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-09T11:36:16Z","receivedAt":"2015-11-09T11:36:16Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"\n________________________________________\nFrom: Jeff King [peff@peff.net]\nSent: Tuesday, November 03, 2015 22:40\nTo: Junio C Hamano\nCc: Victor Leschuk; git@vger.kernel.org; Victor Leschuk; torvalds@linux-foundation.org; john@keeping.me.uk\nSubject: Re: [PATCH v4] Add git-grep threads param\n\nOn Tue, Nov 03, 2015 at 09:22:09AM -0800, Junio C Hamano wrote:\n\n> > +grep.threads::\n> > + Number of grep worker threads, use it to tune up performance on\n> > + multicore machines. Default value is 8. Set to 0 to disable threading.\n> > +\n>\n> I am not enthused by this \"Set to 0 to disable\". As Zero is\n> magical, it would be more useful if 1 meant that threading is not\n> used (i.e. there is only 1 worker), and 0 meant that we would\n> automatically pick some reasonable parallelism for you (and we\n> promise that the our choice would not be outrageously wrong), or\n> something like that.\n\n> Not just useful, but consistent with other parts of git, like\n> pack.threads, where \"0\" already means \"autodetect\".\n\nHello Peff and Junio,\n\nYeah do also think it would be more reasonable to use \"0\" for \"autodetect\" default value. However chat this autodetect value should be?\n\nFor index index-pack and pack-objects we use ncpus() for this, however according to my tests this wouldn't an Ideal for all cases. Maybe it should be something like ncpus()*2, \nanyway before it we even used hard-coded 8 for all systems...\n\nIn this case we use \"1\" as \"Do not use threads\" and die on all negative numbers during parsing.\n\nAgreed?\n"},{"id":"273082","messageId":"20151109155538.GC27224@sigill.intra.peff.net","threadId":"40658","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA15@mail.accesssoftek.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-09T15:55:38Z","receivedAt":"2015-11-09T15:55:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 09, 2015 at 03:36:16AM -0800, Victor Leschuk wrote:\n\n> Yeah do also think it would be more reasonable to use \"0\" for\n> \"autodetect\" default value. However chat this autodetect value should\n> be?\n> \n> For index index-pack and pack-objects we use ncpus() for this, however\n> according to my tests this wouldn't an Ideal for all cases. Maybe it\n> should be something like ncpus()*2, \n> anyway before it we even used hard-coded 8 for all systems...\n\nWhy don't we leave it at 8, then? That's the conservative choice, and\nonce we have --threads, people can easily experiment with different\nvalues and we can follow-up with a change to the default if need be.\n\n-Peff\n"},{"id":"273083","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA17@mail.accesssoftek.com","threadId":"40658","inReplyTo":"20151109155538.GC27224@sigill.intra.peff.net","subject":"RE: [PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-09T16:34:27Z","receivedAt":"2015-11-09T16:34:27Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"> Why don't we leave it at 8, then? That's the conservative choice, and\n> once we have --threads, people can easily experiment with different\n> values and we can follow-up with a change to the default if need be.\n\nI'd propose the following:\n\n if (list.nr || cached) { \n num_threads = 0; /* Can not multi-thread object lookup */ \n } \n else if (num_threads < 0 && online_cpus() <= 1) { \n num_threads = 0; /* User didn't set threading option and we have <= 1 of hardware cores */ \n } \n else if (num_threads == 0) { \n num_threads = GREP_NUM_THREADS_DEFAULT; /* User explicitly choose default behavior */ \n } \n else if (num_threads < 0) { /* Actually this one should be checked earlier so no need to double check here */ \n die(_(\"Ivalid number of threads specified (%d)\"), num_threads) \n } \n\n--\nVictor\n"},{"id":"273084","messageId":"20151109165343.GA29179@sigill.intra.peff.net","threadId":"40658","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA17@mail.accesssoftek.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-09T16:53:43Z","receivedAt":"2015-11-09T16:53:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 09, 2015 at 08:34:27AM -0800, Victor Leschuk wrote:\n\n> > Why don't we leave it at 8, then? That's the conservative choice, and\n> > once we have --threads, people can easily experiment with different\n> > values and we can follow-up with a change to the default if need be.\n>\n> I'd propose the following:\n>\n> if (list.nr || cached) {\n> num_threads = 0; /* Can not multi-thread object lookup */\n> }\n> else if (num_threads < 0 && online_cpus() <= 1) {\n> num_threads = 0; /* User didn't set threading option and we have <= 1 of hardware cores */\n> }\n\nOK, so you are presumably initializing:\n\n static int num_threads = -1;\n\n> else if (num_threads == 0) {\n> num_threads = GREP_NUM_THREADS_DEFAULT; /* User explicitly choose default behavior */\n> }\n> else if (num_threads < 0) { /* Actually this one should be checked earlier so no need to double check here */\n> die(_(\"Ivalid number of threads specified (%d)\"), num_threads)\n> }\n\nWhat happens if the user has not specified a value (nr_threads == -1)?\nHere you die, but shouldn't you take the default thread value?\n\nI wonder if it would be simpler to just default to 0, and then treat\nnegative values the same as 0 (which is what pack.threads does). Like:\n\n if (list.nr || cached)\n\tnum_threads = 1;\n if (!num_threads)\n\tnum_threads = GREP_NUM_THREADS_DEFAULT;\n\nand then later, instead of use_threads, do:\n\n if (num_threads <= 1) {\n\t... do single-threaded version ...\n } else {\n ... do multi-threaded version ...\n }\n\nThat matches the logic in builtin/pack-objects.c.\n\n-Peff\n"},{"id":"273096","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA18@mail.accesssoftek.com","threadId":"40658","inReplyTo":"20151109165343.GA29179@sigill.intra.peff.net","subject":"RE: [PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-09T17:28:12Z","receivedAt":"2015-11-09T17:28:12Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"\n > if (list.nr || cached)\n> num_threads = 1;\n > if (!num_threads)\n> num_threads = GREP_NUM_THREADS_DEFAULT;\n\n> and then later, instead of use_threads, do:\n\n > if (num_threads <= 1) {\n ... do single-threaded version ...\n > } else {\n ... do multi-threaded version ...\n > }\n\n > That matches the logic in builtin/pack-objects.c.\n\nMaybe use the simplest version (and keep num_numbers == 0 also as flag for all other checks in code like if(num_flags) .... ):\n\nif (list.nr || cached )\n num_threads = 0; // do not use threads\nelse if (num_threads == 0)\n num_threads = online_cpus() <= 1 ? 0 : GREP_NUM_THREADS_DEFAULT;\nelse if (num_threads < 0)\n die(...)\n\n// here we are num_threads > zero, so do nothing\n"},{"id":"273097","messageId":"20151109174738.GA29468@sigill.intra.peff.net","threadId":"40658","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA18@mail.accesssoftek.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-09T17:47:38Z","receivedAt":"2015-11-09T17:47:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 09, 2015 at 09:28:12AM -0800, Victor Leschuk wrote:\n\n> Maybe use the simplest version (and keep num_numbers == 0 also as flag for all other checks in code like if(num_flags) .... ):\n> \n> if (list.nr || cached )\n> num_threads = 0; // do not use threads\n> else if (num_threads == 0)\n> num_threads = online_cpus() <= 1 ? 0 : GREP_NUM_THREADS_DEFAULT;\n\nThat's OK.\n\n> else if (num_threads < 0)\n> die(...)\n\nDo we really want to die here? I think \"threads < 0\" works the same as\n\"threads==0\" in other git programs. It's also a weird place to die. It\nwould make:\n\n git grep --cached --threads=-1\n\nsilently work, while:\n\n git grep --threads=-1\n\nwould die.\n\nIf we do accept it, it may make sense to normalize it to 0 so that you\ncan just check \"!num_threads\" elsewhere in the code.\n\n-Peff\n"},{"id":"273100","messageId":"CA+55aFzHic5AN05QkbERFszRC=i3aDDGy9yhXEjgzZjwzFVBLQ@mail.gmail.com","threadId":"40658","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA18@mail.accesssoftek.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-11-09T17:55:34Z","receivedAt":"2015-11-09T17:55:34Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Nov 9, 2015 at 9:28 AM, Victor Leschuk\n wrote:\n>\n> Maybe use the simplest version (and keep num_numbers == 0 also as flag for all other checks in code like if(num_flags) .... ):\n>\n> if (list.nr || cached )\n> num_threads = 0; // do not use threads\n> else if (num_threads == 0)\n> num_threads = online_cpus() <= 1 ? 0 : GREP_NUM_THREADS_DEFAULT;\n\nI will say this AGAIN.\n\nThe number of threads is *not* about the number of CPU's. Stop this\ncraziness. It's wrong.\n\nThe number of threads is about parallelism. Yes, CPU's is a small part\nof it. But as mentioned earlier, the *big* wins are for slow\nfilesystems, NFS in particular. On NFS, even if you have things\ncached, the cache revalidation is going to cause network traffic\nalmost every time. Being able to have several of those outstanding is\na big deal.\n\nSo stop with the \"online_cpus()\" stuff. And don't base your benchmarks\npurely on the CPU-bound case. Because the CPU-bound case is the case\nthat is already generally so good that few people will care all *that*\ndeeply.\n\nMany of the things git does are not for \"best-case\" behavior, but to\navoid bad \"worst-case\" situations. Look at things like the index\npreloading (also threaded). The big win there is - again - when the\nstat() calls may need IO. Sure, it can help for CPU use too, but\nespecially on Linux, cached \"stat()\" calls are really quite cheap. The\nbig upside is, again, in situations like git repositories over NFS.\n\nIn the CPU-intensive case, the threading might make things go from a\ncouple of seconds to half a second. Big deal. You're not getting up to\nget a coffee in either case.\n\nIn the network traffic case, the threading might make things go from\none minute to ten seconds. And *that* is a big deal. That's huge.\nThat's \"annoyingly slow\" to \"oh, this is the fastest SCM I have ever\nworked with in my life\".\n\nThat can literally be something that changes how you work for a\ndeveloper. You start doing things that you simply would never\notherwise do.\n\nSo *none* of the threading in git is about CPU's. Maybe we should add\nbig honking comments about that.\n\nAnd that big honking comment should be in the documentation for the\nnew flag too. Because it would be really really sad if people say \"I\nhave a laptop with just two cores, so I'll set the threading option to\n2\", when they then work mostly over a slow wireless network and their\ncompany wants minimal local installs.\n\nThe biggest reason to NOT EVER add that configuration is literally the\nconfusion about this being about CPU's. That was what got the whole\nthread started, that was what the original benchmark numbers were\nabout, and that was WRONG.\n\nSo I would strongly suggest that Junio ignore these patches unless\nthey very clearly talk about the whole IO issue. Both in the source\ncode, in the commit messages, and in the documentation for the config\noption.\n\n Linus\n"},{"id":"273101","messageId":"20151109181319.GA30003@sigill.intra.peff.net","threadId":"40658","inReplyTo":"CA+55aFzHic5AN05QkbERFszRC=i3aDDGy9yhXEjgzZjwzFVBLQ@mail.gmail.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-11-09T18:13:19Z","receivedAt":"2015-11-09T18:13:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 09, 2015 at 09:55:34AM -0800, Linus Torvalds wrote:\n\n> > if (list.nr || cached )\n> > num_threads = 0; // do not use threads\n> > else if (num_threads == 0)\n> > num_threads = online_cpus() <= 1 ? 0 : GREP_NUM_THREADS_DEFAULT;\n> \n> I will say this AGAIN.\n> \n> The number of threads is *not* about the number of CPU's. Stop this\n> craziness. It's wrong.\n\nI agree with you (and that's why we have a hard-coded default here, and\nnot online_cpus()).\n\nThis check is just whether to turn the threading on or not, based on\nwhether we have multiple CPUs at all. And I agree that probably should\n_not_ be respecting online_cpus() either, because we are better off\nparallelizing even on a single CPU to avoid fs latency.\n\nBut note that this is already what the code _currently_ does. I do not\nmind changing it, but that is a separate notion from Victor's topic,\nwhich is to make the number of threads configurable at all.\n\nWhat I'd really hope to see is:\n\n 1. Victor adds --threads support, and changes _nothing else_ about the\n defaults.\n\n 2. People experiment with --threads to see what works best in a\n variety of cases (especially things like cold-cache NFS). Then we\n get patches based on real world numbers to adjust:\n\n 2a. GREP_NUM_THREADS_DEFAULT (either bumping the hard-coded\n value, or using some dynamic heuristic)\n\n 2b. What to do for the single-CPU case.\n\nWe're doing step 1 here. I don't mind jumping to 2b if we're pretty sure\nit's the right thing (and I think it probably is), but it should\ndefinitely be a separate patch.\n\n> So *none* of the threading in git is about CPU's. Maybe we should add\n> big honking comments about that.\n\nMinor nit: there definitely _are_ CPU-bound parallel tasks in git.\npack-objects and index-pack come to mind. If we add any user-visible\nadvice about tweaking thread configuration, we should be sure\nthat it is scoped to the appropriate flags.\n\n> And that big honking comment should be in the documentation for the\n> new flag too. Because it would be really really sad if people say \"I\n> have a laptop with just two cores, so I'll set the threading option to\n> 2\", when they then work mostly over a slow wireless network and their\n> company wants minimal local installs.\n\nAgreed. It would probably make sense for the \"--threads\" option\ndocumentation for each command to discuss whether it is meant to\nparallelize work across CPUs, or avoid filesystem latency.\n\n-Peff\n"},{"id":"273102","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA19@mail.accesssoftek.com","threadId":"40658","inReplyTo":"CA+55aFzHic5AN05QkbERFszRC=i3aDDGy9yhXEjgzZjwzFVBLQ@mail.gmail.com","subject":"RE: [PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-09T18:32:32Z","receivedAt":"2015-11-09T18:32:32Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"On Mon, Nov 9, 2015 at 9:28 AM, Victor Leschuk\n wrote:\n>\n> Maybe use the simplest version (and keep num_numbers == 0 also as flag for all other checks in code like if(num_flags) .... ):\n>\n> if (list.nr || cached )\n> num_threads = 0; // do not use threads\n> else if (num_threads == 0)\n> num_threads = online_cpus() <= 1 ? 0 : GREP_NUM_THREADS_DEFAULT;\n\n> I will say this AGAIN.\n\n> The number of threads is *not* about the number of CPU's. Stop this\ncraziness. It's wrong.\n\nActually I have never said the nCPUs played main role in it. The patch is intended to provide user ability to change this threads number according to their needs and to touch as small amount of other code as possible.\n\n> The number of threads is about parallelism. Yes, CPU's is a small part\n> of it. But as mentioned earlier, the *big* wins are for slow\n> filesystems, NFS in particular. On NFS, even if you have things\n> cached, the cache revalidation is going to cause network traffic\n> almost every time. Being able to have several of those outstanding is\n> a big deal.\n\n> So stop with the \"online_cpus()\" stuff. And don't base your benchmarks\n> purely on the CPU-bound case. Because the CPU-bound case is the case\n> that is already generally so good that few people will care all *that*\n> deeply.\n\nI have performed a cold-cached FS test (in previous thread to minimize the CPU part in the results) and it showed high correlation between speed and thread_num. Isn't it what you said? Even on systems with small number of cores we can gain profit of multithreading. That's I why I suggest this param to be customizable and not HARDCODED.\n\nWe need to create a clear text for the documentation that this number should not based on number of CPU-s only. Currently do not mention anything on it.\n\n--\nVictor\n"},{"id":"273103","messageId":"CAGZ79kanRZDCCoN2A+Xb=OseXc1WfF23VNV-aE7s6q1Rr4zccg@mail.gmail.com","threadId":"40658","inReplyTo":"CA+55aFzHic5AN05QkbERFszRC=i3aDDGy9yhXEjgzZjwzFVBLQ@mail.gmail.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-09T18:40:25Z","receivedAt":"2015-11-09T18:40:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 9, 2015 at 9:55 AM, Linus Torvalds\n wrote:\n>\n> So stop with the \"online_cpus()\" stuff. And don't base your benchmarks\n> purely on the CPU-bound case. Because the CPU-bound case is the case\n> that is already generally so good that few people will care all *that*\n> deeply.\n>\n> Many of the things git does are not for \"best-case\" behavior, but to\n> avoid bad \"worst-case\" situations. Look at things like the index\n> preloading (also threaded). The big win there is - again - when the\n> stat() calls may need IO. Sure, it can help for CPU use too, but\n> especially on Linux, cached \"stat()\" calls are really quite cheap. The\n> big upside is, again, in situations like git repositories over NFS.\n>\n> In the CPU-intensive case, the threading might make things go from a\n> couple of seconds to half a second. Big deal. You're not getting up to\n> get a coffee in either case.\n\nChiming in here as I have another series in flight doing parallelism.\n(Submodules done in parallel including fetching, cloning, checking out)\n\nonline_cpus() seems to be one of the easiest ballpark estimates for\nthe power of a system.\n\nSo what I would have liked to use would be some kind of\n\n parallel_expect_bottleneck(enum kinds);\n\nwith kinds being one of (FS, NETWORK, CPU, MEMORY?)\nto get an estimate 'good' number to use.\n"},{"id":"273105","messageId":"CA+55aFwV7c6=4mXPuB0c21rK3TSVWEw9JT-kiu35RuMzuHxoVg@mail.gmail.com","threadId":"40658","inReplyTo":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA19@mail.accesssoftek.com","subject":"Re: [PATCH v4] Add git-grep threads param","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-11-09T19:11:08Z","receivedAt":"2015-11-09T19:11:08Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Mon, Nov 9, 2015 at 10:32 AM, Victor Leschuk\n wrote:\n> On Mon, Nov 9, 2015 at 9:28 AM, Victor Leschuk\n>> num_threads = online_cpus() <= 1 ? 0 : GREP_NUM_THREADS_DEFAULT;\n>\n> Actually I have never said the nCPUs played main role in it. T\n\nThe pseudo-code you sent disagrees. Not that \"online_cpus() <= 1\" is\nlikely to ever be really an issue on any development platform from the\nlast decade.\n\nHowever, I do have to admit that that \"online_cpus()\" check goes back\na long time, so I guess I can't really blame you.\n\nAt least in the index preloading, I was very conscious of the IO\nissues. It doesn't actually make a big difference on traditional disks\n(seek times dominate, and concurrent IO often doesn't help at all),\nbut the reason I keep on bringing up NFS is that back when I used CVS\n(oh, the horrors), I *also* worked at a company that did everything\nover NFS. CPU ended up almost never being the limiting factor for any\nSCM operation.\n\nSo I don't have a very good idea of *what* we should use for automatic\nthread detection, but I'm pretty sure online_cpu's should not be it.\nExcept, like Jeff mentioned, for pack formation (which does tend to be\nall about CPU).\n\nSadly, detecting what kind of filesystem you are on and how well\ncached it is, is really pretty hard. Even when you have OS-specific\nknowledge, and can look up the *type* of the filesystem, what often\nmatters more is things like \"is the filesystem on a rotational media\nor using flash?\" etc.\n\nIn the meantime I'd argue for just getting rid of the online_cpu's\ncheck, because\n\n (a) I think it's actively misleading\n\n (b) the threaded grep probably doesn't hurt much even on a single\nCPU, and the _potential_ upside from IO could easily dwarf the cost.\n\n (c) do developers actually have single-core machines any more?\n\nBut if somebody can come up with a smarter model, that would certainly\nbe good too. The IO advantages really don't tend to be there for\nrotational media, but for both flash and network filesystems, threaded\nIO can be a huge deal.\n\n Linus\n"},{"id":"273108","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA1A@mail.accesssoftek.com","threadId":"40658","inReplyTo":"CA+55aFwV7c6=4mXPuB0c21rK3TSVWEw9JT-kiu35RuMzuHxoVg@mail.gmail.com","subject":"RE: [PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-09T19:52:24Z","receivedAt":"2015-11-09T19:52:24Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":".\n\n> In the meantime I'd argue for just getting rid of the online_cpu's\n> check, because\n\n> (a) I think it's actively misleading\n\n> (b) the threaded grep probably doesn't hurt much even on a single\n> CPU, and the _potential_ upside from IO could easily dwarf the cost.\n\n> (c) do developers actually have single-core machines any more?\n\n Linus\n\nSo as far as I understood your point at current moment would be better to leave online_cpus() check behind,\nkeep the default threads value (and do so for other threaded programs, in separate patches of course).\n\nAfter that we may focus (in future) on developing smarter heuristics for parallelity-relationed params.\n\nAlso we should specify in documentation that number of your CPUs may not the optimal value and customer should find his own best values based on other circumstances.\n\nCorrect?\n"},{"id":"273113","messageId":"6AE1604EE3EC5F4296C096518C6B77EE5D0FDABA1B@mail.accesssoftek.com","threadId":"40658","inReplyTo":"CA+55aFyhBN5fEpB-CLQFhhDyf7nijs_Y3aZCSQAcpVPmruZLFg@mail.gmail.com","subject":"RE: [PATCH v4] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@accesssoftek.com","sentAt":"2015-11-09T21:51:48Z","receivedAt":"2015-11-09T21:51:48Z","isPatch":true,"sender":{"key":"vleschuk@accesssoftek.com","avatar":null},"body":"\nCorrect.\n\nI think adding the option (both to command line and to config file) is good, as long as the IO issues are documented. And default to just the fixed number of threads for now - and with the option, maybe people can then more easily try out different scenarios and maybe improve on the particular choice of fixed number of threads.\n\n> Or maybe the default should be something that isn't even described by any particular fixed number.\n\n> For example, maybe the default that value could be something quite dynamic: start off with a single thread (or a very low thread number) and just set a timer. After 0.1s, if CPU usage is low, start more threads. After another 0.1s, if that improved things, maybe > we could add still more threads...\n\n> Note that \"CPU usage is low\" can be hard to get portably, but we could approximate it with \"how much work did we actually get done\". If we only grepped a couple of files, that might be because of IO issues. And if speed does not improve when we move from a \n> single thread to, say, four threads, then we should probably *not* increase the thread number again at 0.2s.\n\n> So I think there are many possible avenues to explore that might be interesting. I do *not* think that \"online_cpus()\" is one of them, > except perhaps as a very rough measure of \"is this a beefy system or not\" (but even that is questionable - 32 CPUs is definitely \n> likely \"very beefy, so use lots of threads\", but even 8 CPUs might still be just a phone, and I'm not sure that tells you a lot, really.\n Linus\n\nHere is my version of note for Documentaion:\n\n Number of grep worker threads, use it to tune up performance on\n your machines. Leave it unset or set to \"0\" if you want to use default number\n (currently default number is 8 for all systems, however this behavior can\n be changed in future versions to better suite your hardware and circumstances).\n\n\n--\nVictor\n"}]}