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

On Tue, Apr 07 2015, Jeff King [off-list ref] wrote:

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

quoted

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

On Wed, Apr 08, 2015 at 12:43:09AM +0200, Rasmus Villemoes wrote:

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.

Ah, right, that makes sense.

I think it may be OK to just `die()` on ENOMEM here. The "try to free"
routine is basically about unmapping pack memory, in case it is clogging
up the address space (it is mmap'd, so it shouldn't cause real RAM
pressure; the OS can just drop the pages from the cache if it wants).

But getdelim() calls are not likely to be big allocations in the first
place (so they are not likely to fail at all, and if they do, we really
are horribly out of memory). I'd also question whether
release_pack_memory is doing anything in practice on 64-bit machines. We
have quite a bit of address space to work with in that case.

-Peff