Re: [PATCH] grep: do not do external grep on skip-worktree entries

7 messages, 5 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] grep: do not do external grep on skip-worktree entries

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:47:57

Jeff King [off-list ref] writes:
On Sun, Jan 03, 2010 at 12:49:15PM -0800, Junio C Hamano wrote:
quoted
Linus Torvalds [off-list ref] writes:
quoted
Which means that if you put /usr/xpg4/bin before other paths in your PATH, 
you'll totally break such systems, because now you get the (inferior) 
tools in xpg4 before the preferred tools in /usr/local. Or - this also 
happens - people end up installing their own versions in $HOME/bin, 
because the system admin is uncaring or incompetent.
The build allows you to define SANE_TOOL_PATH ("the tools found in here
are saner than the ones in /usr/bin or /bin" is what it means) and we
insert it just in front of /usr/bin or /bin in the original PATH (see
git_brokne_path_fix in git-sh-setup.sh).

I would call this the right thing (TM) or the best workaround we could do
under the constraints, depending on the mood.
I agree that Solaris default tools are insane, but is there any reason
to munge the PATH for a single feature like external grep? Why not
EXTERNAL_GREP=/usr/xpg4/bin/grep (or /usr/local/bin/grep) in the
Makefile? Why not GIT_EXTERNAL_GREP=$HOME/bin/grep in the environment?
That git-sh-setup "fix" is not for running external grep.  It is for our
scripted Porcelains that rely on working basic tools (sed, tr, who knows
what else is broken).

In fact, our Makefile by default punts on external grep on Sun's.  Run
"git grep NO_EXTERNAL_GREP -- Makefile" to see for yourself --- it would
work even on Solaris ;-)
Obviously we still need SANE_TOOL_PATH for systems where the /usr/bin is
so crappy as to be unusable for our scripts. But surely we can do better
for individual tools where the user might have some more specific
preference about which tool he uses.

-Peff

Re: [PATCH] grep: do not do external grep on skip-worktree entries

From: Jeff King <hidden>
Date: 2016-06-15 22:47:58

On Sun, Jan 03, 2010 at 09:52:10PM -0800, Junio C Hamano wrote:
quoted
I agree that Solaris default tools are insane, but is there any reason
to munge the PATH for a single feature like external grep? Why not
EXTERNAL_GREP=/usr/xpg4/bin/grep (or /usr/local/bin/grep) in the
Makefile? Why not GIT_EXTERNAL_GREP=$HOME/bin/grep in the environment?
That git-sh-setup "fix" is not for running external grep.  It is for our
scripted Porcelains that rely on working basic tools (sed, tr, who knows
what else is broken).
Right, but I thought this thread was about external grep, and I thought
you were saying "if you want decent tools, you can use SANE_TOOL_PATH".
And I think we can do much better for that particular case than
recommending SANE_TOOL_PATH (but it seems that is not what you were
actually recommending).

But I admit, I have never really wanted to specify my own external grep.
Wanting your own grep for _features_ is probably insane, as some of your
greps (on worktree files) will use the external grep, and some (on
cached files) will not.  So it is really just an optimization, and I
have never felt it so slow that I cared about messing with an
alternative grep on Solaris.

I have to wonder, though...did anybody ever actually profile our
internal grep to find out _why_ it was so much slower than GNU grep?
Could we simply ship a better grep engine and obsolete external grep?
In fact, our Makefile by default punts on external grep on Sun's.  Run
"git grep NO_EXTERNAL_GREP -- Makefile" to see for yourself --- it would
work even on Solaris ;-)
Yes, I am even mentioned in the commit log of 01ae841c. :)

-Peff

Re: [PATCH] grep: do not do external grep on skip-worktree entries

From: Nguyen Thai Ngoc Duy <hidden>
Date: 2016-06-15 22:47:58

On Mon, Jan 4, 2010 at 12:52 PM, Junio C Hamano [off-list ref] wrote:
quoted
I agree that Solaris default tools are insane, but is there any reason
to munge the PATH for a single feature like external grep? Why not
EXTERNAL_GREP=/usr/xpg4/bin/grep (or /usr/local/bin/grep) in the
Makefile? Why not GIT_EXTERNAL_GREP=$HOME/bin/grep in the environment?
That git-sh-setup "fix" is not for running external grep.  It is for our
scripted Porcelains that rely on working basic tools (sed, tr, who knows
what else is broken).

