{"thread":{"id":"40761","subject":"[PATCH v5] Add git-grep threads param","startedAt":"2015-11-10T13:28:38Z","lastAt":"2015-11-10T20:55:35Z","messageCount":2,"participants":["Victor Leschuk","Eric Sunshine"],"isPatch":true,"patchVersion":5,"patchTotal":null},"messages":[{"id":"273149","messageId":"1447162118-17636-1-git-send-email-vleschuk@accesssoftek.com","threadId":"40761","inReplyTo":null,"subject":"[PATCH v5] Add git-grep threads param","fromName":"Victor Leschuk","fromEmail":"vleschuk@gmail.com","sentAt":"2015-11-10T13:28:38Z","receivedAt":"2015-11-10T13:28:38Z","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               |  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..467fa7b 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 suite 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..91027b6 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 for now is using 8 threads for all systems.\n+\tDefault behavior can be changed in future versions\n+\tto better suite 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 <num>::\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 suite hardware and circumstances.\n+\n -f <file>::\n \tRead patterns from <file>, 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 <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@@ -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":"273160","messageId":"CAPig+cQXCHzAJ80ZEHcM_+7J7jRs1obwuSzqe7sRDnuocMYUbw@mail.gmail.com","threadId":"40761","inReplyTo":"1447162118-17636-1-git-send-email-vleschuk@accesssoftek.com","subject":"Re: [PATCH v5] Add git-grep threads param","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-11-10T20:55:35Z","receivedAt":"2015-11-10T20:55:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Nov 10, 2015 at 8:28 AM, Victor Leschuk <vleschuk@gmail.com> 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> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>\n> ---\n\nAs an aid for reviewers, please use this space (below the \"---\" line)\nto describe what changed since the previous version. It's also helpful\nif you can provide a link to the previous version in the mailing list\narchive.\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>\n> diff --git a/Documentation/config.txt 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 suite hardware and circumstances.\n\ns/suite/suit/ (here and elsewhere)\n\nWas the conclusion of discussion that there should be some explanation\nhere that this is more about tuning for I/O rather than CPU, or did I\nmisunderstand?\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\n> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt\n> index 4a44d6d..91027b6 100644\n> --- a/Documentation/git-grep.txt\n> +++ b/Documentation/git-grep.txt\n> diff --git a/builtin/grep.c b/builtin/grep.c\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\nThe original code consulted online_cpus(), but the new code does not.\nThis is a sufficiently significant change that it deserves mention in\nthe commit message. In fact, it's really a distinct change which might\ndeserve being done in its own preparatory patch (with an explanation\nsomething along the lines of \"even single-core machines may benefit\nfrom threading when the bottleneck is I/O\").\n\n>  #else\n> -       use_threads = 0;\n> +       num_threads = 0;\n>  #endif\n"}]}