Re: [PATCH] defer expensive load_ref_decorations until needed

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2017, 05:03 UTC
Message-ID: <xmqqbmjuvrab.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171121234336.10209-1-phil.hord@gmail.com>

Phil Hord <phil.hord@gmail.com> writes:

Show 20 quoted lines

> With many thousands of references, a simple `git rev-parse HEAD` may take
> more than a second to return because it first loads all the refs into
> memory even though it will never use them.
>
> Defer loading any references until we actually need them.
>
> Signed-off-by: Phil Hord <phil.hord@gmail.com>
> ---
>  log-tree.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/log-tree.c b/log-tree.c
> index 3b904f037..c1509f8b9 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -84,8 +84,10 @@ void add_name_decoration(enum decoration_type type, const char *name, struct obj
>  	res->next = add_decoration(&name_decoration, obj, res);
>  }
>  
> +static void maybe_load_ref_decorations();

I'll tweak that "()" and the other one we see below to "(void)" while queuing.

I am not sure if "maybe_" is a good name here, though. If anything, you are making the semantics of "load_ref_decorations()" to "maybe" (but I do not suggest renaming that one).

How about calling it to load_ref_decorations_lazily() or something?

I also wonder if decoration_loaded should now become function-scope static in this new helper, but that can be left outside of the topic.

Other than that, I like what this patch attempts to do. A nicely identified low-hanging fruit ;-).

Thanks.

Show 22 quoted lines

>  const struct name_decoration *get_name_decoration(const struct object *obj)
>  {
> +	maybe_load_ref_decorations();
>  	return lookup_decoration(&name_decoration, obj);
>  }
>  
> @@ -150,10 +152,13 @@ static int add_graft_decoration(const struct commit_graft *graft, void *cb_data)
>  
>  void load_ref_decorations(int flags)
>  {
> -	if (!decoration_loaded) {
> +	decoration_flags = flags;
> +}
>  
> +static void maybe_load_ref_decorations()
> +{
> +	if (!decoration_loaded) {
>  		decoration_loaded = 1;
> -		decoration_flags = flags;
>  		for_each_ref(add_ref_decoration, NULL);
>  		head_ref(add_ref_decoration, NULL);
>  		for_each_commit_graft(add_graft_decoration, NULL);