[PATCH v6] Add git-grep threads param

[PATCH v6] Add git-grep threads param

[Victor Leschuk]

"git grep" can now be configured (or told from the command line)
 how many threads to use when searching in the working tree files.

 Changes to default behavior: number of threads now doesn't depend
 on online_cpus(), e.g. if specific number is not configured
 GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU. 

Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
---
History of changes from the first version ($gmane/280053/:
	* Param renamed from threads-num to threads
	* Short version of '--threads' cmd key was removed
	* Made num_threads 'decision-tree' more obvious 
	  and easy to edit for future use ($gmane/280089)
	* Moved option description to more suitable place in documentation ($gmane/280188)
	* Hid threads param from 'external' grep.c, made it private for builtin/grep.c ($gmane/280188)
	* Improved num_threads 'decision-tree', got rid of dependency on online_cpus ($gmane/280299)
	* Improved param documentation ($gmane/280299)


 Documentation/config.txt               |  7 +++++
 Documentation/git-grep.txt             | 15 ++++++++++
 builtin/grep.c                         | 50 +++++++++++++++++++++++-----------
 contrib/completion/git-completion.bash |  1 +
 4 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..5084e36 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1447,6 +1447,13 @@ grep.extendedRegexp::
 	option is ignored when the 'grep.patternType' option is set to a value
 	other than 'default'.
 
+grep.threads::
+	Number of grep worker threads, use it to tune up performance on
+	your machines. Leave it unset (or set to 0) for default behavior,
+	which for now is using 8 threads for all systems.
+	Default behavior can be changed in future versions
+	to better suit hardware and circumstances.
+
 gpg.program::
 	Use this custom program instead of "gpg" found on $PATH when
 	making or verifying a PGP signature. The program must support the
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4a44d6d..8222a83 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -23,6 +23,7 @@ SYNOPSIS
 	   [--break] [--heading] [-p | --show-function]
 	   [-A <post-context>] [-B <pre-context>] [-C <context>]
 	   [-W | --function-context]
+	   [--threads <num>]
 	   [-f <file>] [-e] <pattern>
 	   [--and|--or|--not|(|)|-e <pattern>...]
 	   [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
@@ -53,6 +54,13 @@ grep.extendedRegexp::
 	option is ignored when the 'grep.patternType' option is set to a value
 	other than 'default'.
 
+grep.threads::
+	Number of grep worker threads, use it to tune up performance on
+	your machines. Leave it unset (or set to 0) for default behavior,
+	which for now is using 8 threads for all systems.
+	Default behavior can be changed in future versions
+	to better suit hardware and circumstances.
+
 grep.fullName::
 	If set to true, enable '--full-name' option by default.
 
@@ -227,6 +235,13 @@ OPTIONS
 	effectively showing the whole function in which the match was
 	found.
 
+--threads <num>::
+	Number of grep worker threads, use it to tune up performance on
+	your machines. Leave it unset (or set to 0) for default behavior,
+	which for now is using 8 threads for all systems.
+	Default behavior can be changed in future versions
+	to better suit hardware and circumstances.
+
 -f <file>::
 	Read patterns from <file>, one per line.
 
diff --git a/builtin/grep.c b/builtin/grep.c
index d04f440..f0e3dfb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -24,11 +24,11 @@ static char const * const grep_usage[] = {
 	NULL
 };
 
-static int use_threads = 1;
+#define GREP_NUM_THREADS_DEFAULT 8
+static int num_threads = 0;
 
 #ifndef NO_PTHREADS
-#define THREADS 8
-static pthread_t threads[THREADS];
+static pthread_t *threads;
 
 /* We use one producer thread and THREADS consumer
  * threads. The producer adds struct work_items to 'todo' and the
@@ -63,13 +63,13 @@ static pthread_mutex_t grep_mutex;
 
 static inline void grep_lock(void)
 {
-	if (use_threads)
+	if (num_threads)
 		pthread_mutex_lock(&grep_mutex);
 }
 
 static inline void grep_unlock(void)
 {
-	if (use_threads)
+	if (num_threads)
 		pthread_mutex_unlock(&grep_mutex);
 }
 
@@ -206,7 +206,8 @@ static void start_threads(struct grep_opt *opt)
 		strbuf_init(&todo[i].out, 0);
 	}
 
-	for (i = 0; i < ARRAY_SIZE(threads); i++) {
+	threads = xcalloc(num_threads, sizeof(pthread_t));
+	for (i = 0; i < num_threads; i++) {
 		int err;
 		struct grep_opt *o = grep_opt_dup(opt);
 		o->output = strbuf_out;
@@ -238,12 +239,14 @@ static int wait_all(void)
 	pthread_cond_broadcast(&cond_add);
 	grep_unlock();
 
-	for (i = 0; i < ARRAY_SIZE(threads); i++) {
+	for (i = 0; i < num_threads; i++) {
 		void *h;
 		pthread_join(threads[i], &h);
 		hit |= (int) (intptr_t) h;
 	}
 
+	free(threads);
+
 	pthread_mutex_destroy(&grep_mutex);
 	pthread_mutex_destroy(&grep_read_mutex);
 	pthread_mutex_destroy(&grep_attr_mutex);
@@ -262,10 +265,19 @@ static int wait_all(void)
 }
 #endif
 
+static int grep_threads_config(const char *var, const char *value, void *cb)
+{
+	if (!strcmp(var, "grep.threads"))
+		num_threads = git_config_int(var, value); /* Sanity check of value will be perfomed later */
+	return 0;
+}
+
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
 	int st = grep_config(var, value, cb);
-	if (git_color_default_config(var, value, cb) < 0)
+	if (grep_threads_config(var, value, cb) < 0)
+		st = -1;
+	else if (git_color_default_config(var, value, cb) < 0)
 		st = -1;
 	return st;
 }
