| Message ID | c7f22d8d-10f8-0c97-672e-0a50182901e0@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | name-rev: improve memory usage | expand |
On 2/4/2020 4:24 PM, René Scharfe wrote: > We can calculate the size of new name easily and precisely. Open-code > the xstrfmt() calls and grow the buffers as needed before filling them. > This provides a surprisingly large benefit when working with the > Chromium repository; here are the numbers measured using hyperfine > before: > > Benchmark #1: ./git -C ../chromium/src name-rev --all > Time (mean ± σ): 5.822 s ± 0.013 s [User: 5.304 s, System: 0.516 s] > Range (min … max): 5.803 s … 5.837 s 10 runs > > ... and with this patch: > > Benchmark #1: ./git -C ../chromium/src name-rev --all > Time (mean ± σ): 1.527 s ± 0.003 s [User: 1.015 s, System: 0.511 s] > Range (min … max): 1.524 s … 1.535 s 10 runs Nice! > + if (name->generation > 0) { > + strbuf_grow(&sb, len + > + 1 + decimal_width(name->generation) + > + 1 + decimal_width(parent_number)); Just curious: these strbuf_grow() calls are what _really_ improve the performance, right? If you dropped them, then can we expect the performance to be similar to the old code? > + strbuf_addf(&sb, "%.*s~%d^%d", (int)len, name->tip_name, > + name->generation, parent_number); > + } else { > + strbuf_grow(&sb, len + > + 1 + decimal_width(parent_number)); > + strbuf_addf(&sb, "%.*s^%d", (int)len, name->tip_name, > + parent_number); > + } > + return strbuf_detach(&sb, NULL); > } > > static void name_rev(struct commit *start_commit, > -- > 2.25.0 >
Am 05.02.20 um 04:19 schrieb Derrick Stolee: > On 2/4/2020 4:24 PM, René Scharfe wrote: >> We can calculate the size of new name easily and precisely. Open-code >> the xstrfmt() calls and grow the buffers as needed before filling them. >> This provides a surprisingly large benefit when working with the >> Chromium repository; here are the numbers measured using hyperfine >> before: >> >> Benchmark #1: ./git -C ../chromium/src name-rev --all >> Time (mean ± σ): 5.822 s ± 0.013 s [User: 5.304 s, System: 0.516 s] >> Range (min … max): 5.803 s … 5.837 s 10 runs >> >> ... and with this patch: >> >> Benchmark #1: ./git -C ../chromium/src name-rev --all >> Time (mean ± σ): 1.527 s ± 0.003 s [User: 1.015 s, System: 0.511 s] >> Range (min … max): 1.524 s … 1.535 s 10 runs > > Nice! > >> + if (name->generation > 0) { >> + strbuf_grow(&sb, len + >> + 1 + decimal_width(name->generation) + >> + 1 + decimal_width(parent_number)); > > Just curious: these strbuf_grow() calls are what _really_ improve the > performance, right? If you dropped them, then can we expect the performance > to be similar to the old code? The replaced xstrfmt() is defined in strbuf.c and trivially wraps xstrvfmt(), which in turn does: struct strbuf buf = STRBUF_INIT; strbuf_vaddf(&buf, fmt, ap); return strbuf_detach(&buf, NULL); And strbuf_addf() trivially wraps strbuf_vaddf(). So indeed, all I did was to inline xstrfmt() and add the strbuf_grow() calls. Their high impact shocked me a bit. vsnprintf(3) really doesn't seem to like working with a buffer that is too small (when it's supposed to just calculate how many bytes it would have needed), at least not in the one from glibc 2.29. >> + strbuf_addf(&sb, "%.*s~%d^%d", (int)len, name->tip_name, >> + name->generation, parent_number); >> + } else { >> + strbuf_grow(&sb, len + >> + 1 + decimal_width(parent_number)); >> + strbuf_addf(&sb, "%.*s^%d", (int)len, name->tip_name, >> + parent_number); >> + } >> + return strbuf_detach(&sb, NULL); >> } >> >> static void name_rev(struct commit *start_commit, >> -- >> 2.25.0 >> >
diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 6701fb1569..793356edd1 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -103,15 +103,23 @@ static struct rev_name *create_or_update_name(struct commit *commit, static char *get_parent_name(const struct rev_name *name, int parent_number) { + struct strbuf sb = STRBUF_INIT; size_t len; strip_suffix(name->tip_name, "^0", &len); - if (name->generation > 0) - return xstrfmt("%.*s~%d^%d", (int)len, name->tip_name, - name->generation, parent_number); - else - return xstrfmt("%.*s^%d", (int)len, name->tip_name, - parent_number); + if (name->generation > 0) { + strbuf_grow(&sb, len + + 1 + decimal_width(name->generation) + + 1 + decimal_width(parent_number)); + strbuf_addf(&sb, "%.*s~%d^%d", (int)len, name->tip_name, + name->generation, parent_number); + } else { + strbuf_grow(&sb, len + + 1 + decimal_width(parent_number)); + strbuf_addf(&sb, "%.*s^%d", (int)len, name->tip_name, + parent_number); + } + return strbuf_detach(&sb, NULL); } static void name_rev(struct commit *start_commit,
We can calculate the size of new name easily and precisely. Open-code the xstrfmt() calls and grow the buffers as needed before filling them. This provides a surprisingly large benefit when working with the Chromium repository; here are the numbers measured using hyperfine before: Benchmark #1: ./git -C ../chromium/src name-rev --all Time (mean ± σ): 5.822 s ± 0.013 s [User: 5.304 s, System: 0.516 s] Range (min … max): 5.803 s … 5.837 s 10 runs ... and with this patch: Benchmark #1: ./git -C ../chromium/src name-rev --all Time (mean ± σ): 1.527 s ± 0.003 s [User: 1.015 s, System: 0.511 s] Range (min … max): 1.524 s … 1.535 s 10 runs Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/name-rev.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) -- 2.25.0