Re: [PATCH 08/15] name-rev: pull out deref handling from the recursion
From: SZEDER Gábor <hidden>
Date: 2019-09-23 20:47:21
On Mon, Sep 23, 2019 at 09:55:11PM +0200, René Scharfe wrote:
-- >8 -- Subject: [PATCH] name-rev: use FLEX_ARRAY for tip_name in struct rev_name Give each rev_name its very own tip_name string. This simplifies memory ownership, as callers of name_rev() only have to make sure the tip_name parameter exists for the duration of the call and don't have to preserve it for the whole run of the program. It also saves four or eight bytes per object because this change removes the pointer indirection. Memory usage is still higher for linear histories that previously shared the same tip_name value between multiple name_rev instances.
Besides looking at memory usage, have you run any performance benchmarks? Here it seems to make 'git name-rev --all >out' slower by 17% in the git repo and by 19.5% in the linux repo.
quoted hunk ↗ jump to hunk
Signed-off-by: René Scharfe <redacted> --- builtin/name-rev.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)diff --git a/builtin/name-rev.c b/builtin/name-rev.c index c785fe16ba..4162fb29ee 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c@@ -12,11 +12,11 @@ #define CUTOFF_DATE_SLOP 86400 /* one day */ typedef struct rev_name { - const char *tip_name; timestamp_t taggerdate; int generation; int distance; int from_tag; + char tip_name[FLEX_ARRAY]; } rev_name; define_commit_slab(commit_rev_name, struct rev_name *);@@ -97,17 +97,14 @@ static void name_rev(struct commit *commit, die("generation: %d, but deref?", generation); } - if (name == NULL) { - name = xmalloc(sizeof(rev_name)); - set_commit_rev_name(commit, name); - goto copy_data; - } else if (is_better_name(name, taggerdate, distance, from_tag)) { -copy_data: - name->tip_name = tip_name; + if (!name || is_better_name(name, taggerdate, distance, from_tag)) { + free(name); + FLEX_ALLOC_STR(name, tip_name, tip_name); name->taggerdate = taggerdate; name->generation = generation; name->distance = distance; name->from_tag = from_tag; + set_commit_rev_name(commit, name); } else { free(to_free); return;@@ -131,12 +128,14 @@ static void name_rev(struct commit *commit, name_rev(parents->item, new_name, taggerdate, 0, distance + MERGE_TRAVERSAL_WEIGHT, from_tag, 0); + free(new_name); } else { name_rev(parents->item, tip_name, taggerdate, generation + 1, distance + 1, from_tag, 0); } } + free(to_free); } static int subpath_matches(const char *path, const char *filter)@@ -270,8 +269,7 @@ static int name_ref(const char *path, const struct object_id *oid, int flags, vo if (taggerdate == TIME_MAX) taggerdate = ((struct commit *)o)->date; path = name_ref_abbrev(path, can_abbreviate_output); - name_rev(commit, xstrdup(path), taggerdate, 0, 0, - from_tag, deref); + name_rev(commit, path, taggerdate, 0, 0, from_tag, deref); } return 0; } --2.23.0