@@ -294,7 +306,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
 	}
 
 #ifndef NO_PTHREADS
-	if (use_threads) {
+	if (num_threads) {
 		add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);
 		strbuf_release(&pathbuf);
 		return 0;
@@ -323,7 +335,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
 		strbuf_addstr(&buf, filename);
 
 #ifndef NO_PTHREADS
-	if (use_threads) {
+	if (num_threads) {
 		add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);
 		strbuf_release(&buf);
 		return 0;
@@ -702,6 +714,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			N_("show <n> context lines before matches")),
 		OPT_INTEGER('A', "after-context", &opt.post_context,
 			N_("show <n> context lines after matches")),
+		OPT_INTEGER(0, "threads", &num_threads,
+			N_("use <n> worker threads")),
 		OPT_NUMBER_CALLBACK(&opt, N_("shortcut for -C NUM"),
 			context_callback),
 		OPT_BOOL('p', "show-function", &opt.funcname,
@@ -801,7 +815,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		opt.output_priv = &path_list;
 		opt.output = append_path;
 		string_list_append(&path_list, show_in_pager);
-		use_threads = 0;
+		num_threads = 0;
 	}
 
 	if (!opt.pattern_list)
@@ -832,14 +846,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	}
 
 #ifndef NO_PTHREADS
-	if (list.nr || cached || online_cpus() == 1)
-		use_threads = 0;
+	if (list.nr || cached)
+		num_threads = 0; /* Can not multi-thread object lookup */
+	else if (num_threads == 0)
+		num_threads = GREP_NUM_THREADS_DEFAULT; /* User didn't specify value, or just wants default behavior */
+	else if (num_threads < 0)
+		die("Invalid number of threads specified (%d)", num_threads);
 #else
-	use_threads = 0;
+	num_threads = 0;
 #endif
 
 #ifndef NO_PTHREADS
