Re: [PATCH 1/2] Introduce grep threads param

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 16, 2015, 00:26 UTC
Message-ID: <CAPig+cRTz=DMd6XyJ=co26d2c=PgVqhhsWQpgy53930MdC_=Rw@mail.gmail.com>
In-Reply-To: <xmqq60zzfpdz.fsf@gitster.mtv.corp.google.com>

On Tue, Dec 15, 2015 at 3:06 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Victor Leschuk <vleschuk@gmail.com> writes:
>> Subject: Re: [PATCH 1/2] Introduce grep threads param
>
> I'll retitle this to something like
>
>     grep: add --threads=<num> 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.

Show 13 quoted lines

>> +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.