Re: [PATCH v2] rev-list: exclude promisor objects at walk time

From: Jeff King <peff@peff.net>
Date: Apr 4, 2019, 23:08 UTC
Message-ID: <20190404230759.GA26623@sigill.intra.peff.net>
In-Reply-To: <9f327d6d8dc5e71eb0039aef3ac76ea16c2adab3.1554417917.git.steadmon@google.com>

On Thu, Apr 04, 2019 at 03:53:56PM -0700, Josh Steadmon wrote:

Show 7 quoted lines

> For large repositories, enumerating the list of all promisor objects (in
> order to exclude them from a rev-list walk) can take a significant
> amount of time).
> 
> When --exclude-promisor-objects is passed to rev-list, don't enumerate
> the promisor objects. Instead, filter them (and any children objects)
> during the actual graph walk.

Yeah, this is definitely the approach I was thinking of.

Did you (or anybody else) have any thoughts on the case where a given object is referred to both by a promisor and a non-promisor (and we don't have it)? That's the "shortcut" I think we're taking here: we would no longer realize that it's available via the promisor when we traverse to it from the non-promisor. I'm just not clear on whether that can ever happen.

Show 5 quoted lines

> Helped-By: Jonathan Tan <jonathantanmy@google.com>
> Helped-By: Jeff King <peff@peff.net>
> Helped-By: Jonathan Nieder <jrnieder@gmail.com>
> 
> Signed-off-by: Josh Steadmon <steadmon@google.com>

Minor nit, but these should all be part of the same block.

Show 21 quoted lines

> diff --git a/list-objects.c b/list-objects.c
> index dc77361e11..d1eaa0999e 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -30,6 +30,7 @@ static void process_blob(struct traversal_context *ctx,
>  	struct object *obj = &blob->object;
>  	size_t pathlen;
>  	enum list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_DO_SHOW;
> +	struct object_info oi = OBJECT_INFO_INIT;
>  
>  	if (!ctx->revs->blob_objects)
>  		return;
> @@ -37,6 +38,11 @@ static void process_blob(struct traversal_context *ctx,
>  		die("bad blob object");
>  	if (obj->flags & (UNINTERESTING | SEEN))
>  		return;
> +	if (ctx->revs->exclude_promisor_objects &&
> +	    !oid_object_info_extended(the_repository, &obj->oid, &oi, 0) &&
> +	    oi.whence == OI_PACKED &&
> +	    oi.u.packed.pack->pack_promisor)
> +		return;

This conditional gets repeated a lot in your patch. Perhaps it's worth a helper so we can say:

  if (skip_promisor_object(&ctx->revs, &obj->oid))
	return;

in each place?

One other possible small optimization: we don't look up the object unless the caller asked to exclude promisors, which is good. But we could also keep a single flag for "is there a promisor pack at all?". When there isn't, we know there's no point in looking for the object.

It might not matter much in practice. The main caller here is going to be check_connected(), and it only passes --exclude-promisor-objects if it's in a partial clone.

> [...]

I didn't see any tweaks to the callers, which makes sense; we're already passing --exclude-promisor-objects as necessary. Which means by itself, this patch should be making things faster, right? Do you have timings to show that off?

-Peff