Re: [PATCH v4 2/8] grep: skip pthreads overhead when using one thread

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Jun 1, 2017 at 11:36 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Thu, Jun 1, 2017 at 11:20 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Thu, Jun 1, 2017 at 11:20 AM, Ævar Arnfjörð Bjarmason
>> <avarab@gmail.com> wrote:
>>
>>> +       if (num_threads == 1)
>>> +               num_threads = 0;
>>
>> I would think that it is easier to maintain the code when keep the 1
>> hard coded, and apply the following diff instead. If we encounter
>> a 0 later on, it is not clear what the original user input was.
>> (Did the user ask for 0 as a proxy for GREP_NUM_THREADS_DEFAULT ?
>> do they care about the number of threads?)
>> It is less complexity in the decision logic here.
>>
>> --8<-- (white space broken)
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index c6c26e9b9e..6ad9b3da20 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -1231,7 +1231,7 @@ int cmd_grep(int argc, const char **argv, const
>> char *prefix)
>>  #endif
>>
>>  #ifndef NO_PTHREADS
>> -       if (num_threads) {
>> +       if (num_threads > 1) {
>>                 if (!(opt.name_only || opt.unmatch_name_only || opt.count)
>>                     && (opt.pre_context || opt.post_context ||
>>                         opt.file_break || opt.funcbody))
>> @@ -1295,7 +1295,7 @@ int cmd_grep(int argc, const char **argv, const
>> char *prefix)
>>                 hit = grep_objects(&opt, &pathspec, &list);
>>         }
>>
>> -       if (num_threads)
>> +       if (num_threads > 1)
>>                 hit |= wait_all();
>>         if (hit && show_in_pager)
>>                 run_pager(&opt, prefix);
>> --8<--
>
> If I've understood you correctly (what I applied on top of this based
> on the above at the end of the mail) this segfaults because now we
> won't compile the pattern.
>
> I agree that this logic is a bit tricky, but it must be considered
> with the "!num_threads" logic in preceding "grep: don't redundantly
> compile throwaway patterns under threading" patch.
>
> I.e. we already have num_threads being assigned to 0 under
> NO_PTHREADS, doing the same for the PTHREADS codepath seemed like the
> simplest solution to me.
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index a0a3922f92..6d16df2526 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1238,25 +1238,23 @@ int cmd_grep(int argc, const char **argv,
> const char *prefix)
>                 num_threads = GREP_NUM_THREADS_DEFAULT;
>         else if (num_threads < 0)
>                 die(_("invalid number of threads specified (%d)"), num_threads);
> -       if (num_threads == 1)
> -               num_threads = 0;
>  #else
>         if (num_threads)
>                 warning(_("no threads support, ignoring --threads"));
> -       num_threads = 0;
> +       num_threads = 0; /* If we have no threads... */
>  #endif
>
> -       if (!num_threads)
> +       if (num_threads) /* Or if we've decided not to use them... */

I didn't mean to change this bit, it should remain "if
(!num_threads)". I was in the middle of monkeypatching and didn't
review the diff carefully enough. But it any case, without this change
the rest of this diff is your proposed (but segfaulting) change as I
understand it.

>                 /*
>                  * The compiled patterns on the main path are only
>                  * used when not using threading. Otherwise
>                  * start_threads() below calls compile_grep_patterns()
>                  * for each thread.
>                  */
> -               compile_grep_patterns(&opt);
> +               compile_grep_patterns(&opt); /* We'll compile the
> pattern here */
>  #ifndef NO_PTHREADS
> -       if (num_threads) {
> +       if (num_threads > 1) {
>                 if (!(opt.name_only || opt.unmatch_name_only || opt.count)
>                     && (opt.pre_context || opt.post_context ||
>                         opt.file_break || opt.funcbody))
> @@ -1320,7 +1318,7 @@ int cmd_grep(int argc, const char **argv, const
> char *prefix)
>                 hit = grep_objects(&opt, &pathspec, &list);
>         }
>
> -       if (num_threads)
> +       if (num_threads > 1)
>                 hit |= wait_all();
>         if (hit && show_in_pager)
>                 run_pager(&opt, prefix);