[PATCH v8 0/2] Add git-grep threads param

[PATCH v8 0/2] Add git-grep threads param

[Victor Leschuk]

Introducing v8 of git-grep threads param patch.
Patch is now split in 2 parts - 1/2 is actually renewed v6 version (see list of changes below),
2/2 removes dependency on online_cpus() - as we discussed with Eric this is rather 
significant change in default behavior and should be placed into separate patch.

Here is list of changes since v6 ($gmane/281160):

  * Fixed broken t7811: moved all threads_num setup to 1 place (for -O option it was in wrong place)
  * Fixed 'invalid number of threads' message so that it could be translated
  * Got rid of grep_threads_config() - its too trivial to be separate function
  * Fixed xcalloc() args (sizeof(pthread_t) -> sizeof(*threads)) to correspond to general git style
  * Improved commit message (in 2/2) to explain why online_cpus() is now not used in threads_num setup
  * The full param documentation was moved into single place (grep.threads description in git-grep.txt) and is referenced from other places. Also made few language improvements in documentation.
  * Style improvements: splitted too long lines

Victor Leschuk (2):
  "git grep" can now be configured (or told from the command line)    
    how many threads to use when searching in the working tree files.
  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.

 Documentation/config.txt               |  4 +++
 Documentation/git-grep.txt             | 12 +++++++++
 builtin/grep.c                         | 49 +++++++++++++++++++++++-----------
 contrib/completion/git-completion.bash |  1 +
 4 files changed, 51 insertions(+), 15 deletions(-)

-- 
2.6.3.369.g3e7f205.dirty

[PATCH 1/2] Introduce 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.

Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
---
 Documentation/config.txt               |  4 +++
 Documentation/git-grep.txt             | 12 +++++++++
 builtin/grep.c                         | 49 +++++++++++++++++++++++-----------
 contrib/completion/git-completion.bash |  1 +
 4 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2d06b11..cbf4071 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1450,6 +1450,10 @@ 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.
+	See `grep.threads` in linkgit:git-grep[1] for more information.
+
 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..25e6dc5 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 is using 8 threads for all systems.
+	Default behavior may change in future versions
+	to better suit hardware and circumstances.
+
 grep.fullName::
 	If set to true, enable '--full-name' option by default.
 
@@ -227,6 +235,10 @@ OPTIONS
 	effectively showing the whole function in which the match was
 	found.
 
+--threads <num>::
+	Number of grep worker threads.
+	See `grep.threads` in 'CONFIGURATION' for more information.
+
 -f <file>::
 	Read patterns from <file>, one per line.
 
diff --git a/builtin/grep.c b/builtin/grep.c
index 4229cae..e9aebab 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(*threads));
+	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);
@@ -267,6 +270,12 @@ 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)
 		st = -1;
+
+	if (!strcmp(var, "grep.threads")) {
+		/* Sanity check of value will be perfomed later */
+		num_threads = git_config_int(var, value);
+	}
+
 	return st;
 }
 
@@ -294,7 +303,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 +332,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;
@@ -697,6 +706,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,
@@ -786,7 +797,6 @@ 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;
 	}
 
 	if (!opt.pattern_list)
@@ -817,14 +827,23 @@ 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 || online_cpus() == 1 || show_in_pager) {
+		/* Can not multi-thread object lookup */
+		num_threads = 0;
+	}
+	else if (num_threads == 0) {
+		/* User didn't specify value, or just wants default behavior */
+		num_threads = GREP_NUM_THREADS_DEFAULT;
+	}
+	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))
@@ -894,7 +913,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 111b053..d5c3e3f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1311,6 +1311,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 1/2] Introduce grep threads param

[Junio C Hamano]

Victor Leschuk <vleschuk@gmail.com> writes:

> Subject: Re: [PATCH 1/2] Introduce grep threads param

I'll retitle this to something like

    grep: add --threads=<num> option and grep.threads configuration

while queuing (which I did for v7 earlier).

>  "git grep" can now be configured (or told from the command line)
>  how many threads to use when searching in the working tree files.
>
> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
> ---
> ...
> +grep.threads::
> +	Number of grep worker threads.

"Number of grep worker threads to use"?

> +	See `grep.threads` in linkgit:git-grep[1] for more information.
> ...
> +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 is using 8 threads for all systems.
> +	Default behavior may change in future versions
> +	to better suit hardware and circumstances.

The last sentence is too noisy.  Perhaps drop it and phrase it like
this instead?

    grep.threads::
            Number of grep worker threads to use.  If unset (or set to 0),
            to 0), 8 threads are used by default (for now).

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 4229cae..e9aebab 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;

