Subject: Re: [PATCH v5 05/16] refs/reftable: batch refname availability checks

From: shejialuo <[email-masked]>
Date: Wed, 12 Mar 2025 20:54:18 +0800

On Thu, Mar 06, 2025 at 04:08:36PM +0100, Patrick Steinhardt wrote:
> Refactor the "reftable" backend to batch the availability check for
> refnames. This does not yet have an effect on performance as
> `refs_verify_refnames_available()` effectively still performs the
> availability check for each refname individually. But this will be
> optimized in subsequent commits, where we learn to optimize some parts
> of the logic when checking multiple refnames for availability.
> 
> Signed-off-by: Patrick Steinhardt <[email-masked]>
> ---
>  refs/reftable-backend.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index d39a14c5a46..2a90e7cb391 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1069,6 +1069,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>  		reftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, "ref_transaction_prepare");
>  	struct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT;
>  	struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
> +	struct string_list refnames_to_check = STRING_LIST_INIT_NODUP;
>  	struct reftable_transaction_data *tx_data = NULL;
>  	struct reftable_backend *be;
>  	struct object_id head_oid;
> @@ -1224,12 +1225,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>  			 * can output a proper error message instead of failing
>  			 * at a later point.
>  			 */
> -			ret = refs_verify_refname_available(ref_store, u->refname,
> -							    &affected_refnames, NULL,
> -							    transaction->flags & REF_TRANSACTION_FLAG_INITIAL,
> -							    err);
> -			if (ret < 0)
> -				goto done;
> +			string_list_append(&refnames_to_check, u->refname);
>  

Instead of checking in `prepare`, we will just append the refname to the
list.

>  			/*
>  			 * There is no need to write the reference deletion
> @@ -1379,6 +1375,13 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>  		}
>  	}
>  
> +	string_list_sort(&refnames_to_check);

I am curious why we need to sort the refnames here. I think at current,
we don't optimize the `refs_verify_refnames_available` function. No
matter whether the `refnames_to_check` is sorted, it should not
change the result of `refs_verify_refnames_available` function, right? I
guess this statement may be related to optimization part. If so, I think
we should delete this line and add in the later commit.

However, I am not sure.

> +	ret = refs_verify_refnames_available(ref_store, &refnames_to_check, &affected_refnames, NULL,
> +					     transaction->flags & REF_TRANSACTION_FLAG_INITIAL,
> +					     err);
> +	if (ret < 0)
> +		goto done;
> +
>  	transaction->backend_data = tx_data;
>  	transaction->state = REF_TRANSACTION_PREPARED;
>  
> @@ -1394,6 +1397,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>  	string_list_clear(&affected_refnames, 0);
>  	strbuf_release(&referent);
>  	strbuf_release(&head_referent);
> +	string_list_clear(&refnames_to_check, 0);
>  
>  	return ret;
>  }