# [PATCH] use strbuf_addbuf() for appending a strbuf to another 5 messages from 2016-07-19 to 2016-07-22. Participants: René Scharfe, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/42880 ## René Scharfe, 2016-07-19 18:36 Subject: [PATCH] use strbuf_addbuf() for appending a strbuf to another Message-ID: <578E732D.2090803@web.de> URL: https://gitlist.dev/e/578E732D.2090803%40web.de ``` Use strbuf_addbuf() where possible; it's shorter and more efficient. Signed-off-by: Rene Scharfe --- dir.c | 2 +- path.c | 2 +- wt-status.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dir.c b/dir.c index 6172b34..0ea235f 100644 --- a/dir.c +++ b/dir.c @@ -2364,7 +2364,7 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra varint_len = encode_varint(untracked->ident.len, varbuf); strbuf_add(out, varbuf, varint_len); - strbuf_add(out, untracked->ident.buf, untracked->ident.len); + strbuf_addbuf(out, &untracked->ident); strbuf_add(out, ouc, ouc_size(len)); free(ouc); diff --git a/path.c b/path.c index 259aeed..17551c4 100644 --- a/path.c +++ b/path.c @@ -483,7 +483,7 @@ static void do_submodule_path(struct strbuf *buf, const char *path, strbuf_addstr(buf, git_dir); } strbuf_addch(buf, '/'); - strbuf_addstr(&git_submodule_dir, buf->buf); + strbuf_addbuf(&git_submodule_dir, buf); strbuf_vaddf(buf, fmt, args); diff --git a/wt-status.c b/wt-status.c index c19b52c..dbdb543 100644 --- a/wt-status.c +++ b/wt-status.c @@ -1062,7 +1062,7 @@ static void abbrev_sha1_in_line(struct strbuf *line) strbuf_addf(split[1], "%s ", abbrev); strbuf_reset(line); for (i = 0; split[i]; i++) - strbuf_addf(line, "%s", split[i]->buf); + strbuf_addbuf(line, split[i]); } } strbuf_list_free(split); -- 2.9.2 ``` ## Jeff King, 2016-07-20 13:20 Subject: Re: [PATCH] use strbuf_addbuf() for appending a strbuf to another Message-ID: <20160720132025.GD17469@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160720132025.GD17469%40sigill.intra.peff.net In-Reply-To: <578E732D.2090803@web.de> ``` On Tue, Jul 19, 2016 at 08:36:29PM +0200, René Scharfe wrote: > Use strbuf_addbuf() where possible; it's shorter and more efficient. After seeing "efficient", I was momentarily surprised by the first hunk: > diff --git a/dir.c b/dir.c > index 6172b34..0ea235f 100644 > --- a/dir.c > +++ b/dir.c > @@ -2364,7 +2364,7 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra > > varint_len = encode_varint(untracked->ident.len, varbuf); > strbuf_add(out, varbuf, varint_len); > - strbuf_add(out, untracked->ident.buf, untracked->ident.len); > + strbuf_addbuf(out, &untracked->ident); This is actually slightly _less_ efficient, because we already are using the precomputed len, and the new code will call an extra strbuf_grow() to cover the case where the two arguments are the same. See 81d2cae (strbuf_addbuf(): allow passing the same buf to dst and src, 2010-01-12). But it almost certainly doesn't matter, and it definitely _is_ an improvement for the other "addstr" cases, which are doing an unncessary strlen(). And anyway the readability improvement trumps all of that in my mind. So I think overall it is a nice cleanup; I'm mostly just commenting for the sake of other reviewers. -Peff ``` ## René Scharfe, 2016-07-21 16:46 Subject: Re: [PATCH] use strbuf_addbuf() for appending a strbuf to another Message-ID: <5790FC74.6000602@web.de> URL: https://gitlist.dev/e/5790FC74.6000602%40web.de In-Reply-To: <20160720132025.GD17469@sigill.intra.peff.net> ``` Am 20.07.2016 um 15:20 schrieb Jeff King: > On Tue, Jul 19, 2016 at 08:36:29PM +0200, René Scharfe wrote: > >> Use strbuf_addbuf() where possible; it's shorter and more efficient. > > After seeing "efficient", I was momentarily surprised by the first hunk: > >> diff --git a/dir.c b/dir.c >> index 6172b34..0ea235f 100644 >> --- a/dir.c >> +++ b/dir.c >> @@ -2364,7 +2364,7 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra >> >> varint_len = encode_varint(untracked->ident.len, varbuf); >> strbuf_add(out, varbuf, varint_len); >> - strbuf_add(out, untracked->ident.buf, untracked->ident.len); >> + strbuf_addbuf(out, &untracked->ident); > > This is actually slightly _less_ efficient, because we already are using > the precomputed len, and the new code will call an extra strbuf_grow() > to cover the case where the two arguments are the same. See 81d2cae > (strbuf_addbuf(): allow passing the same buf to dst and src, > 2010-01-12). Ah, I wasn't aware of that. Calling strbuf_grow() twice shouldn't be thaaat bad. However, I wonder where we duplicate strbufs, or why we would ever want to do so. Anyway, here's a patch for that: -- >8 -- Subject: strbuf: avoid calling strbuf_grow() twice in strbuf_addbuf() Implement strbuf_addbuf() as a normal function in order to avoid calling strbuf_grow() twice, with the second callinside strbud_add() being a no-op. This is slightly faster and also reduces the text size a bit. Signed-off-by: Rene Scharfe --- strbuf.c | 7 +++++++ strbuf.h | 6 +----- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/strbuf.c b/strbuf.c index 1ba600b..f3bd571 100644 --- a/strbuf.c +++ b/strbuf.c @@ -197,6 +197,13 @@ void strbuf_add(struct strbuf *sb, const void *data, size_t len) strbuf_setlen(sb, sb->len + len); } +void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) +{ + strbuf_grow(sb, sb2->len); + memcpy(sb->buf + sb->len, sb2->buf, sb2->len); + strbuf_setlen(sb, sb->len + sb2->len); +} + void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len) { strbuf_grow(sb, len); diff --git a/strbuf.h b/strbuf.h index 83c5c98..ba8d5f1 100644 --- a/strbuf.h +++ b/strbuf.h @@ -263,11 +263,7 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) /** * Copy the contents of another buffer at the end of the current one. */ -static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) -{ - strbuf_grow(sb, sb2->len); - strbuf_add(sb, sb2->buf, sb2->len); -} +extern void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2); /** * Copy part of the buffer from a given position till a given length to the -- 2.9.2 ``` ## Jeff King, 2016-07-21 21:02 Subject: Re: [PATCH] use strbuf_addbuf() for appending a strbuf to another Message-ID: <20160721210253.GA4604@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160721210253.GA4604%40sigill.intra.peff.net In-Reply-To: <5790FC74.6000602@web.de> ``` On Thu, Jul 21, 2016 at 06:46:44PM +0200, René Scharfe wrote: > >> - strbuf_add(out, untracked->ident.buf, untracked->ident.len); > >> + strbuf_addbuf(out, &untracked->ident); > > > > This is actually slightly _less_ efficient, because we already are using > > the precomputed len, and the new code will call an extra strbuf_grow() > > to cover the case where the two arguments are the same. See 81d2cae > > (strbuf_addbuf(): allow passing the same buf to dst and src, > > 2010-01-12). > > Ah, I wasn't aware of that. Calling strbuf_grow() twice shouldn't be > thaaat bad. However, I wonder where we duplicate strbufs, or why we > would ever want to do so. Anyway, here's a patch for that: I don't think we do. Going back to the original discussion: http://thread.gmane.org/gmane.comp.version-control.git/136141/focus=136774 it was mostly just "hey, this would fail really confusingly if we ever did, so let's make it safe". The second strbuf_grow() is by definition a noop (which is why 81d2cae works at all), but we do pay the size-computation cost. > -- >8 -- > Subject: strbuf: avoid calling strbuf_grow() twice in strbuf_addbuf() > > Implement strbuf_addbuf() as a normal function in order to avoid calling > strbuf_grow() twice, with the second callinside strbud_add() being a > no-op. This is slightly faster and also reduces the text size a bit. Seems reasonable. IMHO the main advantage is that one does not have to reason about the double strbuf_grow() (i.e., that the strbuf_add() is safe because we know its grow is a noop). -Peff ``` ## Junio C Hamano, 2016-07-22 16:22 Subject: Re: [PATCH] use strbuf_addbuf() for appending a strbuf to another Message-ID: URL: https://gitlist.dev/e/xmqqr3albq2s.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160721210253.GA4604@sigill.intra.peff.net> ``` Jeff King writes: > I don't think we do. Going back to the original discussion: > > http://thread.gmane.org/gmane.comp.version-control.git/136141/focus=136774 > > it was mostly just "hey, this would fail really confusingly if we ever > did, so let's make it safe". > > The second strbuf_grow() is by definition a noop (which is why 81d2cae > works at all), but we do pay the size-computation cost. Both true. If the second one is not a noop, nothing is fixed by 81d2cae at all, but it is subtle. René's update makes it far easier to understand what is going on. >> -- >8 -- >> Subject: strbuf: avoid calling strbuf_grow() twice in strbuf_addbuf() >> >> Implement strbuf_addbuf() as a normal function in order to avoid calling >> strbuf_grow() twice, with the second callinside strbud_add() being a >> no-op. This is slightly faster and also reduces the text size a bit. > > Seems reasonable. IMHO the main advantage is that one does not have to > reason about the double strbuf_grow() (i.e., that the strbuf_add() is > safe because we know its grow is a noop). > > -Peff ```