Re: [PATCH 01/12] grep: add ability to disable threading with --threads=0 or grep.threads=0

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 11, 2017, 20:56 UTC
Message-ID: <CACBZZX7Xi2OWqHQd7jTGBEZyqcWk59oXbPJOjuYrYAFzd5huCA@mail.gmail.com>
In-Reply-To: <20170411203434.iiupo2oovzviqju5@sigill.intra.peff.net>

On Tue, Apr 11, 2017 at 10:34 PM, Jeff King <peff@peff.net> wrote:

Show 44 quoted lines

> On Tue, Apr 11, 2017 at 10:20:59PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> I'm struggling to find a use-case where threading makes sense at all.
>> The example in the initial introduction in 5b594f457a is always slower
>> with >0 for me, and since then in 0579f91dd7 it got disabled entirely
>> for non-worktree cases.
>
> It's a big win for me in worktree greps of linux.git:
>
>   $ best-of-five git grep --threads=1 '[q]werty'
>   Attempt 1: 0.713
>   Attempt 2: 0.708
>   Attempt 3: 0.689
>   Attempt 4: 0.695
>   Attempt 5: 0.7
>
>   real  0m0.689s
>   user  0m0.560s
>   sys   0m0.248s
>
>   $ best-of-five git grep --threads=8 '[q]werty'
>   Attempt 1: 0.238
>   Attempt 2: 0.225
>   Attempt 3: 0.222
>   Attempt 4: 0.221
>   Attempt 5: 0.225
>
>   real  0m0.221s
>   user  0m0.936s
>   sys   0m0.356s
>
> In non-worktree cases most of the time goes to accessing objects, which
> happens under a lock. So you don't get any real parallelism, just
> overhead.
>
>> But assuming it works for someone out there, then 0 threads is clearly
>> not the same as 1. On linux.git with pcre2 grepping for [q]werty for
>> example[1]
>
> Right, my suggestion was to teach "grep" to treat --threads=1 as "do not
> spawn any other threads". I.e., to make it like the "0" case you were
> proposing, and then leave "0" as "auto-detect". There would be no way to
> spawn a _single_ thread and feed it. But why would you want to do that?
> It's always going to be strictly worse than not threading at all.

I understand, but given the two profiles we've posted it seems clear that there's cases where if we did that, we'd be locking people out of their optimal thread configuration, which would be --thread=1 with my patch, but wouldn't exist with this proposed change.

If you see better timings with 8 threads than 1 on linux.git, but I see strictly worse, then if you apply my patch doesn't --threads=0 look worse than --threads=1, which looks worse than --threads=2 etc, until you reach some number where you either run out of CPU or I/O throughput.

Anyway, I really don't care about this feature much, I just wanted a way to disable threading, but looking at the perf profiles I wonder if doing your proposed change would cause a regression in some cases where someone really wanted /one/ thread.

But of course my patch breaks the long documented grep.threads=0 for "give me threads that you auto detect" to now mean "you get none".

Also doesn't --thread=1 right now mean "one thread, but two workers?". I haven't dug into the grep worker/thread code, but it compiles the the pattern twice, so isn't both the non-thread main process & the sole thread it spawns on --thread=1 doing work, so in some other universe it's synonymous with --workers=2?

If so do pack-objects & index-pack also behave like that? If so this whole thing is very confusing for users, because some will read 1 thread and think "one worker", whereas it really means "two workers, one using a thread, if you want three workers spawn two threads".

Bah!