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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 3, 2015, 17:22 UTC
Message-ID: <xmqqvb9jc81q.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1445980944-24000-1-git-send-email-vleschuk@accesssoftek.com>

Victor Leschuk <vleschuk@gmail.com> writes:

Show 25 quoted lines

> Make number of git-grep worker threads a configuration parameter.
> According to several tests on systems with different number of CPU cores
> the hard-coded number of 8 threads is not optimal for all systems:
> tuning this parameter can significantly speed up grep performance.
>
> Signed-off-by: Victor Leschuk <vleschuk@accesssoftek.com>
> ---
>  Documentation/config.txt               |  4 +++
>  Documentation/git-grep.txt             |  9 ++++++
>  builtin/grep.c                         | 56 ++++++++++++++++++++++++----------
>  contrib/completion/git-completion.bash |  1 +
>  4 files changed, 54 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 391a0c3..1dd2a61 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1447,6 +1447,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, use it to tune up performance on
> +	multicore machines. Default value is 8. Set to 0 to disable threading.
> +

I am not enthused by this "Set to 0 to disable". As Zero is magical, it would be more useful if 1 meant that threading is not used (i.e. there is only 1 worker), and 0 meant that we would automatically pick some reasonable parallelism for you (and we promise that the our choice would not be outrageously wrong), or something like that.

Of course, we can do grep.threads=auto to make it even more explicit, but I'd imagine that the in-core code to parse the option and config to the num_threads variable would need one "int" value to represent that "auto", and 0 would be a natural choice for that.

Thanks.