Re: [PATCH 08/15] name-rev: pull out deref handling from the recursion

["SZEDER Gábor" <szeder.dev@gmail.com>]

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.


> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  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