strbuf.c: optimize program logic

From: ZheNing Hu <[email protected]>

the usage in strbuf.h tell us"Alloc is somehow a
"private" member that should not be messed with.
use strbuf_avail()instead."

strbuf.c +3 -3

index e3397cc4c72..76f560a28d0 100644
@@ -517,7 +517,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
 	strbuf_grow(sb, hint ? hint : 8192);
 	for (;;) {
-		ssize_t want = sb->alloc - sb->len - 1;
+		ssize_t want = strbuf_avail(sb);
 		ssize_t got = read_in_full(fd, sb->buf + sb->len, want);
 
 		if (got < 0) {

@@ -527,7 +527,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 				strbuf_setlen(sb, oldlen);
 			return -1;
 		}
-		sb->len += got;
+		strbuf_setlen(sb, sb->len + got);
 		if (got < want)
 			break;
 		strbuf_grow(sb, 8192);

@@ -543,7 +543,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
 	ssize_t cnt;
 
 	strbuf_grow(sb, hint ? hint : 8192);
-	cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
+	cnt = xread(fd, sb->buf + sb->len, strbuf_avail(sb));
 	if (cnt > 0)
 		strbuf_setlen(sb, sb->len + cnt);
 	else if (oldalloc == 0)

base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707

Quoted 2 lines · 阿德烈 via GitGitGadget

From: ZheNing Hu <[email protected]>

Quoted 3 lines

the usage in strbuf.h tell us"Alloc is somehow a
"private" member that should not be messed with.
use strbuf_avail()instead."

When we use the word "private", it generally means it is private to the implementation of the API. IOW, it is usually fine for the implementation of the API (i.e. for strbuf API, what you see in strbuf.c) to use private members.

In any case, these changes are _not_ optimizations.

Replacing (alloc - len - 1) with strbuf_avail() is at best an equivalent rewrite (which is a good thing from readability's point of view, but not an optimization). We know sb->alloc during the loop is never 0, but the compiler may miss the fact, so the inlined implementation of _avail, i.e.

        static inline size_t strbuf_avail(const struct strbuf *sb)
        {
                return sb->alloc ? sb->alloc - sb->len - 1 : 0;
        }

may not incur call overhead, but may be pessimizing the executed code.

If you compare the code in the loop in the second hunk below with what _setlen() does, I think you'll see the overhead of _setlen() relative to the original code is even higher, so it may also be pessimizing, not optimizing.

So, overall, I am not all that enthused to see this patch.

One thing I noticed is that, whether open coded like sb->len += got or made into parameter to strbuf_setlen(sb, sb->len + got), we are not careful about sb->len growing too large and overflowing with the addition. That may potentially be an interesting thing to look into, but at the same time, unlike the usual "compute the number of bytes we need to allocate and then call xmalloc()" pattern, where we try to be careful in the "compute" step by using st_add() macros, this code actually keep growing the buffer, so by the time the size_t overflows and wraps around, we'd certainly have exhausted the memory already, so it won't be an issue.

But we may want to audit existing code that is not careful when preparing the second parameter to strbuf_setlen(). We just analyzed, if we were to accept this patch, that "sb->len + got" that appear as the second parameter to new call of strbuf_setlen() looks bad but would not matter in practice, but we may not be so lucky in other places.

Thanks for a food for thought.

Quoted 33 lines

strbuf.c +3 -3

index e3397cc4c72..76f560a28d0 100644
@@ -517,7 +517,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
 
      strbuf_grow(sb, hint ? hint : 8192);
      for (;;) {
-             ssize_t want = sb->alloc - sb->len - 1;
+             ssize_t want = strbuf_avail(sb);
              ssize_t got = read_in_full(fd, sb->buf + sb->len, want);
 
              if (got < 0) {

@@ -527,7 +527,7 @@ ssize_t strbuf_read(struct strbuf *sb, int fd, size_t hint)
                              strbuf_setlen(sb, oldlen);
                      return -1;
              }
-             sb->len += got;
+             strbuf_setlen(sb, sb->len + got);
              if (got < want)
                      break;
              strbuf_grow(sb, 8192);

@@ -543,7 +543,7 @@ ssize_t strbuf_read_once(struct strbuf *sb, int fd, size_t hint)
      ssize_t cnt;
 
      strbuf_grow(sb, hint ? hint : 8192);
-     cnt = xread(fd, sb->buf + sb->len, sb->alloc - sb->len - 1);
+     cnt = xread(fd, sb->buf + sb->len, strbuf_avail(sb));
      if (cnt > 0)
              strbuf_setlen(sb, sb->len + cnt);
      else if (oldalloc == 0)

base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707

Quoted 9 lines · Junio C Hamano <[email protected]> 于2021年1月26日周二 下午2:17写道：

Quoted 5 lines · 阿德烈 via GitGitGadget

From: ZheNing Hu <[email protected]>

the usage in strbuf.h tell us"Alloc is somehow a
"private" member that should not be messed with.
use strbuf_avail()instead."

Quoted 5 lines

When we use the word "private", it generally means it is private to the implementation of the API. IOW, it is usually fine for the implementation of the API (i.e. for strbuf API, what you see in strbuf.c) to use private members.

Well, I just think most other functions in strbuf.c follow the use of strbuf_avail() instead of "sb->alloc-sb->len-1", and the "sb->alloc-sb->len-1" that appears in strbuf_read() is not so uniform.

Quoted 5 lines · 阿德烈 via GitGitGadget

From: ZheNing Hu <[email protected]>

the usage in strbuf.h tell us"Alloc is somehow a
"private" member that should not be messed with.
use strbuf_avail()instead."

When we use the word "private", it generally means it is private to the implementation of the API. IOW, it is usually fine for the implementation of the API (i.e. for strbuf API, what you see in strbuf.c) to use private members.

In any case, these changes are _not_ optimizations.

Replacing (alloc - len - 1) with strbuf_avail() is at best an equivalent rewrite (which is a good thing from readability's point of view, but not an optimization). We know sb->alloc during the loop is never 0, but the compiler may miss the fact, so the inlined implementation of _avail, i.e.

	static inline size_t strbuf_avail(const struct strbuf *sb)
	{
	        return sb->alloc ? sb->alloc - sb->len - 1 : 0;
        }

may not incur call overhead, but may be pessimizing the executed code.

If you compare the code in the loop in the second hunk below with what _setlen() does, I think you'll see the overhead of _setlen() relative to the original code is even higher, so it may also be pessimizing, not optimizing.

So, overall, I am not all that enthused to see this patch.

One thing I noticed is that, whether open coded like sb->len += got or made into parameter to strbuf_setlen(sb, sb->len + got), we are not careful about sb->len growing too large and overflowing with the addition. That may potentially be an interesting thing to look into, but at the same time, unlike the usual "compute the number of bytes we need to allocate and then call xmalloc()" pattern, where we try to be careful in the "compute" step by using st_add() macros, this code actually keep growing the buffer, so by the time the size_t overflows and wraps around, we'd certainly have exhausted the memory already, so it won't be an issue.

PS The original patch does not seem to have made it to the list for some
reason (I didn't get a copy, and neither did lore.kernel.org).

Quoted 8 lines · Junio C Hamano

Quoted 5 lines · 阿德烈 via GitGitGadget

From: ZheNing Hu <[email protected]>

the usage in strbuf.h tell us"Alloc is somehow a
"private" member that should not be messed with.
use strbuf_avail()instead."

Quoted 6 lines

When we use the word "private", it generally means it is private to the implementation of the API. IOW, it is usually fine for the implementation of the API (i.e. for strbuf API, what you see in strbuf.c) to use private members.

In any case, these changes are _not_ optimizations.

Yeah, I had both of those thoughts, too. :)

Though...

Quoted 14 lines

Replacing (alloc - len - 1) with strbuf_avail() is at best an equivalent rewrite (which is a good thing from readability's point of view, but not an optimization). We know sb->alloc during the loop is never 0, but the compiler may miss the fact, so the inlined implementation of _avail, i.e.

      static inline size_t strbuf_avail(const struct strbuf *sb)
      {
              return sb->alloc ? sb->alloc - sb->len - 1 : 0;
        }

may not incur call overhead, but may be pessimizing the executed code.

Quoted 6 lines

If you compare the code in the loop in the second hunk below with what _setlen() does, I think you'll see the overhead of _setlen() relative to the original code is even higher, so it may also be pessimizing, not optimizing.

So, overall, I am not all that enthused to see this patch.

I would generally value readability/consistency here over trying to micro-optimize an if-zero check.

However, if strbuf_avail() ever did return 0, I'm not sure the loop would make forward progress:

          strbuf_grow(sb, hint ? hint : 8192);
          for (;;) {
                  ssize_t want = strbuf_avail(sb);
                  ssize_t got = read_in_full(fd, sb->buf + sb->len, want);

                  if (got < 0) {
                          if (oldalloc == 0)
                                  strbuf_release(sb);
                          else
                                  strbuf_setlen(sb, oldlen);
                          return -1;
                  }
                  strbuf_setlen(sb, sb->len + got);
                  if (got < want)
                          break;
                  strbuf_grow(sb, 8192);
          }

we'd just ask to read 0 bytes over and over. That almost makes me want to add:

  if (!want)
        BUG("strbuf did not actually grow!?");

or possibly to teach the "if (got < want)" condition to check for a zero return (though I guess that would probably just end up confusing us into thinking we hit EOF).

Quoted 10 lines

One thing I noticed is that, whether open coded like sb->len += got or made into parameter to strbuf_setlen(sb, sb->len + got), we are not careful about sb->len growing too large and overflowing with the addition. That may potentially be an interesting thing to look into, but at the same time, unlike the usual "compute the number of bytes we need to allocate and then call xmalloc()" pattern, where we try to be careful in the "compute" step by using st_add() macros, this code actually keep growing the buffer, so by the time the size_t overflows and wraps around, we'd certainly have exhausted the memory already, so it won't be an issue.

I think "len" is OK here. An invariant of strbuf is that "len" is smaller than "alloc" for obvious reasons. So as long as the actual strbuf_grow() is safe, then extending "len".

I'm not sure that strbuf_grow() is safe, though. It relies on ALLOC_GROW, which does not use st_add(), etc.

-Peff

PS The original patch does not seem to have made it to the list for some
reason (I didn't get a copy, and neither did lore.kernel.org).

Quoted 3 lines

I'm not sure that strbuf_grow() is safe, though. It relies on ALLOC_GROW, which does not use st_add(), etc.

I want to say is that strbuf_grow() have checked overflow before
ALLOC_GROW,so that strbuf_grow()is maybe also safe too? :)
void strbuf_grow(struct strbuf *sb, size_t extra)

{
       int new_buf = !sb->alloc;
       if (unsigned_add_overflows(extra, 1) ||
           unsigned_add_overflows(sb->len, extra + 1))
               die("you want to use way too much memory");
       ...