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

From: Jeff King <peff@peff.net>
Date: Apr 7, 2015, 19:04 UTC
Message-ID: <20150407190413.GB2553@peff.net>
In-Reply-To: <87zj6kjbgu.fsf@rasmusvillemoes.dk>

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

Show 11 quoted lines

> >   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