Please do not initialize static to 0 (or NULL).

> @@ -267,6 +270,12 @@ 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)
>  		st = -1;
> +
> +	if (!strcmp(var, "grep.threads")) {
> +		/* Sanity check of value will be perfomed later */

Hmm, is that a good design?

A user may hear "invalid number of threads specified (-4)" later,
but if that came from "grep.threads", especially when the user did
not say "--threads=-4" from the command line, would she know to
check her configuration file?

If she had "grep.threads=Yes" in her configuration, we would
helpfully tell her that 'Yes' given to grep.threads is not a valid
integer.  Shouldn't we do the same for '-4' given to grep.threads,
too?

	if (!strcmp(var, "grep.threads")) {
		num_threads = git_config_int(var, value);
		if (num_threads < 0)
			die(_("invalid number of threads specified (%d) for %s"),
			    num_threads, var);
	}

perhaps.

> @@ -817,14 +827,23 @@ 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 || online_cpus() == 1 || show_in_pager) {
> +		/* Can not multi-thread object lookup */
> +		num_threads = 0;

Removing 'use_threads = 0' from an earlier part and moving the check
to show_in_pager is a good idea, but it invalidates this comment.
The earlier three (actually two and a half) are "cannot" cases,
i.e. the object layer is not easily threaded without locking, and
when you have a single core, you do not truly run multiple
operations at the same time, but as [PATCH 2/2] does, threading in
"grep" is not about CPU alone, so that is why I am demoting it to
just a half ;-).  But show_in_pager is "we do not want to", I think.

In any case, this comment and "User didn't specify" below are not
telling the reader something very much useful.  You probably should
remove them.

> +	}
> +	else if (num_threads == 0) {
> +		/* User didn't specify value, or just wants default behavior */
> +		num_threads = GREP_NUM_THREADS_DEFAULT;
> +	}
> +	else if (num_threads < 0) {
> +		die(_("invalid number of threads specified (%d)"), num_threads);
> +	}

Many unnecessary braces.

I think [2/2] and also moving the code to disable threading when
show-in-pager mode should be separate "preparatory clean-up" patches
before this main patch.  I'll push out what I think this topic
should be on 'pu' later today (with fixups suggested above squashed
in); please check them and see what you think.

Thanks.

Re: [PATCH 1/2] Introduce grep threads param

[Victor Leschuk]

Hello Junio,

On 12/15/2015 11:06 PM, Junio C Hamano wrote:
> Victor Leschuk <vleschuk@gmail.com> writes:
>
>> Subject: Re: [PATCH 1/2] Introduce grep threads param
> I'll retitle this to something like
>
>      grep: add --threads=<num> option and grep.threads configuration
>
> while queuing (which I did for v7 earlier).
Ok, thanks.
>
>>   "git grep" can now be configured (or told from the command line)
>>   how many threads to use when searching in the working tree files.
>>
>> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
>> ---
>> ...
>> +grep.threads::
>> +	Number of grep worker threads.
> "Number of grep worker threads to use"?
Accepted, will change.
>
>> +	See `grep.threads` in linkgit:git-grep[1] for more information.
>> ...
>> +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 is using 8 threads for all systems.
>> +	Default behavior may change in future versions
>> +	to better suit hardware and circumstances.
> The last sentence is too noisy.  Perhaps drop it and phrase it like
> this instead?
>
>      grep.threads::
>              Number of grep worker threads to use.  If unset (or set to 0),
>              to 0), 8 threads are used by default (for now).
Agree.
>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index 4229cae..e9aebab 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;
> Please do not initialize static to 0 (or NULL).
Ok.
>
>> @@ -267,6 +270,12 @@ 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)
>>   		st = -1;
>> +
>> +	if (!strcmp(var, "grep.threads")) {
>> +		/* Sanity check of value will be perfomed later */
> Hmm, is that a good design?
>
> A user may hear "invalid number of threads specified (-4)" later,
> but if that came from "grep.threads", especially when the user did
> not say "--threads=-4" from the command line, would she know to
> check her configuration file?
>
> If she had "grep.threads=Yes" in her configuration, we would
> helpfully tell her that 'Yes' given to grep.threads is not a valid
> integer.  Shouldn't we do the same for '-4' given to grep.threads,
> too?
>
> 	if (!strcmp(var, "grep.threads")) {
> 		num_threads = git_config_int(var, value);
> 		if (num_threads < 0)
> 			die(_("invalid number of threads specified (%d) for %s"),
> 			    num_threads, var);
> 	}
>
> perhaps.
When I was doing this I looked at other code and saw that exact message 
"Invalid number of threads..." is used in other parts of git and is 
present in 'po' translations. Thus I decided to use exactly the same 
message in order not to create numerous almost similar localizations 
(which we should do if we use two different messages in different 
places). Do you think we need to create two more different messages (and 
translations, I can prepare Russian and French) and create two checks 
for cmd and config?
>
>> @@ -817,14 +827,23 @@ 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 || online_cpus() == 1 || show_in_pager) {
>> +		/* Can not multi-thread object lookup */
>> +		num_threads = 0;
> Removing 'use_threads = 0' from an earlier part and moving the check
> to show_in_pager is a good idea, but it invalidates this comment.
> The earlier three (actually two and a half) are "cannot" cases,
> i.e. the object layer is not easily threaded without locking, and
> when you have a single core, you do not truly run multiple
> operations at the same time, but as [PATCH 2/2] does, threading in
> "grep" is not about CPU alone, so that is why I am demoting it to
> just a half ;-).  But show_in_pager is "we do not want to", I think.
>
> In any case, this comment and "User didn't specify" below are not
> telling the reader something very much useful.  You probably should
> remove them.
Ok, will remove comments.
>
>> +	}
>> +	else if (num_threads == 0) {
>> +		/* User didn't specify value, or just wants default behavior */
>> +		num_threads = GREP_NUM_THREADS_DEFAULT;
>> +	}
>> +	else if (num_threads < 0) {
>> +		die(_("invalid number of threads specified (%d)"), num_threads);
>> +	}
> Many unnecessary braces.
I put braces to make code look more unified. I had to put braces here:

