From: Jim Meyering <hidden> Date: 2016-06-15 22:43:18
Linus Torvalds [off-list ref] wrote:
On Mon, 25 Jun 2007, Jim Meyering wrote:
quoted
[this patch depends on the one I posted here:
http://marc.info/?l=git&m=118280134031923&w=2 ]
Without this patch, git-rev-list unnecessarily omits strerror(errno)
from its diagnostic, upon write failure:
And this is a perfect example of what's wrong with the whole thing.
Dammit, how many times do I need to say this:
- If you want reliable errors, don't use stdio!
That fflush is there FOR A REASON. You removed it FOR A MUCH LESS
IMPORTANT REASON!
Wow. No need to curse and get into ALL_CAPS_MODE every time you
reply to me. It does not advance your cause.
Remember: I'm trying to improve existing code here.
You should save some of your ire for the person who wrote that code.
That fflush is there exactly because WE DO NOT WANT TO BUFFER the list of
commits, because that thing is meant very much to be used for pipelines,
and it's quite common that the receiving end is going to do something
asynchronous with the result, and can - and does - want the results as
soon as possible.
That's good to know. I'm glad you pointed it out. It would have been
nice to have a comment. However, wouldn't it be better at least to check
for and report fflush failure? fflush usually does a write, after all.
Most of the rest of the code is careful to diagnose write errors at
the source. Why not here?
I've posted a revised patch.
IOW, things like "gitk" use git-rev-list exactly to get the list of
commits, and they want that list *incrementally*. They don't want to wait
for git-rev-list to have filled up some 8kB buffer of commits. Especially
since generating those commits can be slow if we're talking about a big
tree and some path-limited stuff.
So for example, do something like
git rev-list HEAD -- drivers/char/drm/Makefile
and if you don't see the result scroll a line at a time on a slower
machine, there's something *wrong*.
Junio, I'm NAK'ing this very forcefully!
Jim: I don't know what I'm doing wrong, but I'm apparently not reaching
you. So let me try one more time:
- stdio really isn't very good with error handling
- if you use stdio, YOU HAD BETTER ACCEPT THAT
Using stdio is fine, as long as you know and respect its limitations.
It's a real shame that you have to intersperse your often-valuable
feedback with such vitriol.
Remember: I'm trying to improve existing code here.
You should save some of your ire for the person who wrote that code.
Ehh. Remind me who I should be pissed at when the old code was _better_
before your change?
With the current git.c, we report write errors quite well. We don't give
the exact output you want, but on a scale of 1-10, how important is that?
Pretty damn low on the list.
And the reason I'm really really irritated at you is that you ignore me
when I tell you what your bugs are.
- I *told* you that EPIPE is special. What did you do? Ignore my advice,
and made a broken patch that did exactly the opposite of what I told
you.
- And I *told* you that you shouldn't care about errno for stdio, because
stdio was broken. What did you do? You again ignored my advice, and
made _another_ broken patch, exactly the _opposite_ of what I told you.
If you really really *must* get that ENOSPC error string output, create a
helper function like
void flush_or_die(FILE *f, const char *desc)
{
if (ferror(f))
die("write failure on %s", desc)
if (fflush(f)) {
if (errno == EPIPE)
exit(0);
die("write failure on %s: %s",
desc, strerror(errno));
}
}
and then you can start adding calls to "flush_or_die()" to appropriate
places. You could replace the "fflush()" in builtin-rev-list.c with a
"flush_or_die()".
And then you could add a call to "log_tree_commit()" (in log-tree.c), and
that would probably be an improvement too (especially if we start having
things like gitk parse "git log" output, and try to deprecate the old
really low-level plumbing a bit).
Linus
If you really really *must* get that ENOSPC error string output, create a
helper function like
void flush_or_die(FILE *f, const char *desc)
{
if (ferror(f))
die("write failure on %s", desc)
Actually, even this is really nasty, and it's a case where the current
"git.c" code can also fail.
It's an example of where EPIPE can actually cause a write failure that you
can never figure out what the reason for the error was, because the flush
was caused by something earlier (write too much for the buffer or
whatever), exactly because stdio throws the error away.
So after thinking about it some more, I would suggest just ignoring ferror
entirely, and hoping that any errors are caught by the fflush().
I do hate stdio error checking. In my opinion, there really is only *one*
correct way to use stdio error checking: ignore it. It doesn't work. The
thing is fundamentally mis-designed for error handling.
So I think the right solution would literally be to either not do this
broken error checking at all, or to rewrite the code that cares about
errors to not use stdio.
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.
But we don't have any sane way to save that kind of information (and we
definitely do *not* want to do the "fstat()" thing on every flush). We
could use the stdio buffering mode, but
- it's too weak (we want not line buffered or block buffered, we want
basically "record buffered")
- I don't think there are any portable ways to read it (only set it,
using "setbuf()" and friends).
Anyway, getting rid of stdio for writes we care about really *would* be a
nice thing. But it's a lot of boring, nasty work.
So here's a patch that I think is acceptable. IT IS NOT PERFECT. Stdio
simply cannot do a good job on errors, but it has a comment about the case
where it just decides to ignore ferror() instead of doing what I suggest
above.
I'm not saying this is great. But doing this right really does require
avoiding stdio entirely.
Does this work for you?
(I pass the "desc" string thing in case there is some future use where we
also want to use stdio, but we use it for something else than just regular
stdout. Dunno).
Linus
---
builtin-rev-list.c | 2 +-
cache.h | 2 ++
log-tree.c | 1 +
write_or_die.c | 20 ++++++++++++++++++++
4 files changed, 24 insertions(+), 1 deletions(-)
Actually, even this is really nasty, and it's a case where the current
"git.c" code can also fail.
Just to clarify: this is why the git.c code obviously does the fstat(), in
case anybody wondered.
So I didn't mean to imply that the new git.c code in 'next' is wrong, I
just meant to imply that the "ferror()"+"fflush()" sequence that it uses
and that I copied for my example is a very unreliable sequence, and it
basically fails exactly because you can never know what caused the
ferror() to trigger - if it *ever* triggers, you're basically screwed.
I wonder how many applications actually ever use ferror() and friends.
Linus
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
I think flushing here is a good change regardless of the error checking.
Sometimes, when you are severely limiting commits, the whole output is
smaller than the buffer, and you end up waiting a long time for any
output even though your answer may have been found immediately.
For example, 'git-whatchanged -Sfoo' when 'foo' was introduced in the
last couple of commits (but wasn't referenced before) will have to
calculate diffs on all of history before producing output. Flushing
after every commit restores the illusion that git provides your answer
instaneously. :)
-Peff
On Mon, Jun 25, 2007 at 04:16:56PM -0700, Linus Torvalds wrote:
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'.
Is there something obviously wrong with doing something like this?
if ((fstat(fileno(stdout), &st) < 0) &&
!S_ISREG(st.st_mode))
setbuf(stdout, NULL);
This would change stdout to use completely unbuffered I/O we're not
sending the output to a file.
- Ted
I think flushing here is a good change regardless of the error checking.
Sometimes, when you are severely limiting commits, the whole output is
smaller than the buffer, and you end up waiting a long time for any
output even though your answer may have been found immediately.
Exactly. That's why it was done in git-rev-list, and why it's good to do
in "git log" too.
The slowdown worries me a bit, but it's only noticeable with *lots* of
output - ie no path limiting (and no diffs - the diff generation becomes
the bottleneck if you do diffs). So the flushing slows down a case that we
do ridiculously well right now, so I doubt anybody will really care.
It's more a benchmarking issue: "we can show the whole log of the kernel
in under two seconds", and it didn't slow down *that* much.
For example, 'git-whatchanged -Sfoo' when 'foo' was introduced in the
last couple of commits (but wasn't referenced before) will have to
calculate diffs on all of history before producing output. Flushing
after every commit restores the illusion that git provides your answer
instaneously. :)
On that note, it should probably also do it for the incremental output of
git blame.
Linus
---
Is there something obviously wrong with doing something like this?
if ((fstat(fileno(stdout), &st) < 0) &&
!S_ISREG(st.st_mode))
setbuf(stdout, NULL);
This would change stdout to use completely unbuffered I/O we're not
sending the output to a file.
Well, we might as well keep it line-buffered, so I'd use setvbuf(_IOLBF)
instead. Totally unbuffered is bad, since we often do printf's in smaller
chunks.
But we actually _do_ want fully buffered from a performance angle.
Especially for the big stuff, which is usually the *diffs*, not the commit
messages. Not so much an issue with git-rev-list, but with "git log -p"
you would normally not want it line-buffered, and it's actually much nicer
to let it be fully buffered and then do a flush at the end.
Even pipes are often used for "throughput" stuff if you end up doing some
post-processing (ie "git log -p | gather-statistics"), and yes, I actually
do things like that - it's nice for things like looking at how many lines
have been added during the last release cycle:
git log -p v2.6.21.. | grep '^+' | wc -l
and I'd really like thigns like that to be close to optimal.
How much the system call overhead is, I don't know, though. So it might be
worth testing out. Under Linux, you'll probably have a fairly hard time
seeing any difference, but under other OS's you have both system call
latency issues *and* possible scheduling issues (ie the above kind of
pipeline can act very differently from a scheduling standpoint if you send
lots of small things rather than buffer things a bit on the generating
side)
Linus
On Tue, Jun 26, 2007 at 10:32:23AM -0700, Linus Torvalds wrote:
But we actually _do_ want fully buffered from a performance angle.
Especially for the big stuff, which is usually the *diffs*, not the commit
messages. Not so much an issue with git-rev-list, but with "git log -p"
you would normally not want it line-buffered, and it's actually much nicer
to let it be fully buffered and then do a flush at the end.
Well, sometimes we do and sometimes we don't right? Some of it
depends on how large the *stuff* we are dumping out (diffs vs. commit
objects), and what the receiving process on the other bit of the
pipeline is doing with the data --- is it doing some kind of
statistical reduction (git-rev-log .. | wc -l) versus some kind of
asynchronous processing (git-rev-log as used by gitk).
So maybe the answer is that when outputing to a file, we always use
full stdio buffering, and in other cases there should be some
intelligent defaults plus command-line overrides. So when we emit
lists of commit identifiers, it should probably be line buffered, and
if it is diffs, it should probably be fully buffered, etc.?
- Ted
On Tue, Jun 26, 2007 at 10:32:23AM -0700, Linus Torvalds wrote:
quoted
But we actually _do_ want fully buffered from a performance angle.
Especially for the big stuff, which is usually the *diffs*, not the commit
messages. Not so much an issue with git-rev-list, but with "git log -p"
you would normally not want it line-buffered, and it's actually much nicer
to let it be fully buffered and then do a flush at the end.
Well, sometimes we do and sometimes we don't right?
No.
We basically _never_ want "line buffered" or "unbuffered", which is what
stdio knows how to do. That sucks in _all_ cases.
What we want is "fully buffered" for plain files, and "record buffered"
for anything else (where a "record" is basically the "commit + optional
diff").
We can get the record buffered by adding the fflush() calls, but the thing
is, we'd want to _avoid_ that if it was a file. It's just that there is no
way to set that kind of flag portably with stdio, we'd have to carry it
around _separately_ from stdio, which is a big pain.
But if we decide that this only matters with stdout (which currently is
what the patches have done), we could of course just make it a single
global variable (like "stdout" itself already is). Then we could just make
git.c start out by testing stdout at startup and setting the global
variable.
Linus
On Tue, Jun 26, 2007 at 10:32:23AM -0700, Linus Torvalds wrote:
But we actually _do_ want fully buffered from a performance angle.
Especially for the big stuff, which is usually the *diffs*, not the commit
messages. Not so much an issue with git-rev-list, but with "git log -p"
you would normally not want it line-buffered, and it's actually much nicer
to let it be fully buffered and then do a flush at the end.
Even pipes are often used for "throughput" stuff if you end up doing some
post-processing (ie "git log -p | gather-statistics"), and yes, I actually
do things like that - it's nice for things like looking at how many lines
have been added during the last release cycle
git log -p v2.6.21.. | grep '^+' | wc -l
and I'd really like thigns like that to be close to optimal.
How much the system call overhead is, I don't know, though. So it might be
worth testing out....
So just for yuks, I devised the following patch, and did some measurements....
For the above pipeline, the result was hardly worth mentioning:
With flushing:
% time git log -p v2.6.21.. | grep '^+' | wc -l
real 0m22.330s
user 0m21.512s
sys 0m0.807s
# of write() system calls according to strace -c: 15167
Without flushing:
% time git log -p v2.6.21.. | grep '^+' | wc -l
real 0m22.367s
user 0m21.355s
sys 0m0.720s
# of write() system calls according to strace -c: 11373
So here's the worst case:
% time git-rev-list HEAD > /dev/null
real 0m1.575s
user 0m1.477s
sys 0m0.053s
# of write() system calls according to strace -c: 582
% (export GIT_NEVER_FLUSH_STDOUT=t; time git-rev-list HEAD > /dev/null)
real 0m1.535s
user 0m1.463s
sys 0m0.027s
# of write() system calls according to strace -c: 58055
Under Linux, you'll probably have a fairly hard time
seeing any difference, but under other OS's you have both system call
latency issues *and* possible scheduling issues (ie the above kind of
pipeline can act very differently from a scheduling standpoint if you send
lots of small things rather than buffer things a bit on the generating
side)
Indeed. So it's not at all clear this patch is worth applying, but
maybe it would make a difference on some other OS; of course, this
patch also obviates the original intent of Jim Meyering's patch, since
it means that we won't print a useful error message if stdout has been
redirected to a file and the write returns ENOSPC, since we won't be
fflush() when stdout is redirected to a file.
The added fflush() calls to the incremental git-blame and the
git-log-*/git-whatchanged might make it worthwhile for tools that
depend on those outputs and want faster user response time. So maybe
adding the fflush() call might be worthwhile for those programs.
- Ted
commit 7f483ec6366f62d52199e3edefa292a110fcb5c8
Author: Theodore Ts'o [off-list ref]
Date: Thu Jun 28 14:10:58 2007 -0400
Don't fflush(stdout) when it's not helpful
This patch arose from a discussion started by Jim Meyering's patch
whose intention was to provide better diagnostics for failed writes.
Linus proposed a better way to do things, which also had the added
benefit that adding a fflush() to git-log-* operations and incremental
git-blame operations could improve interactive respose time feel, at
the cost of making things a bit slower when we aren't piping the
output to a downstream program.
This patch skips the fflush() calls when stdout is a regular file, or
if the environment variable GIT_NEVER_FLUSH_STDOUT is set. This
latter can speed up a command such as:
(export GIT_NEVER_FLUSH_STDOUT=t; git-rev-list HEAD | wc -l)
a tiny amount.
Cc: Linus Torvalds [off-list ref]
Signed-off-by: "Theodore Ts'o" [off-list ref]
From: Jeff King <hidden> Date: 2016-06-15 22:43:18
On Thu, Jun 28, 2007 at 03:04:06PM -0400, Theodore Tso wrote:
This patch skips the fflush() calls when stdout is a regular file, or
if the environment variable GIT_NEVER_FLUSH_STDOUT is set. This
latter can speed up a command such as:
(export GIT_NEVER_FLUSH_STDOUT=t; git-rev-list HEAD | wc -l)
I wonder if this would be more natural in the opposite form:
GIT_FLUSH_STDOUT=1 git-rev-list HEAD
In general, you don't want to do the flushing unless:
- it's going to the pager
- some program is reading incrementally
In the first case, we can just turn on GIT_FLUSH_STDOUT when we kick off
the pager. In the second case, that program can just add the variable to
its invocation.
On top of which, in your patch the type of output trumps the environment
variable, which seems backwards. In other words, I can't do this:
GIT_FLUSH_EVEN_THOUGH_ITS_A_FILE=1 git-rev-list HEAD >file
[in another window] tail -f file
I would think an explicit preference from a variable should override any
guesses.
-Peff
On Thu, Jun 28, 2007 at 05:34:51PM -0400, Jeff King wrote:
On top of which, in your patch the type of output trumps the environment
variable, which seems backwards. In other words, I can't do this:
GIT_FLUSH_EVEN_THOUGH_ITS_A_FILE=1 git-rev-list HEAD >file
[in another window] tail -f file
I would think an explicit preference from a variable should override any
guesses.
Good point. Here's a revised patch where GIT_FLUSH=0 and GIT_FLASH=1
trumps any hueristics.
My comments about this making only trivial differences on Linux still
apply (although it does make things slightly more optimal). I suspect
it might make more of a difference on MacOS, but I haven't had time to
benchmark it.
- Ted
---
commit 422becc0f8430d2386ceed92f224a94c9047751e
Author: Theodore Ts'o [off-list ref]
Date: Thu Jun 28 14:10:58 2007 -0400
Don't fflush(stdout) when it's not helpful
This patch arose from a discussion started by Jim Meyering's patch
whose intention was to provide better diagnostics for failed writes.
Linus proposed a better way to do things, which also had the added
benefit that adding a fflush() to git-log-* operations and incremental
git-blame operations could improve interactive respose time feel, at
the cost of making things a bit slower when we aren't piping the
output to a downstream program.
This patch skips the fflush() calls when stdout is a regular file, or
if the environment variable GIT_FLUSH is set to "0". This latter can
speed up a command such as:
GIT_FLUSH=0 strace -c -f -e write time git-rev-list HEAD | wc -l
a tiny amount.
Cc: Linus Torvalds [off-list ref]
Signed-off-by: "Theodore Ts'o" [off-list ref]
@@ -396,6 +396,16 @@ other 'GIT_PAGER':: This environment variable overrides `$PAGER`.+'GIT_FLUSH'::+ If this environment variable is set to "1", then commands such+ as git-blame (in incremental mode), git-rev-list, git-log,+ git-whatchanged, etc., will force a flush of the output stream+ after each commit-oriented record have been flushed. If this+ variable is set to "0", the output of these commands will be done+ using completely buffered I/O. If this environment variable is+ not set, git will choose buffered or record-oriented flushing+ based on whether stdout appears to be redirected to a file or not.+ 'GIT_TRACE':: If this variable is set to "1", "2" or "true" (comparison is case insensitive), git will print `trace:` messages on
Any particular reason why you still check for GIT_NEVER_FLUSH_STDOUT in
addition to GIT_FLUSH?
Gruesse,
--
Frank Lichtenheld [off-list ref]
www: http://www.djpig.de/
On Fri, Jun 29, 2007 at 03:05:08AM +0200, Frank Lichtenheld wrote:
Any particular reason why you still check for GIT_NEVER_FLUSH_STDOUT in
addition to GIT_FLUSH?
Yup, I forgot to remove it. :-)
- Ted
---
commit 473065f89f27de476d12d774141009cd4f2600c4
Author: Theodore Ts'o [off-list ref]
Date: Thu Jun 28 14:10:58 2007 -0400
Don't fflush(stdout) when it's not helpful
This patch arose from a discussion started by Jim Meyering's patch
whose intention was to provide better diagnostics for failed writes.
Linus proposed a better way to do things, which also had the added
benefit that adding a fflush() to git-log-* operations and incremental
git-blame operations could improve interactive respose time feel, at
the cost of making things a bit slower when we aren't piping the
output to a downstream program.
This patch skips the fflush() calls when stdout is a regular file, or
if the environment variable GIT_FLUSH is set to "0". This latter can
speed up a command such as:
GIT_FLUSH=0 strace -c -f -e write time git-rev-list HEAD | wc -l
a tiny amount.
Cc: Linus Torvalds [off-list ref]
Signed-off-by: "Theodore Ts'o" [off-list ref]
@@ -396,6 +396,16 @@ other 'GIT_PAGER':: This environment variable overrides `$PAGER`.+'GIT_FLUSH'::+ If this environment variable is set to "1", then commands such+ as git-blame (in incremental mode), git-rev-list, git-log,+ git-whatchanged, etc., will force a flush of the output stream+ after each commit-oriented record have been flushed. If this+ variable is set to "0", the output of these commands will be done+ using completely buffered I/O. If this environment variable is+ not set, git will choose buffered or record-oriented flushing+ based on whether stdout appears to be redirected to a file or not.+ 'GIT_TRACE':: If this variable is set to "1", "2" or "true" (comparison is case insensitive), git will print `trace:` messages on
Looks much better to me, but I have one minor nit: stdout_is_file is a
poor name, since it can mean either that stdout is a file, or that
flushing was explicitly turned off. Naming it something like
stdout_wants_flush would make much more sense. Though it's not a huge
deal since the function is fairly short, I think it makes things a
little easier to read (I had to double-check the negation on atoi(cp) ==
0 before I convinced myself the code was correct).
-Peff