Re: [PATCH] Threaded grep (was: Re: [PATCH] grep: do not do external grep on skip-worktree entries)

Re: [PATCH] Threaded grep (was: Re: [PATCH] grep: do not do external grep on skip-worktree entries)

[Fredrik Kuivinen]

[I messed up the Cc list when I sent the first mail in this thread, so
it didn't reach git@vger. This time it's fixed for real. Sorry for the
extra copy.]

On Fri, Jan 8, 2010 at 19:04, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
>
> On Fri, 8 Jan 2010, Fredrik Kuivinen wrote:
>>
>> I only have access to a couple of boxes with more than one core so
>> some more testing would be greatly appreciated. On the boxes I have
>> tested this on the added parallelism roughly cut the time to grep the
>> linux kernel in half (compared to the built-in grep). It also compares
>> favourably to the external GNU grep (these are best of three runs):
>
> On my box (in all cases best-of-five):
>
>  - "NO_THREADS=1 git grep --no-ext-grep qwerty":
>
>        real    0m0.945s
>        user    0m0.808s
>        sys     0m0.128s
>
>  - "git grep --no-ext-grep qwerty":
>
>        real    0m0.402s
>        user    0m1.116s
>        sys     0m0.216s
>
>  - "git grep qwerty":
>
>        real    0m0.408s
>        user    0m0.176s
>        sys     0m0.152s
>
> so it _just_ beat the external grep thanks to using 330% CPU time. An
> improvement, yes, but the CPU wastage is kind of sad. It really would be
> nice to see if we could get rid of the stupid per-line overhead some way.

I agree. The per-line thing seems to be fixed with Junios recent patch.

> Btw, there does seem to be some unnecessary synchronization there, because
> if I pick a pattern that has no matches at all, my best parallel number
> goes down to 0.316. But the variation in times for the parallel one is so
> big that I don't know how relevant that all is.
>
> I suspect you need more threads than CPU's due to the waiting (so that
> other threads can pick up the slack when one thread ends up waiting to
> output). Or don't wait at all, and queue it up instead.

Yes, you are right, there is some unnecessary synchronization. I am
working on a new patch which queues the output instead.

- Fredrik