-	if (use_threads) {
+	if (num_threads) {
 		if (!(opt.name_only || opt.unmatch_name_only || opt.count)
 		    && (opt.pre_context || opt.post_context ||
 			opt.file_break || opt.funcbody))
@@ -909,7 +927,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		hit = grep_objects(&opt, &pathspec, &list);
 	}
 
-	if (use_threads)
+	if (num_threads)
 		hit |= wait_all();
 	if (hit && show_in_pager)
 		run_pager(&opt, prefix);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 482ca84..390d9c0 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1310,6 +1310,7 @@ _git_grep ()
 			--full-name --line-number
 			--extended-regexp --basic-regexp --fixed-strings
 			--perl-regexp
+			--threads
 			--files-with-matches --name-only
 			--files-without-match
 			--max-depth
-- 
2.6.3.369.g3e7f205.dirty

RE: [PATCH v6] Add git-grep threads param

[Victor Leschuk]

Hello all,

The 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?

Thanks.
--
Best Regards,
Victor
________________________________________
From: Victor Leschuk [vleschuk@gmail.com]
Sent: Wednesday, November 11, 2015 03:52
To: git@vger.kernel.org
Cc: Victor Leschuk
Subject: [PATCH v6] Add git-grep threads param

"git grep" can now be configured (or told from the command line)
 how many threads to use when searching in the working tree files.

 Changes to default behavior: number of threads now doesn't depend
 on online_cpus(), e.g. if specific number is not configured
 GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU.

Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
---
History of changes from the first version ($gmane/280053/:
        * Param renamed from threads-num to threads
        * Short version of '--threads' cmd key was removed
        * Made num_threads 'decision-tree' more obvious
          and easy to edit for future use ($gmane/280089)
        * Moved option description to more suitable place in documentation ($gmane/280188)
        * Hid threads param from 'external' grep.c, made it private for builtin/grep.c ($gmane/280188)
        * Improved num_threads 'decision-tree', got rid of dependency on online_cpus ($gmane/280299)
        * Improved param documentation ($gmane/280299)


 Documentation/config.txt               |  7 +++++
 Documentation/git-grep.txt             | 15 ++++++++++
 builtin/grep.c                         | 50 +++++++++++++++++++++++-----------
 contrib/completion/git-completion.bash |  1 +
 4 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..5084e36 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1447,6 +1447,13 @@ grep.extendedRegexp::
        option is ignored when the 'grep.patternType' option is set to a value
        other than 'default'.

+grep.threads::
+       Number of grep worker threads, use it to tune up performance on
+       your machines. Leave it unset (or set to 0) for default behavior,
+       which for now is using 8 threads for all systems.
+       Default behavior can be changed in future versions
+       to better suit hardware and circumstances.
+
 gpg.program::
        Use this custom program instead of "gpg" found on $PATH when
        making or verifying a PGP signature. The program must support the
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4a44d6d..8222a83 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -23,6 +23,7 @@ SYNOPSIS
           [--break] [--heading] [-p | --show-function]
           [-A <post-context>] [-B <pre-context>] [-C <context>]
           [-W | --function-context]
+          [--threads <num>]
           [-f <file>] [-e] <pattern>
           [--and|--or|--not|(|)|-e <pattern>...]
           [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
@@ -53,6 +54,13 @@ grep.extendedRegexp::
        option is ignored when the 'grep.patternType' option is set to a value
        other than 'default'.

+grep.threads::
+       Number of grep worker threads, use it to tune up performance on
+       your machines. Leave it unset (or set to 0) for default behavior,
+       which for now is using 8 threads for all systems.
+       Default behavior can be changed in future versions
+       to better suit hardware and circumstances.
+
 grep.fullName::
        If set to true, enable '--full-name' option by default.

@@ -227,6 +235,13 @@ OPTIONS
        effectively showing the whole function in which the match was
        found.

+--threads <num>::
+       Number of grep worker threads, use it to tune up performance on
+       your machines. Leave it unset (or set to 0) for default behavior,
+       which for now is using 8 threads for all systems.
+       Default behavior can be changed in future versions
+       to better suit hardware and circumstances.
+
 -f <file>::
        Read patterns from <file>, one per line.

diff --git a/builtin/grep.c b/builtin/grep.c
index d04f440..f0e3dfb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -24,11 +24,11 @@ static char const * const grep_usage[] = {
        NULL
 };

-static int use_threads = 1;
+#define GREP_NUM_THREADS_DEFAULT 8
+static int num_threads = 0;

 #ifndef NO_PTHREADS
-#define THREADS 8
-static pthread_t threads[THREADS];
+static pthread_t *threads;

 /* We use one producer thread and THREADS consumer
  * threads. The producer adds struct work_items to 'todo' and the
@@ -63,13 +63,13 @@ static pthread_mutex_t grep_mutex;

 static inline void grep_lock(void)
 {
-       if (use_threads)
+       if (num_threads)
                pthread_mutex_lock(&grep_mutex);
 }

 static inline void grep_unlock(void)
 {
-       if (use_threads)
+       if (num_threads)
                pthread_mutex_unlock(&grep_mutex);
 }

@@ -206,7 +206,8 @@ static void start_threads(struct grep_opt *opt)
                strbuf_init(&todo[i].out, 0);
        }

-       for (i = 0; i < ARRAY_SIZE(threads); i++) {
+       threads = xcalloc(num_threads, sizeof(pthread_t));
+       for (i = 0; i < num_threads; i++) {
                int err;
                struct grep_opt *o = grep_opt_dup(opt);
                o->output = strbuf_out;
@@ -238,12 +239,14 @@ static int wait_all(void)
        pthread_cond_broadcast(&cond_add);
        grep_unlock();

-       for (i = 0; i < ARRAY_SIZE(threads); i++) {
+       for (i = 0; i < num_threads; i++) {
                void *h;
                pthread_join(threads[i], &h);
                hit |= (int) (intptr_t) h;
        }

+       free(threads);
+
        pthread_mutex_destroy(&grep_mutex);
        pthread_mutex_destroy(&grep_read_mutex);
        pthread_mutex_destroy(&grep_attr_mutex);
@@ -262,10 +265,19 @@ static int wait_all(void)
 }
 #endif

+static int grep_threads_config(const char *var, const char *value, void *cb)
+{
+       if (!strcmp(var, "grep.threads"))
+               num_threads = git_config_int(var, value); /* Sanity check of value will be perfomed later */
+       return 0;
+}
+
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
        int st = grep_config(var, value, cb);
-       if (git_color_default_config(var, value, cb) < 0)
+       if (grep_threads_config(var, value, cb) < 0)
+               st = -1;
+       else if (git_color_default_config(var, value, cb) < 0)
                st = -1;
        return st;
 }
@@ -294,7 +306,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
        }

 #ifndef NO_PTHREADS
