Re: [PATCH] files-backend: cheapen refname_available check when locking refs

[Jeff King <peff@peff.net>]

On Thu, Aug 17, 2017 at 05:12:50PM +0200, Michael Haggerty wrote:

> I was testing this using the reporter's recipe (but fetching from a
> local clone), and found the following surprising timing numbers:
> 
> b05855b5bc (before the slowdown): 22.7 s
> 524a9fdb51 (immediately after the slowdown): 13 minutes
> 4e81f1ecf1 (after this fix): 14.5 s
> 
> The fact that the fetch is now significantly *faster* than before the
> slowdown seems not to have anything to do with the reference code.

I bisected this (with some hackery, since the commits in the middle all
take 13 minutes to run). The other speedup is indeed unrelated, and is
due to Brandon's aacc5c1a81 (submodule: refactor logic to determine
changed submodules, 2017-05-01).

The commit message doesn't mention performance (it's mostly about code
reduction). I think the speedup comes from using
diff_tree_combined_merge() instead of manually diffing each commit
against its parents. But I didn't do further timings to verify that (I'm
reporting it here mostly as an interesting curiosity for submodule
folks).

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index e9b95592b6..f2a420c611 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -631,11 +631,11 @@ static int lock_raw_ref(struct files_ref_store *refs,
>  
>  		/*
>  		 * If the ref did not exist and we are creating it,
> -		 * make sure there is no existing ref that conflicts
> -		 * with refname:
> +		 * make sure there is no existing packed ref that
> +		 * conflicts with refname:
>  		 */
>  		if (refs_verify_refname_available(
> -				    &refs->base, refname,
> +				    refs->packed_ref_store, refname,
>  				    extras, skip, err))
>  			goto error_return;
>  	}

This seems too easy to be true. :) But I think it matches what we were
doing before 524a9fdb51 (so it's correct), and the performance numbers
don't lie.

-Peff