diff mbox series

[v2,5/5] notes: use strbuf_attach to take ownership of the object contents

Message ID 20240218195938.6253-6-maarten.bosmans@vortech.nl (mailing list archive)
State New, archived
Headers show
Series Speed up git-notes show | expand

Commit Message

Maarten Bosmans Feb. 18, 2024, 7:59 p.m. UTC
From: Maarten Bosmans <mkbosmans@gmail.com>

From: Maarten Bosmans <maarten.bosmans@vortech.nl>

Avoid an extra allocation in the strbuf when pushing the string into it.

Signed-off-by: Maarten Bosmans <maarten.bosmans@vortech.nl>
---
 builtin/notes.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Jeff King Feb. 20, 2024, 2:12 a.m. UTC | #1
On Sun, Feb 18, 2024 at 08:59:38PM +0100, Maarten Bosmans wrote:

> @@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>  		if (!prev_buf)
>  			die(_("unable to read %s"), oid_to_hex(note));
>  		if (size)
> -			strbuf_add(&buf, prev_buf, size);
> +			strbuf_attach(&buf, prev_buf, size, size + 1);
>  		if (d.buf.len && size)
>  			append_separator(&buf);
>  		strbuf_insert(&d.buf, 0, buf.buf, buf.len);
>  
> -		free(prev_buf);
>  		strbuf_release(&buf);
>  	}

Is it possible for "size" to be 0, but prev_buf to be non-NULL? I assume
it is so if the previous note is the empty object (and anyway, we'd have
died earlier if prev_buf was NULL). In that case your patch introduces a
leak (we do not attach prev_buf to buf, but we no longer free prev_buf).

I'm a little skeptical that this is actually increasing the speed of the
command in a measurable way, though. It's one allocation/copy, right
next to a big old strbuf_insert() that is going to splice into an
existing array.

-Peff
Maarten Bosmans Feb. 20, 2024, 7:42 a.m. UTC | #2
Op di 20 feb 2024 om 03:12 schreef Jeff King <peff@peff.net>:
>
> On Sun, Feb 18, 2024 at 08:59:38PM +0100, Maarten Bosmans wrote:
>
> > @@ -705,12 +703,11 @@ static int append_edit(int argc, const char **argv, const char *prefix)
> >               if (!prev_buf)
> >                       die(_("unable to read %s"), oid_to_hex(note));
> >               if (size)
> > -                     strbuf_add(&buf, prev_buf, size);
> > +                     strbuf_attach(&buf, prev_buf, size, size + 1);
> >               if (d.buf.len && size)
> >                       append_separator(&buf);
> >               strbuf_insert(&d.buf, 0, buf.buf, buf.len);
> >
> > -             free(prev_buf);
> >               strbuf_release(&buf);
> >       }
>
> Is it possible for "size" to be 0, but prev_buf to be non-NULL? I assume
> it is so if the previous note is the empty object (and anyway, we'd have
> died earlier if prev_buf was NULL). In that case your patch introduces a
> leak (we do not attach prev_buf to buf, but we no longer free prev_buf).

You are right. I think the `if (size)` is not needed and removing it
would remove the potential for a leak.

> I'm a little skeptical that this is actually increasing the speed of the
> command in a measurable way, though. It's one allocation/copy, right
> next to a big old strbuf_insert() that is going to splice into an
> existing array.

Yeah, I was doubting this patch a bit too. The simple idiom of
starting with an empty strbuf and appending strings to it seems pretty
nice and clear, so may be there's value in leaving it at that.
The speed increase is not measurable of course. I was simply operating
in full on lets-eliminate-all-sources-of-overhead mode while profiling
the notes show code.

I'll drop the patch, in order to keep focus in this series.

Maarten
diff mbox series

Patch

diff --git a/builtin/notes.c b/builtin/notes.c
index 6863935d03..5be24a9c58 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -314,10 +314,8 @@  static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 	if (type != OBJ_BLOB)
 		die(_("cannot read note data from non-blob object '%s'."), arg);
 
-	strbuf_add(&msg->buf, value, len);
-	free(value);
+	strbuf_attach(&msg->buf, value, len, len + 1);
 
-	msg->buf.len = len;
 	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
 	d->messages[d->msg_nr - 1] = msg;
 	msg->stripspace = NO_STRIPSPACE;
@@ -705,12 +703,11 @@  static int append_edit(int argc, const char **argv, const char *prefix)
 		if (!prev_buf)
 			die(_("unable to read %s"), oid_to_hex(note));
 		if (size)
-			strbuf_add(&buf, prev_buf, size);
+			strbuf_attach(&buf, prev_buf, size, size + 1);
 		if (d.buf.len && size)
 			append_separator(&buf);
 		strbuf_insert(&d.buf, 0, buf.buf, buf.len);
 
-		free(prev_buf);
 		strbuf_release(&buf);
 	}