René Scharfe <l.s.r@xxxxxx> writes:
> 1c56fc2084 (name-rev: pre-size buffer in get_parent_name(), 2020-02-04)
> got a big performance boost in an unusual repository by calculating the
> name length in advance. This is a bit awkward, as it references the
> name components twice.
>
> Use a memory pool to store the strings for the struct rev_name member
> tip_name. Using mem_pool_strfmt() allows efficient allocation without
> explicit size calculation. This simplifies the formatting part of the
> code without giving up performance:
>
> Benchmark 1: ./git_2.44.0 -C ../chromium/src name-rev --all
> Time (mean ± σ): 1.231 s ± 0.013 s [User: 1.082 s, System: 0.136 s]
> Range (min … max): 1.214 s … 1.252 s 10 runs
>
> Benchmark 2: ./git -C ../chromium/src name-rev --all
> Time (mean ± σ): 1.220 s ± 0.020 s [User: 1.083 s, System: 0.130 s]
> Range (min … max): 1.197 s … 1.254 s 10 runs
>
> Don't bother discarding the memory pool just before exiting. The effort
> for that would be very low, but actually measurable in the above
> example, with no benefit to users. At least UNLEAK it to calm down leak
> checkers. This addresses the leaks that 45a14f578e (Revert "name-rev:
> release unused name strings", 2022-04-22) brought back.
>
> Signed-off-by: René Scharfe <l.s.r@xxxxxx>
> ---
> This doesn't make any test script leak-free, though.
Hmph, is the root cause of the leak because no sensible ownership
rules are applied to .tip_name? Sometimes it is allocated for the
paritcular rev_name, but some other times the pointer is copied from
another rev_name.tip_name? As the way currently the code uses the
.tip_name member seems to be "allocate and use without any freeing",
I tend to agree that throwing them into mem-pool does make sense.
> builtin/name-rev.c | 39 ++++++++++++++++++++-------------------
> 1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 2dd1807c4e..ad9930c831 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -15,6 +15,7 @@
> #include "commit-slab.h"
> #include "commit-graph.h"
> #include "wildmatch.h"
> +#include "mem-pool.h"
>
> /*
> * One day. See the 'name a rev shortly after epoch' test in t6120 when
> @@ -155,30 +156,25 @@ static struct rev_name *create_or_update_name(struct commit *commit,
> return name;
> }
>
> -static char *get_parent_name(const struct rev_name *name, int parent_number)
> +static char *get_parent_name(const struct rev_name *name, int parent_number,
> + struct mem_pool *string_pool)
> {
> - struct strbuf sb = STRBUF_INIT;
> size_t len;
>
> strip_suffix(name->tip_name, "^0", &len);
> 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);
> + return mem_pool_strfmt(string_pool, "%.*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 mem_pool_strfmt(string_pool, "%.*s^%d",
> + (int)len, name->tip_name, parent_number);
> }
> - return strbuf_detach(&sb, NULL);
> }
>
> static void name_rev(struct commit *start_commit,
> const char *tip_name, timestamp_t taggerdate,
> - int from_tag, int deref)
> + int from_tag, int deref, struct mem_pool *string_pool)
> {
> struct prio_queue queue;
> struct commit *commit;
> @@ -195,9 +191,10 @@ static void name_rev(struct commit *start_commit,
> if (!start_name)
> return;
> if (deref)
> - start_name->tip_name = xstrfmt("%s^0", tip_name);
> + start_name->tip_name = mem_pool_strfmt(string_pool, "%s^0",
> + tip_name);
> else
> - start_name->tip_name = xstrdup(tip_name);
> + start_name->tip_name = mem_pool_strdup(string_pool, tip_name);
>
> memset(&queue, 0, sizeof(queue)); /* Use the prio_queue as LIFO */
> prio_queue_put(&queue, start_commit);
> @@ -235,7 +232,8 @@ static void name_rev(struct commit *start_commit,
> if (parent_number > 1)
> parent_name->tip_name =
> get_parent_name(name,
> - parent_number);
> + parent_number,
> + string_pool);
> else
> parent_name->tip_name = name->tip_name;
> ALLOC_GROW(parents_to_queue,
> @@ -415,7 +413,7 @@ static int name_ref(const char *path, const struct object_id *oid,
> return 0;
> }
>
> -static void name_tips(void)
> +static void name_tips(struct mem_pool *string_pool)
> {
> int i;
>
> @@ -428,7 +426,7 @@ static void name_tips(void)
> struct tip_table_entry *e = &tip_table.table[i];
> if (e->commit) {
> name_rev(e->commit, e->refname, e->taggerdate,
> - e->from_tag, e->deref);
> + e->from_tag, e->deref, string_pool);
> }
> }
> }
> @@ -561,6 +559,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)
>
> int cmd_name_rev(int argc, const char **argv, const char *prefix)
> {
> + struct mem_pool string_pool;
> struct object_array revs = OBJECT_ARRAY_INIT;
> int all = 0, annotate_stdin = 0, transform_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
> struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
> @@ -587,6 +586,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> OPT_END(),
> };
>
> + mem_pool_init(&string_pool, 0);
> init_commit_rev_name(&rev_names);
> git_config(git_default_config, NULL);
> argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
> @@ -648,7 +648,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> adjust_cutoff_timestamp_for_slop();
>
> for_each_ref(name_ref, &data);
> - name_tips();
> + name_tips(&string_pool);
>
> if (annotate_stdin) {
> struct strbuf sb = STRBUF_INIT;
> @@ -676,6 +676,7 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix)
> always, allow_undefined, data.name_only);
> }
>
> + UNLEAK(string_pool);
> UNLEAK(revs);
> return 0;
> }
> --
> 2.44.0
