John Keeping <john@xxxxxxxxxxxxx> writes:
> On Thu, Oct 22, 2015 at 04:23:56PM +0300, Victor Leschuk wrote:
>> Hello all, I suggest we make number of git-grep worker threads a configuration
>> parameter. I have run several tests on systems with different number of CPU cores.
>> It appeared that the hard-coded number 8 lowers performance on both of my systems:
>> on my 4-core and 8-core systems the thread number of 4 worked about 20% faster than
>> default 8. So I think it is better to allow users tune this parameter.
>
> For git-pack-objects we call the command-line parameter "--threads" and
> the config variable "pack.threads". Is there a reason not to use the
> same name here (i.e. "--threads" and "grep.threads")?
Good suggestion.
>> +-t <num>::
>> +--threads-num <num>::
>> + Set number of worker threads to <num>. Default is 8.
It is not like you would want to specify different degree of
parallelism for every invocation of "grep". The command line option
for this feature is expected to be used extremely infrequently, only
to defeat a configuration variable that is misconfigured to a wrong
value. Squatting on a short-and-sweet single-letter option name is
unwarranted for an option like this.
>> - for (i = 0; i < ARRAY_SIZE(threads); i++) {
>> + threads = xmalloc(sizeof(pthread_t) * opt->num_threads);
xcalloc(nmemb, size)?
>> @@ -702,6 +704,10 @@ 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")),
>> +#ifndef NO_PTHREADS
>> + OPT_INTEGER('t', "threads-num", &opt.num_threads,
>> + N_("use <n> worker threads")),
>> +#endif /* !NO_PTHREADS */
>> OPT_NUMBER_CALLBACK(&opt, N_("shortcut for -C NUM"),
>> context_callback),
>> OPT_BOOL('p', "show-function", &opt.funcname,
>> @@ -910,7 +916,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> }
>>
>> if (use_threads)
>> - hit |= wait_all();
>> + hit |= wait_all(&opt);
I do not think anybody checked if opt.num_threads is a sensible
number after parse_options() returned at this point. There is a
code outside the context before this hunk that decides if it makes
sense to use threads and turns use_threads to zero, which should
be the right place to do the missing sanity check, I thinnk.
>> diff --git a/grep.c b/grep.c
>> index 7b2b96a..17e6a7c 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -40,6 +40,9 @@ void init_grep_defaults(void)
>> color_set(opt->color_selected, "");
>> color_set(opt->color_sep, GIT_COLOR_CYAN);
>> opt->color = -1;
>> +#ifndef NO_PTHREADS
>> + opt->num_threads = GREP_NUM_THREADS_DEFAULT;
>> +#endif /* !NO_PTHREADS */
>> }
These #ifndef/#endif's are unnecessary distraction. Just set them
unconditionally, even in NO_PTHREADS build (and of course include
the field even in NO_PTHREADS build in grep.h).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html