Re: [PATCH 3/4] peel_ref: check object type before loading

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2012, 19:06 UTC
Message-ID: <7vbogiys47.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20121004080253.GC31325@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

> The point of peel_ref is to dereference tags; if the base
> object is not a tag, then we can return early without even
> loading the object into memory.
>
> This patch accomplishes that by checking sha1_object_info
> for the type. For a packed object, we can get away with just
> looking in the pack index. For a loose object, we only need
> to inflate the first couple of header bytes.

We look at the pack index and have to follow its delta chain down to the base to find its type; if the object is deeply deltified, this certainly is an overall loss.

The only case sha1_object_info() could work well for an object near the tip of a deep delta chain is to find its size, as the diff-delta encodes the size of the base and the size of the result of applying the delta to the base, so you do not have to follow the chain when you are only interested in the final size.

But alas nobody calls sha1_object_info() for only size but not type (there are some callers that are interested in only type but not size).

Show 20 quoted lines

> This is a bit of a gamble; if we do find a tag object, then
> we will end up loading the content anyway, and the extra
> lookup will have been wasteful. However, if it is not a tag
> object, then we save loading the object entirely. Depending
> on the ratio of non-tags to tags in the input, this can be a
> minor win or minor loss.
>
> However, it does give us one potential major win: if a ref
> points to a large blob (e.g., via an unannotated tag), then
> we can avoid looking at it entirely.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> This optimization is the one that gave me the most pause. While
> upload-pack does call peel_ref on everything, the other callers all
> constrain themselves to refs/tags/. So for many projects, we will be
> calling it mostly on annotated tags, and it may be a very small net
> loss. But in practice, it will not matter for most projects with a sane
> number of normal tags, and saving even one accidental giant blob load
> can have a huge impact.

I may be missing something, but the above description is not convincing to me. When was the last time you pointed a blob directly with a ref, whether large or small, and whether within refs/tags or outside?

Show 26 quoted lines

>
>  refs.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index f672ad9..02e47b1 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1225,8 +1225,15 @@ fallback:
>  	}
>  
>  fallback:
> -	o = parse_object(base);
> -	if (o && o->type == OBJ_TAG) {
> +	o = lookup_unknown_object(base);
> +	if (o->type == OBJ_NONE) {
> +		int type = sha1_object_info(base, NULL);
> +		if (type < 0)
> +			return -1;
> +		o->type = type;
> +	}
> +
> +	if (o->type == OBJ_TAG) {
>  		o = deref_tag_noverify(o);
>  		if (o) {
>  			hashcpy(sha1, o->sha1);