-       if (use_threads) {
+       if (num_threads) {
                add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);
                strbuf_release(&pathbuf);
                return 0;
@@ -323,7 +335,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
                strbuf_addstr(&buf, filename);

 #ifndef NO_PTHREADS
-       if (use_threads) {
+       if (num_threads) {
                add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);
                strbuf_release(&buf);
                return 0;
@@ -702,6 +714,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
                        N_("show <n> context lines before matches")),
                OPT_INTEGER('A', "after-context", &opt.post_context,
                        N_("show <n> context lines after matches")),
+               OPT_INTEGER(0, "threads", &num_threads,
+                       N_("use <n> worker threads")),
                OPT_NUMBER_CALLBACK(&opt, N_("shortcut for -C NUM"),
                        context_callback),
                OPT_BOOL('p', "show-function", &opt.funcname,
@@ -801,7 +815,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
                opt.output_priv = &path_list;
                opt.output = append_path;
                string_list_append(&path_list, show_in_pager);
-               use_threads = 0;
+               num_threads = 0;
        }

        if (!opt.pattern_list)
@@ -832,14 +846,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
        }

 #ifndef NO_PTHREADS
-       if (list.nr || cached || online_cpus() == 1)
-               use_threads = 0;
+       if (list.nr || cached)
+               num_threads = 0; /* Can not multi-thread object lookup */
+       else if (num_threads == 0)
+               num_threads = GREP_NUM_THREADS_DEFAULT; /* User didn't specify value, or just wants default behavior */
+       else if (num_threads < 0)
+               die("Invalid number of threads specified (%d)", num_threads);
 #else
-       use_threads = 0;
+       num_threads = 0;
 #endif

 #ifndef NO_PTHREADS
