Re: [PATCH] git-rev-list: give better diagnostic for failed write

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Jun 25, 2007, 23:16 UTC
Message-ID: <alpine.LFD.0.98.0706251607000.8675@woody.linux-foundation.org>
In-Reply-To: <alpine.LFD.0.98.0706251536240.8675@woody.linux-foundation.org>

On Mon, 25 Jun 2007, Linus Torvalds wrote:

Show 6 quoted lines

> 
> There's also another issue: regular files really *are* different from 
> pipes and sockets and other things. Not because of EPIPE, but because you 
> want different buffering behaviour. For a regular file, we really don't 
> even care about the line buffering, and we'd actually be better off (from 
> a performance angle) without it.

Just for fun, I tried this out.

Doing

	time git log > logfile

on the kernel repo with and without the patch I just sent out, I get:

Without:

	real    0m1.361s
	user    0m1.312s
	sys     0m0.040s

With:

	real    0m1.687s
	user    0m1.392s
	sys     0m0.284s

so doing the extra flushing does actually cost us (it's just fundamentally more expensive to do disk IO on non-block-boundaries).

It would be much nicer if we only did it for sockets and pipes, which don't have the same block-boundary issues anyway (there's still the system call cost, but on a pipe/socket, the real costs tend to be elsewhere).

Again, this is something that a non-stdio-based buffering library would easily handle. You could just test the file descriptor _once_ at the beginning, to see if it's a regular file or not. And then you could have the error handling where it belongs (when the IO is actually done, and the error actually happens) rather than in the callers using a bad interface that sometimes loses 'errno'.

Btw, to balance the above performance comment: doing the flush_or_die() obviously *does* mean that you get better performance in the odd cases. For example, if you do

	[torvalds@woody linux]$ trap '' SIGPIPE
	[torvalds@woody linux]$ time git log | head

I get get 0.002s, while it used to be:

	real    0m1.382s
	user    0m1.340s
	sys     0m0.028s

just because it did the whole thing regardless of any EPIPE errors.

Of course, that case probably isn't very usual, but I could imagine that some users of "git blame -C --incremental" could actually cause situations like this (ie just stop listening when they got the part they're interested in, and maybe they'd have some strange reason to ignore SIGPIPE).

So I'm not opposed to the patch I sent out, I just wanted to point out that this is an area we *could* improve upon if we didn't do that stdio thing.

		Linus