+	else if (num_threads == 0) {
+		/* User didn't specify value, or just wants default behavior */


In order to be able to place long comment above the line. And code like:

if (a)
   do_smth();
else if (b) {
   do_smth1();
   do_smth2();
}
else
   do_smth3();

Looks rather ugly to me. Usually I put braces to all 'ifs' if any part 
of block requires them.
Actually I will remove comments as I said above, and thus remove all braces.
>
> I think [2/2] and also moving the code to disable threading when
> show-in-pager mode should be separate "preparatory clean-up" patches
> before this main patch.  I'll push out what I think this topic
> should be on 'pu' later today (with fixups suggested above squashed
> in); please check them and see what you think.
>
> Thanks.
Ok, I will prepare v8 as soon as we finish discussion on this one.

Thanks for the review.

--
Victor

Re: [PATCH 1/2] Introduce grep threads param

[Eric Sunshine]

On Tue, Dec 15, 2015 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Victor Leschuk <vleschuk@gmail.com> writes:
>> Subject: Re: [PATCH 1/2] Introduce grep threads param
>
> I'll retitle this to something like
>
>     grep: add --threads=<num> option and grep.threads configuration
>
> while queuing (which I did for v7 earlier).
>
> I think [2/2] and also moving the code to disable threading when
> show-in-pager mode should be separate "preparatory clean-up" patches
> before this main patch.  I'll push out what I think this topic
> should be on 'pu' later today (with fixups suggested above squashed
> in); please check them and see what you think.

I read over what was pushed to 'pu' and noticed a couple problems.

First, the 'online_cpus() == 1' check, which was removed in patch 1/3,
accidentally creeps back in with patch 3/3.

>> +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 is using 8 threads for all systems.
>> +     Default behavior may change in future versions
>> +     to better suit hardware and circumstances.
>
> The last sentence is too noisy.  Perhaps drop it and phrase it like
> this instead?
>
>     grep.threads::
>             Number of grep worker threads to use.  If unset (or set to 0),
>             to 0), 8 threads are used by default (for now).

Second, the stray "to 0)," on the second line needs to be dropped.

Other than that, the series looks reasonable.

[PATCH 2/2] Get rid of online_cpus() when determining grep threads num

[Victor Leschuk]

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.

Reason: multithreading can improve performance even on single core machines
as IO is also a major factor here. Using multiple threads can significantly
boost grep performance when working on slow filesystems (or repo isn't cached)
or through network (for example repo is located on NFS).

Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index e9aebab..1315905 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -827,7 +827,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	}
 
 #ifndef NO_PTHREADS
-	if (list.nr || cached || online_cpus() == 1 || show_in_pager) {
+	if (list.nr || cached || show_in_pager) {
 		/* Can not multi-thread object lookup */
 		num_threads = 0;
 	}
-- 
2.6.3.369.g3e7f205.dirty