Re: [PATCH] describe: Don’t look up commits with --exact-match

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 7, 2010, 18:39 UTC
Message-ID: <7vfwu9qvew.fsf@alter.siamese.dyndns.org>
In-Reply-To: <alpine.DEB.2.02.1012061159500.23348@dr-wily.mit.edu>

Anders Kaseorg <andersk@ksplice.com> writes:

Show 25 quoted lines

> From 2ad1e58b8f6e9c117c77748b6e8b85227d9d5412 Mon Sep 17 00:00:00 2001
> From: Anders Kaseorg <andersk@ksplice.com>
> Subject: [PATCH 1/2] describe: Use for_each_rawref
>
> Don’t waste time checking for dangling refs; they wouldn’t affect the
> output of ‘git describe’ anyway.  Although this doesn’t gain much
> performance by itself, it does in conjunction with the next commit.
>
> Signed-off-by: Anders Kaseorg <andersk@ksplice.com>
> ---
>  builtin/describe.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 43caff2..700f740 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -418,7 +418,7 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  		return cmd_name_rev(i + argc, args, prefix);
>  	}
>  
> -	for_each_ref(get_name, NULL);
> +	for_each_rawref(get_name, NULL);
>  	if (!found_names && !always)
>  		die("No names found, cannot describe anything.");

It is Ok during a review cycle that is not meant as the final submission, but please follow Documentation/SubmittingPatches for the final round (i.e. not multiple-changes in a single mail), as mentioned by Jonathan.

Show 8 quoted lines

> From ce8a2ab9cf80247c2834d21f36f63cedb794e62f Mon Sep 17 00:00:00 2001
> From: Anders Kaseorg <andersk@ksplice.com>
> Subject: describe: Don’t look up commits with --exact-match
>
> This makes ‘git describe --exact-match HEAD’ about 15 times faster on
> a cold cache (2.3s instead of 35s) in a linux-2.6 repository with many
> packed tags.  That’s a huge win for the interactivity of the __git_ps1
> shell prompt helper.

Good description of the motivation.  Care to describe how it is achieved?

I may be misreading your intention, but my understanding of the thinking behind what the new code does is...

 - When the object name given from the command line happens to match one
   of the tags exactly (either a lightweight tag in which case we can just
   compare the object names to detect the match, or an annotated tag in
   which case we only need to parse the tag but not the underlying commit
   to do the comparison), we do not have to parse any commit object.

 - Other cases we would need to parse commit objects and traverse the
   history.

 - Hence, the code can be optimized for the case to describe an exact
   match by trying exact object name match first and then loading commit
   objects only when no exact matches found.

 - When we are only looking for exact matches (which is _not_ a rare
   corner case), we won't ever fall back to the "parse commit objects and
   traverse the history" codepath.  So it is important to lazily parse
   commit objects in order to optimize for this special case.

I however think this may hurt when more than one objects are asked to be described and there is no exact match. The fallback codepath that lazily runs lookup_commit_reference_gently() and uses replace_name() to compute the best name needs to run only once, but this part of the code seems to run it for the elements on the names list once per describe() invocation that needs to fall back to this codepath. Would it remove this regression if we make it run only once?

I don't think this affects correctness, though.

Show 13 quoted lines

> @@ -259,6 +255,12 @@ static void describe(const char *arg, int last_one)
>  	if (debug)
>  		fprintf(stderr, "searching to describe %s\n", arg);
>  
> +	for (e = names; e; e = e->next) {
> +		struct commit *c = lookup_commit_reference_gently(e->peeled, 1);
> +		if (c && replace_name(c->util, e->prio, e->sha1, &e->tag))
> +			c->util = e;
> +	}
> +
>  	list = NULL;
>  	cmit->object.flags = SEEN;
>  	commit_list_insert(cmit, &list);

As you can see from its "if (!e->tag)" codepath, replace_name() is not designed to be called excessively (e.g. we will end up running the codepath for an annotated tag that we should know we won't find a tag).

Also, "struct tag *tag" does not need to exist in add_to_known_names() anymore; a NULL is assigned to it and its only use is to get assigned to e->tag.