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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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).

> 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?

>
>  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);