| Message ID | 20240218195938.6253-6-maarten.bosmans@vortech.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Speed up git-notes show | expand |
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
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 --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); }