Re: [PATCH 3/6] strbuf_getwholeline: use getc_unlocked

On Sun, Apr 05 2015, Jeff King [off-list ref] wrote:

which is back to the v2.0.0 number. Even with the extra strlen, it seems
that what fgets does internally beats repeated getc calls. Which I guess
is not too surprising, as each getc() will have to check for underflow
in the buffer. Perhaps there is more room to micro-optimize
strbuf_getwholeline, but I kind of doubt it.

The big downside is that our input strings are no longer NUL-clean
(reading "foo\0bar\n" would yield just "foo". I doubt that matters in
the real world, but it does fail a few of the tests (e.g., t7008 tries
to read a list of patterns which includes NUL, and we silently truncate
the pattern rather than read in the NUL and barf).

So we'd have to either:

  1. Decide that doesn't matter.

  2. Have callers specify a "damn the NULs, I want it fast" flag.

  3. Find some alternative that is more robust than fgets, and faster
     than getc. I don't think there is anything in stdio, but I am not
     above dropping in a faster non-portable call if it is available,
     and then falling back to the current code otherwise.

getdelim is in POSIX 2008
(http://pubs.opengroup.org/stage7tc1/functions/getdelim.html), so should
be available on any half-{d,r}ecent platform. It obviously has the
advantage of having access to the internal stdio buffer, and by
definition handles embedded NULs. No idea if using such modern
interfaces in git is ok, though. 

Implementation-wise, I think strbuf_getwholeline could be implemented
mostly as a simple wrapper for getdelim. If I'm reading the current code
and the posix spec for getdelim correctly, something like this should do
it (though obviously not meant to be included as-is):

diff --git a/strbuf.c b/strbuf.c
index 0346e74..d3338b9 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -434,6 +434,32 @@ int strbuf_getcwd(struct strbuf *sb)
 	return -1;
 }
 
+#if USE_GETDELIM
+/* Hacky declaration to avoid messing with feature test macros. */
+ssize_t getdelim(char **, size_t *, int, FILE *);
+int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
+{
+	ssize_t r;
+
+	if (feof(fp))
+		return EOF;
+
+	strbuf_reset(sb);
+	if (!sb->alloc)
+		sb->buf = NULL;
+
+	r = getdelim(&sb->buf, &sb->alloc, term, fp);
+
+	if (r > 0) {
+		sb->len = r;
+		return 0;
+	}
+	assert(r == -1);
+	if (!sb->buf)
+		strbuf_init(sb);
+	return EOF;
+}
+#else
 int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 {
 	int ch;

@@ -454,6 +480,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	sb->buf[sb->len] = '\0';
 	return 0;
 }
+#endif
 
 int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
 {

Rasmus

On Tue, Apr 07, 2015 at 03:48:33PM +0200, Rasmus Villemoes wrote:

quoted

  3. Find some alternative that is more robust than fgets, and faster
     than getc. I don't think there is anything in stdio, but I am not
     above dropping in a faster non-portable call if it is available,
     and then falling back to the current code otherwise.

getdelim is in POSIX 2008
(http://pubs.opengroup.org/stage7tc1/functions/getdelim.html), so should
be available on any half-{d,r}ecent platform. It obviously has the
advantage of having access to the internal stdio buffer, and by
definition handles embedded NULs. No idea if using such modern
interfaces in git is ok, though.

Thanks, that's perfect. I knew about getline(), but not getdelim(), and
I had thought getline() unconditionally malloc'd. But it doesn't; it
behaves exactly as we are already doing here. :-/

Implementation-wise, I think strbuf_getwholeline could be implemented
mostly as a simple wrapper for getdelim. If I'm reading the current code
and the posix spec for getdelim correctly, something like this should do
it (though obviously not meant to be included as-is):

I think it's close to what we want. strbuf_grow calls xrealloc, which
will call try_to_free_routine() and possibly die() for us. So we would
probably want to check errno on failure and respond similarly if we get
ENOMEM.

Your patch performs even faster than my fgets version (about 8%).

I wonder if it is even worth doing the getc_unlocked dance at all. It
would potentially speed up the fallback code, but my hope that would be
that most systems would simply use the getdelim() version.

-Peff