-       if (use_threads) {
+       if (num_threads) {
                if (!(opt.name_only || opt.unmatch_name_only || opt.count)
                    && (opt.pre_context || opt.post_context ||
                        opt.file_break || opt.funcbody))
@@ -909,7 +927,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
                hit = grep_objects(&opt, &pathspec, &list);
        }

-       if (use_threads)
+       if (num_threads)
                hit |= wait_all();
        if (hit && show_in_pager)
                run_pager(&opt, prefix);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 482ca84..390d9c0 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1310,6 +1310,7 @@ _git_grep ()
                        --full-name --line-number
                        --extended-regexp --basic-regexp --fixed-strings
                        --perl-regexp
+                       --threads
                        --files-with-matches --name-only
                        --files-without-match
                        --max-depth
--
2.6.3.369.g3e7f205.dirty

Re: [PATCH v6] Add git-grep threads param

[Jeff King]

On Mon, Nov 16, 2015 at 05:11:16AM -0800, Victor Leschuk wrote:

> The 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?

Merging to "pu" does not really mean anything; it is simply that the
maintainer has picked it up as a possible topic of interest. Patches can
(and often are) still re-written in that state.

Junio is on vacation for a few weeks, and I'm acting as maintainer in
the interim. I've added your v6 to my pile of patches to look at, but I
haven't gone over it carefully yet.

-Peff

Re: [PATCH v6] Add git-grep threads param

[Eric Sunshine]

On Mon, Nov 16, 2015 at 8:56 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Nov 16, 2015 at 05:11:16AM -0800, Victor Leschuk wrote:
>
>> The 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?
>
> Merging to "pu" does not really mean anything; it is simply that the
> maintainer has picked it up as a possible topic of interest. Patches can
> (and often are) still re-written in that state.
>
> Junio is on vacation for a few weeks, and I'm acting as maintainer in
> the interim. I've added your v6 to my pile of patches to look at, but I
> haven't gone over it carefully yet.

To be perfectly explicit: Patches in 'pu' get replaced wholesale by
newer versions, so you would simply send the new version of the patch
in its entirety (as you did with v6).

RE: [PATCH v6] Add git-grep threads param

[Victor Leschuk]

Hello all, 

does anybody have time to review this patch?

PS Sorry for bothering =)

--
Best Regards,
Victor
________________________________________
From: Victor Leschuk [vleschuk@gmail.com]
Sent: Wednesday, November 11, 2015 03:52
To: git@vger.kernel.org
Cc: Victor Leschuk
Subject: [PATCH v6] Add git-grep threads param

