Re: [PATCH 1/2] Introduce grep threads param
Hello Junio,
On 12/15/2015 11:06 PM, Junio C Hamano wrote:
Victor Leschuk <vlesc...@gmail.com> writes:
Subject: Re: [PATCH 1/2] Introduce grep threads param
I'll retitle this to something like
grep: add --threads= 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 <vlesc...@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 valu
Re: [PATCH 1/2] Introduce grep threads param
Victor Leschuk <vlesc...@gmail.com> writes:
> Subject: Re: [PATCH 1/2] Introduce grep threads param
I'll retitle this to something like
grep: add --threads= 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 <vlesc...@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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Introduce grep threads param
On Tue, Dec 15, 2015 at 3:06 PM, Junio C Hamano <gits...@pobox.com> wrote: > Victor Leschuk <vlesc...@gmail.com> writes: >> Subject: Re: [PATCH 1/2] Introduce grep threads param > > I'll retitle this to something like > > grep: add --threads= 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
