threads / patch / 39469

patchstrbuf_read: skip unnecessary strbuf_grow at eof

Subject: [PATCH] strbuf_read: skip unnecessary strbuf_grow at eof

## tl;dr

3 messages between May 31, 2015 and Jun 1, 2015. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Jim Hill· May 31, 2015, 18:16 UTC · lore

Make strbuf_read not try to do read_in_full's job too. If xread returns less than was requested it can be either eof or an interrupted read. If read_in_full returns less than was requested, it's eof. Use read_in_full to detect eof and not iterate when eof has been seen.

Signed-off-by: Jim Hill <gjthill@gmail.com>
---
 strbuf.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Show changes to strbuf.c +5 −5

diff --git a/strbuf.c b/strbuf.c
index 88cafd4..c70733e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -364,19 +364,19 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 	strbuf_grow(sb, hint ? hint : 8192);
 	for (;;) {
-		ssize_t cnt;
+		ssize_t want = sb->alloc - sb->len - 1;
+		ssize_t got = read_in_full(fd, sb->buf + sb->len, want);
 
-		cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
-		if (cnt < 0) {
+		if (got < 0) {
 			if (oldalloc == 0)
 				strbuf_release(sb);
 			else
 				strbuf_setlen(sb, oldlen);
 			return -1;
 		}
-		if (!cnt)
+		sb->len += got;
+		if (got < want)
 			break;
-		sb->len += cnt;
 		strbuf_grow(sb, 8192);
 	}

-- 
2.4.1.4.gfc728c2

Jeff King· Jun 1, 2015, 10:59 UTC · re: Jim Hill · lore

Re: [PATCH] strbuf_read: skip unnecessary strbuf_grow at eof

On Sun, May 31, 2015 at 11:16:45AM -0700, Jim Hill wrote:

> Make strbuf_read not try to do read_in_full's job too.  If xread returns
> less than was requested it can be either eof or an interrupted read.  If
> read_in_full returns less than was requested, it's eof. Use read_in_full
> to detect eof and not iterate when eof has been seen.

I think this makes sense. I somehow had to read this over several times to understand that the main point is not the cleanup, but rather the space savings from not doing an extra strbuf_grow. Perhaps it is because the main idea is mentioned only in the subject. Or perhaps I was just being dense.

-Peff

Junio C Hamano· Jun 1, 2015, 16:23 UTC · re: Jeff King · lore

Re: [PATCH] strbuf_read: skip unnecessary strbuf_grow at eof

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

> On Sun, May 31, 2015 at 11:16:45AM -0700, Jim Hill wrote:
>
>> Make strbuf_read not try to do read_in_full's job too.  If xread returns
>> less than was requested it can be either eof or an interrupted read.  If
>> read_in_full returns less than was requested, it's eof. Use read_in_full
>> to detect eof and not iterate when eof has been seen.
>
> I think this makes sense. I somehow had to read this over several times
> to understand that the main point is not the cleanup, but rather the
> space savings from not doing an extra strbuf_grow. Perhaps it is because
> the main idea is mentioned only in the subject. Or perhaps I was just
> being dense.

Even after seeing (not "reading", as it was before my first cup of caffeine ;-) this message 30 minutes ago and then reading the patch with a fresh eye, I had the same impression, until I realized there is "too" at the end of the first sentence.

Perhaps

	The loop in strbuf_read() uses xread() repeatedly while
	extending the strbuf until the call returns zero.  If the
	buffer is sufficiently large to begin with, this results in
	xread() returning the remainder of the file to the end
	(returning non-zero), the loop extending the strbuf, and
	then making another call to xread() to have it return zero.

	By using read_in_full(), we can tell when the read reached
	the end of file: when it returns less than was requested,
	it's eof.  This way we can avoid an extra iteration that
	allocates an extra 8kB that is never used.

In any case, the change is very sensible.  Thanks, both.