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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 10, 2015, 20:55 UTC
Message-ID: <CAPig+cQXCHzAJ80ZEHcM_+7J7jRs1obwuSzqe7sRDnuocMYUbw@mail.gmail.com>
In-Reply-To: <1447162118-17636-1-git-send-email-vleschuk@accesssoftek.com>

On Tue, Nov 10, 2015 at 8:28 AM, Victor Leschuk <vleschuk@gmail.com> wrote:

Show 5 quoted lines

> "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>
> ---

As an aid for reviewers, please use this space (below the "---" line) to describe what changed since the previous version. It's also helpful if you can provide a link to the previous version in the mailing list archive.

Show 17 quoted lines

>  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
> @@ -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 suite hardware and circumstances.

s/suite/suit/ (here and elsewhere)

Was the conclusion of discussion that there should be some explanation here that this is more about tuning for I/O rather than CPU, or did I misunderstand?

Show 20 quoted lines

>  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..91027b6 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> diff --git a/builtin/grep.c b/builtin/grep.c
> @@ -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);

The original code consulted online_cpus(), but the new code does not. This is a sufficiently significant change that it deserves mention in the commit message. In fact, it's really a distinct change which might deserve being done in its own preparatory patch (with an explanation something along the lines of "even single-core machines may benefit from threading when the bottleneck is I/O").

>  #else
> -       use_threads = 0;
> +       num_threads = 0;
>  #endif