"git grep" can now be configured (or told from the command line)
 how many threads to use when searching in the working tree files.

 Changes to default behavior: number of threads now doesn't depend
 on online_cpus(), e.g. if specific number is not configured
 GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU.

Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
---
History of changes from the first version ($gmane/280053/:
        * Param renamed from threads-num to threads
        * Short version of '--threads' cmd key was removed
        * Made num_threads 'decision-tree' more obvious
          and easy to edit for future use ($gmane/280089)
        * Moved option description to more suitable place in documentation ($gmane/280188)
        * Hid threads param from 'external' grep.c, made it private for builtin/grep.c ($gmane/280188)
        * Improved num_threads 'decision-tree', got rid of dependency on online_cpus ($gmane/280299)
        * Improved param documentation ($gmane/280299)


 Documentation/config.txt               |  7 +++++
 Documentation/git-grep.txt             | 15 ++++++++++
 builtin/grep.c                         | 50 +++++++++++++++++++++++-----------
 contrib/completion/git-completion.bash |  1 +
 4 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..5084e36 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1447,6 +1447,13 @@ grep.extendedRegexp::
        option is ignored when the 'grep.patternType' option is set to a value
        other than 'default'.

+grep.threads::
+       Number of grep worker threads, use it to tune up performance on
+       your machines. Leave it unset (or set to 0) for default behavior,
+       which for now is using 8 threads for all systems.
+       Default behavior can be changed in future versions
+       to better suit hardware and circumstances.
+
 gpg.program::
        Use this custom program instead of "gpg" found on $PATH when
        making or verifying a PGP signature. The program must support the
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 4a44d6d..8222a83 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -23,6 +23,7 @@ SYNOPSIS
           [--break] [--heading] [-p | --show-function]
           [-A <post-context>] [-B <pre-context>] [-C <context>]
           [-W | --function-context]
+          [--threads <num>]
           [-f <file>] [-e] <pattern>
           [--and|--or|--not|(|)|-e <pattern>...]
           [ [--[no-]exclude-standard] [--cached | --no-index | --untracked] | <tree>...]
@@ -53,6 +54,13 @@ grep.extendedRegexp::
        option is ignored when the 'grep.patternType' option is set to a value
        other than 'default'.

+grep.threads::
+       Number of grep worker threads, use it to tune up performance on
+       your machines. Leave it unset (or set to 0) for default behavior,
+       which for now is using 8 threads for all systems.
+       Default behavior can be changed in future versions
+       to better suit hardware and circumstances.
+
 grep.fullName::
        If set to true, enable '--full-name' option by default.

@@ -227,6 +235,13 @@ OPTIONS
        effectively showing the whole function in which the match was
        found.

+--threads <num>::
+       Number of grep worker threads, use it to tune up performance on
+       your machines. Leave it unset (or set to 0) for default behavior,
+       which for now is using 8 threads for all systems.
+       Default behavior can be changed in future versions
+       to better suit hardware and circumstances.
+
 -f <file>::
        Read patterns from <file>, one per line.

diff --git a/builtin/grep.c b/builtin/grep.c
index d04f440..f0e3dfb 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -24,11 +24,11 @@ static char const * const grep_usage[] = {
        NULL
 };

-static int use_threads = 1;
+#define GREP_NUM_THREADS_DEFAULT 8
+static int num_threads = 0;

 #ifndef NO_PTHREADS
-#define THREADS 8
-static pthread_t threads[THREADS];
+static pthread_t *threads;

 /* We use one producer thread and THREADS consumer
  * threads. The producer adds struct work_items to 'todo' and the
@@ -63,13 +63,13 @@ static pthread_mutex_t grep_mutex;

 static inline void grep_lock(void)
 {
-       if (use_threads)
+       if (num_threads)
                pthread_mutex_lock(&grep_mutex);
 }

 static inline void grep_unlock(void)
 {
-       if (use_threads)
+       if (num_threads)
                pthread_mutex_unlock(&grep_mutex);
 }

@@ -206,7 +206,8 @@ static void start_threads(struct grep_opt *opt)
                strbuf_init(&todo[i].out, 0);
        }

-       for (i = 0; i < ARRAY_SIZE(threads); i++) {
+       threads = xcalloc(num_threads, sizeof(pthread_t));
+       for (i = 0; i < num_threads; i++) {
                int err;
                struct grep_opt *o = grep_opt_dup(opt);
                o->output = strbuf_out;
@@ -238,12 +239,14 @@ static int wait_all(void)
        pthread_cond_broadcast(&cond_add);
        grep_unlock();

-       for (i = 0; i < ARRAY_SIZE(threads); i++) {
+       for (i = 0; i < num_threads; i++) {
                void *h;
                pthread_join(threads[i], &h);
                hit |= (int) (intptr_t) h;
        }

+       free(threads);
+
        pthread_mutex_destroy(&grep_mutex);
        pthread_mutex_destroy(&grep_read_mutex);
        pthread_mutex_destroy(&grep_attr_mutex);
@@ -262,10 +265,19 @@ static int wait_all(void)
 }
 #endif

+static int grep_threads_config(const char *var, const char *value, void *cb)
+{
+       if (!strcmp(var, "grep.threads"))
+               num_threads = git_config_int(var, value); /* Sanity check of value will be perfomed later */
+       return 0;
+}
+
 static int grep_cmd_config(const char *var, const char *value, void *cb)
 {
        int st = grep_config(var, value, cb);
-       if (git_color_default_config(var, value, cb) < 0)
+       if (grep_threads_config(var, value, cb) < 0)
+               st = -1;
+       else if (git_color_default_config(var, value, cb) < 0)
                st = -1;
        return st;
 }
@@ -294,7 +306,7 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
        }

 #ifndef NO_PTHREADS
-       if (use_threads) {
+       if (num_threads) {
                add_work(opt, GREP_SOURCE_SHA1, pathbuf.buf, path, sha1);
                strbuf_release(&pathbuf);
                return 0;
@@ -323,7 +335,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)
                strbuf_addstr(&buf, filename);

 #ifndef NO_PTHREADS
-       if (use_threads) {
+       if (num_threads) {
                add_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);
                strbuf_release(&buf);
                return 0;
@@ -702,6 +714,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
                        N_("show <n> context lines before matches")),
                OPT_INTEGER('A', "after-context", &opt.post_context,
                        N_("show <n> context lines after matches")),
+               OPT_INTEGER(0, "threads", &num_threads,
+                       N_("use <n> worker threads")),
                OPT_NUMBER_CALLBACK(&opt, N_("shortcut for -C NUM"),
                        context_callback),
                OPT_BOOL('p', "show-function", &opt.funcname,
@@ -801,7 +815,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
                opt.output_priv = &path_list;
                opt.output = append_path;
                string_list_append(&path_list, show_in_pager);
-               use_threads = 0;
+               num_threads = 0;
        }

        if (!opt.pattern_list)
@@ -832,14 +846,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
        }

 #ifndef NO_PTHREADS