In fact, our Makefile by default punts on external grep on Sun's.  Run
"git grep NO_EXTERNAL_GREP -- Makefile" to see for yourself --- it would
work even on Solaris ;-)
A bit off-topic. But it seems to me on linux (main development
platform?) GNU grep may be tested more than the builtin grep because
NO_EXTERNAL_GREP would be undefined by default. Should we test both
greps in that case?
-- 
Duy

Re: [PATCH] grep: do not do external grep on skip-worktree entries

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:47:58


On Mon, 4 Jan 2010, Jeff King wrote:
I have to wonder, though...did anybody ever actually profile our
internal grep to find out _why_ it was so much slower than GNU grep?
Could we simply ship a better grep engine and obsolete external grep?
The internal grep is about 2.5 times slower than the external one for me. 
That's a big deal:

 - external grep:

	[torvalds@nehalem linux]$ time git grep qwerty
	...
	real	0m0.412s
	user	0m0.196s
	sys	0m0.132s

 - NO_EXTERNAL_GREP:

	[torvalds@nehalem linux]$ time ~/git/git grep qwerty
	...
	real	0m1.006s
	user	0m0.900s
	sys	0m0.096s

so that's not even close.

And "perf record" followed by "perf report" on the internal one shows 
that it's not even regexec() - we use strstr() for the trivial case:

    43.63%      git  /home/torvalds/git/git         [.] grep_buffer_1
    25.19%      git  /lib64/libc-2.11.so            [.] __strstr_sse42
     9.16%      git  /home/torvalds/git/git         [.] match_one_pattern
     4.79%      git  /lib64/libc-2.11.so            [.] __m128i_strloadu

bit it seems to be all that line-per-line crud. If we got rid of that one, 
and could do the match as a _single_ regexec() instead (at least for the 
trivial cases of just one grep expression), perhaps we'd be better off.

			Linus

Re: [PATCH] grep: do not do external grep on skip-worktree entries

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:47:58


On Tue, 5 Jan 2010, Miles Bader wrote:
On Tue, Jan 5, 2010 at 12:54 AM, Linus Torvalds
[off-list ref] wrote:
quoted
And "perf record" followed by "perf report" on the internal one shows
that it's not even regexec() - we use strstr() for the trivial case:
Does strstr use e.g. boyer-moore?  I imagine grep does...
It doesn't matter. Since we do the line-by-line thing, the input is always 
so short that DFA vs NFA vs BM vs other-clever-search doesn't matter. 
There is no scaling - the grep buffer tends to be too small for the 
algorithm to matter.

And the reason we do things line-by-line is that we need to then output 
things line-per-line.

			Linus

Re: [PATCH] grep: do not do external grep on skip-worktree entries

From: Miles Bader <hidden>
Date: 2016-06-15 22:47:58

On Tue, Jan 5, 2010 at 12:54 AM, Linus Torvalds
[off-list ref] wrote:
And "perf record" followed by "perf report" on the internal one shows
that it's not even regexec() - we use strstr() for the trivial case:
Does strstr use e.g. boyer-moore?  I imagine grep does...

-miles

-- 
Do not taunt Happy Fun Ball.

Re: [PATCH] grep: do not do external grep on skip-worktree entries

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: 2016-06-15 22:47:58


On Mon, 4 Jan 2010, Linus Torvalds wrote:
 - external grep:

	[torvalds@nehalem linux]$ time git grep qwerty
	...
	real	0m0.412s
	user	0m0.196s
	sys	0m0.132s

 - NO_EXTERNAL_GREP:

	[torvalds@nehalem linux]$ time ~/git/git grep qwerty
	...
	real	0m1.006s
	user	0m0.900s
	sys	0m0.096s

so that's not even close.
Side note: at least for me, if we did some auto-parallelization, the 
internal grep would make up for all its other suckiness. Do four or eight 
greps in parallel, and buffer the results (you still need to show them in 
the right order).

That might be an acceptable way to "fix" it. Developers pretty much all 
have at least two cores these days, some of us have four+HT. We use 
threads in other places, maybe this could be one more of them.

(Start 'n' threads, do an initial per-thread regex and 'regcomp()' to make 
it thread-safer, and the only interesting issue would be serializing the 
output. Whenever you get a result, you'd need to make sure that all files 
before have been completed, but you could do that all under a specific 
lock that protects completion information).

		Linus
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help