From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Date: Tue, 07 Apr 2015 22:43:09 GMT
Subject: Re: [PATCH 3/6] strbuf_getwholeline: use getc_unlocked
Message-ID: <87k2xn8sqq.fsf@rasmusvillemoes.dk>
In-Reply-To: <20150407190413.GB2553@peff.net>

On Tue, Apr 07 2015, Jeff King <peff@peff.net> wrote:

> On Tue, Apr 07, 2015 at 03:48:33PM +0200, Rasmus Villemoes wrote:
>
>> 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.

Hm, I'm afraid it's not that simple. It seems that data may be lost from
the stream if getdelim encounters ENOMEM: Looking at the glibc
implementation (libio/iogetdelim.c), if reallocating the user buffer
fails, -1 is returned (presumably with errno==ENOMEM set by realloc), and
there's no way of knowing how many bytes were already copied to the
buffer (cur_len).

For regular files, I suppose one could do a ftell/fseek dance. For
other cases, I don't see a reliable way to retry upon ENOMEM.

Related thread on the posix mailing list:
http://thread.gmane.org/gmane.comp.standards.posix.austin.general/10091

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

Well, it's not really an intrusive patch, and 42% speedup is rather
significant. And, of course, the above ENOMEM issue may mean that
getdelim isn't usable after all.

Rasmus