-       if (list.nr || cached || online_cpus() == 1)
-               use_threads = 0;
+       if (list.nr || cached)
+               num_threads = 0; /* Can not multi-thread object lookup */
+       else if (num_threads == 0)
+               num_threads = GREP_NUM_THREADS_DEFAULT; /* User didn't specify value, or just wants default behavior */
+       else if (num_threads < 0)
+               die("Invalid number of threads specified (%d)", num_threads);
 #else
-       use_threads = 0;
+       num_threads = 0;
 #endif

 #ifndef NO_PTHREADS
-       if (use_threads) {
+       if (num_threads) {
                if (!(opt.name_only || opt.unmatch_name_only || opt.count)
                    && (opt.pre_context || opt.post_context ||
                        opt.file_break || opt.funcbody))
@@ -909,7 +927,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
                hit = grep_objects(&opt, &pathspec, &list);
        }

-       if (use_threads)
+       if (num_threads)
                hit |= wait_all();
        if (hit && show_in_pager)
                run_pager(&opt, prefix);
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 482ca84..390d9c0 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1310,6 +1310,7 @@ _git_grep ()
                        --full-name --line-number
                        --extended-regexp --basic-regexp --fixed-strings
                        --perl-regexp
+                       --threads
                        --files-with-matches --name-only
                        --files-without-match
                        --max-depth
--
2.6.3.369.g3e7f205.dirty

Re: [PATCH v6] Add git-grep threads param

[Duy Nguyen]

On Wed, Nov 11, 2015 at 12:52 PM, Victor Leschuk <vleschuk@gmail.com> wrote:
> "git grep" can now be configured (or told from the command line)
>  how many threads to use when searching in the working tree files.
>
>  Changes to default behavior: number of threads now doesn't depend
>  on online_cpus(), e.g. if specific number is not configured
>  GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU.

Why? (I'm asking for an explanation in the commit message so that I
will not have to ask again in future)

> @@ -206,7 +206,8 @@ static void start_threads(struct grep_opt *opt)
>                 strbuf_init(&todo[i].out, 0);
>         }
>
> -       for (i = 0; i < ARRAY_SIZE(threads); i++) {
> +       threads = xcalloc(num_threads, sizeof(pthread_t));

I think we usually go with sizeof(*threads), but not sure if it's just
a personal taste or the preferred style for git.

>  static int grep_cmd_config(const char *var, const char *value, void *cb)
>  {
>         int st = grep_config(var, value, cb);
> -       if (git_color_default_config(var, value, cb) < 0)
> +       if (grep_threads_config(var, value, cb) < 0)
> +               st = -1;
> +       else if (git_color_default_config(var, value, cb) < 0)
>                 st = -1;
>         return st;

Hm... isn't it simpler to just return -1 instead of assigning to st
first? I think you could just merge grep_threads_config() in this
function because it's not that complex to stay separate..

>  }
-- 
Duy

Re: [PATCH v6] Add git-grep threads param

[Duy Nguyen]

I forgot..

On Wed, Nov 11, 2015 at 12:52 PM, Victor Leschuk <vleschuk@gmail.com> wrote:
> +       else if (num_threads < 0)
> +               die("Invalid number of threads specified (%d)", num_threads);

Please wrap this string with _() so it can be translated
-- 
Duy

Re: [PATCH v6] Add git-grep threads param

[Junio C Hamano]

This version seems to break t7811 when applied on top of 37023ba3
(Seventh batch for 2.7, 2015-10-26).

I'll eject it from 'pu' for today's integration.

Re: [PATCH v6] Add git-grep threads param

[Eric Sunshine]

On Mon, Nov 16, 2015 at 8:11 AM, Victor Leschuk
<vleschuk@accesssoftek.com> wrote:
> "git grep" can now be configured (or told from the command line)
>  how many threads to use when searching in the working tree files.
>
>  Changes to default behavior: number of threads now doesn't depend
>  on online_cpus(), e.g. if specific number is not configured
>  GREP_NUM_THREADS_DEFAULT (8) threads will be used even on 1-core CPU.

It's good that this behavior change (no longer consulting
online_cpus()) is now mentioned[1] in the commit message, however,
it's also important for people to understand why this change was made,
but such explanation is missing here. It's important to justify the
change in the commit message itself since only a few people were
involved in the mailing list discussion which led to the change, and
even those people may forget the reasoning at some point.

