From: Junio C Hamano <hidden> Date: 2021-01-26 06:26:31
"阿德烈 via GitGitGadget" [off-list ref] writes:
From: ZheNing Hu <redacted>
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.
Junio C Hamano [off-list ref] 于2021年1月26日周二 下午2:17写道：
"阿德烈 via GitGitGadget" [off-list ref] writes:
quoted
From: ZheNing Hu <redacted>
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.
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.
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.
I agree,It may be a good practice not to use redundant inline functions,
because it will not make the git binary file too bloated.
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.
This is true, but is there any good way to avoid this form of overflow?
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.
I thought before `strbuf_read_once()`have almost analogous
"strbuf_setlen(sb, sb->len + cnt)",so I change it.May be you are right,
"sb->len + got"is not safe.
From: Jeff King <hidden> Date: 2021-01-26 22:19:27
On Mon, Jan 25, 2021 at 10:17:41PM -0800, Junio C Hamano wrote:
"阿德烈 via GitGitGadget" [off-list ref] writes:
quoted
From: ZheNing Hu <redacted>
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.
Yeah, I had both of those thoughts, too. :)
Though...
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.
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).
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).
On Mon, Jan 25, 2021 at 10:17:41PM -0800, Junio C Hamano wrote:
quoted
"阿德烈 via GitGitGadget" [off-list ref] writes:
quoted
From: ZheNing Hu <redacted>
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.
Yeah, I had both of those thoughts, too. :)
Though...
quoted
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.
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).
I agree.But I always feel that there will be no such strange bug.
quoted
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.
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");
...
}
-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).
From: Jeff King <hidden> Date: 2021-01-30 09:08:48
On Fri, Jan 29, 2021 at 02:09:12PM +0800, 胡哲宁 wrote:
quoted
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");
...
Oh, you're right. I misread it as checking only "extra", but of course
the second line there is making sure our total requested size does not
overflow.
I do think ALLOC_GROW() could still overflow internally as it sizes
larger than sb->len + extra. But this is quite unlikely on a 64-bit
system, as it would imply we're using on the order of 2^63 bytes of RAM
before we enter the function. It potentially could be a problem on a
32-bit system, though I'm not sure how much in practice (the numbers are
small enough to be reasonable, but I'm not sure it's realistic that a
single strbuf could already be using half of the available address
space).
-Peff