Re: [PATCH on sb/more-repo-in-api] revision: use commit graph in get_reference()

From: Jeff King <peff@peff.net>
Date: Dec 5, 2018, 04:54 UTC
Message-ID: <20181205045416.GB12284@sigill.intra.peff.net>
In-Reply-To: <20181204224238.50966-1-jonathantanmy@google.com>

On Tue, Dec 04, 2018 at 02:42:38PM -0800, Jonathan Tan wrote:

Show 22 quoted lines

> diff --git a/revision.c b/revision.c
> index b5108b75ab..e7da2c57ab 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -212,7 +212,20 @@ static struct object *get_reference(struct rev_info *revs, const char *name,
>  {
>  	struct object *object;
>  
> -	object = parse_object(revs->repo, oid);
> +	/*
> +	 * If the repository has commit graphs, repo_parse_commit() avoids
> +	 * reading the object buffer, so use it whenever possible.
> +	 */
> +	if (oid_object_info(revs->repo, oid, NULL) == OBJ_COMMIT) {
> +		struct commit *c = lookup_commit(revs->repo, oid);
> +		if (!repo_parse_commit(revs->repo, c))
> +			object = (struct object *) c;
> +		else
> +			object = NULL;
> +	} else {
> +		object = parse_object(revs->repo, oid);
> +	}

This seems like a reasonable thing to do, but I have sort of a meta-comment. In several places we've started doing this kind of "if it's this type of object, do X, otherwise do Y" optimization (e.g., handling large blobs for streaming).

And in the many cases we end up doubling the effort to do object lookups: here we do one lookup to get the type, and then if it's not a commit (or if we don't have a commit graph) we end up parsing it anyway.

I wonder if we could do better. In this instance, it might make sense to first see if we actually have a commit graph available (it might not have this object, of course, but at least we'd expect it to have most commits). In general, it would be nice if we had a more incremental API for accessing objects: open, get metadata, then read the data. That would make these kinds of optimizations "free".

I don't have numbers for how much the extra lookups cost. The lookups are probably dwarfed by parse_object() in general, so even if we save only a few full object loads, it may be a win. It just seems a shame that we may be making the "slow" paths (when our type-specific check doesn't match) even slower.

-Peff