Moreover, this change (dropping online_cpus()) is sufficiently
significant and distinct from the overall purpose of the present patch
(adding --threads and 'grep.threads') that it really deserves its own
separate patch (as hinted by [1]), which would turn this into a
2-patch series.

[1]: http://article.gmane.org/gmane.comp.version-control.git/281133/

> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1447,6 +1447,13 @@ grep.extendedRegexp::
> +grep.threads::
> +       Number of grep worker threads, use it to tune up performance on
> +       your machines. Leave it unset (or set to 0) for default behavior,
> +       which for now is using 8 threads for all systems.

You could drop "for now", which is redundant with the sentence which follows.

> +       Default behavior can be changed in future versions
> +       to better suit hardware and circumstances.

"may change" might read better than "can be changed".

>  gpg.program::
>         Use this custom program instead of "gpg" found on $PATH when
>         making or verifying a PGP signature. The program must support the
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> @@ -53,6 +54,13 @@ grep.extendedRegexp::
> +grep.threads::
> +       Number of grep worker threads, use it to tune up performance on
> +       your machines. Leave it unset (or set to 0) for default behavior,
> +       which for now is using 8 threads for all systems.
> +       Default behavior can be changed in future versions
> +       to better suit hardware and circumstances.

Same comments as above.

> @@ -227,6 +235,13 @@ OPTIONS
> +--threads <num>::
> +       Number of grep worker threads, use it to tune up performance on
> +       your machines. Leave it unset (or set to 0) for default behavior,
> +       which for now is using 8 threads for all systems.
> +       Default behavior can be changed in future versions
> +       to better suit hardware and circumstances.

Ditto.

Would it make more sense just to have the detailed description in one
place and then reference it from the other places rather than
repeating in all places?

> diff --git a/builtin/grep.c b/builtin/grep.c
> @@ -262,10 +265,19 @@ static int wait_all(void)
> +static int grep_threads_config(const char *var, const char *value, void *cb)
> +{
> +       if (!strcmp(var, "grep.threads"))
> +               num_threads = git_config_int(var, value); /* Sanity check of value will be perfomed later */

Nit: This is a pretty wide line. Perhaps the comment could be moved to
its own line to stay below 80 columns?

> +       return 0;
> +}
> +
>  static int grep_cmd_config(const char *var, const char *value, void *cb)
>  {
>         int st = grep_config(var, value, cb);
> -       if (git_color_default_config(var, value, cb) < 0)
> +       if (grep_threads_config(var, value, cb) < 0)
> +               st = -1;
> +       else if (git_color_default_config(var, value, cb) < 0)
>                 st = -1;

Nit: Custom is to add new cases following existing ones, which would
give you a less noisy diff.

>         return st;
>  }
> @@ -832,14 +846,18 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>         }
>
>  #ifndef NO_PTHREADS
> -       if (list.nr || cached || online_cpus() == 1)
> -               use_threads = 0;
> +       if (list.nr || cached)
> +               num_threads = 0; /* Can not multi-thread object lookup */
> +       else if (num_threads == 0)
> +               num_threads = GREP_NUM_THREADS_DEFAULT; /* User didn't specify value, or just wants default behavior */

Nit: Overly wide line.

> +       else if (num_threads < 0)
> +               die("Invalid number of threads specified (%d)", num_threads);

If you validate the value earlier, rather than delaying it until here,
you can give a more useful error message which would state whence the
bad value arose (config vs. command-line). This may or may not be a
worthwhile improvement.

>  #else
> -       use_threads = 0;
> +       num_threads = 0;
>  #endif
>
>  #ifndef NO_PTHREADS
> -       if (use_threads) {
> +       if (num_threads) {
>                 if (!(opt.name_only || opt.unmatch_name_only || opt.count)
>                     && (opt.pre_context || opt.post_context ||
>                         opt.file_break || opt.funcbody))