Re: [PATCH v5 07/16] refs/files: batch refname availability checks for initial transactions
- From
shejialuo <shejialuo@gmail.com>
- Date
- Mar 12, 2025, 13:06 UTC
- Message-ID
- <Z9GGwNpfDly4_NIL@ArchLinux>
- In-Reply-To
- <20250306-pks-update-ref-optimization-v5-7-dcb2ee037e97@pks.im>
On Thu, Mar 06, 2025 at 04:08:38PM +0100, Patrick Steinhardt wrote:
Show 17 quoted lines
> The "files" backend explicitly carves out special logic for its initial > transaction so that it can avoid writing out every single reference as > a loose reference. While the assumption is that there shouldn't be any > preexisting references, we still have to verify that none of the newly > written references will conflict with any other new reference in the > same transaction. > > Refactor the initial transaction to use batched refname availability > checks. This does not yet have an effect on performance as we still call > `refs_verify_refname_available()` in a loop. But this will change in > subsequent commits and then impact performance when cloning a repository > with many references or when migrating references to the "files" format. > > This will improve performance when cloning a repository with many > references or when migrating references from any format to the "files" > format once the availability checks have learned to optimize checks for > many references in a subsequent commit.
I guess you forgot to delete some sentences for the commit message. This paragraph is a little redundant. In the second paragraph, I think we have already talked about this.
Show 42 quoted lines
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> refs/files-backend.c | 23 ++++++++++++++++-------
> 1 file changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6ce79cf0791..11a620ea11a 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3056,6 +3056,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
> size_t i;
> int ret = 0;
> struct string_list affected_refnames = STRING_LIST_INIT_NODUP;
> + struct string_list refnames_to_check = STRING_LIST_INIT_NODUP;
> struct ref_transaction *packed_transaction = NULL;
> struct ref_transaction *loose_transaction = NULL;
>
> @@ -3105,11 +3106,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
> !is_null_oid(&update->old_oid))
> BUG("initial ref transaction with old_sha1 set");
>
> - if (refs_verify_refname_available(&refs->base, update->refname,
> - &affected_refnames, NULL, 1, err)) {
> - ret = TRANSACTION_NAME_CONFLICT;
> - goto cleanup;
> - }
> + string_list_append(&refnames_to_check, update->refname);
>
> /*
> * packed-refs don't support symbolic refs, root refs and reflogs,
> @@ -3145,8 +3142,19 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
> }
> }
>
> - if (packed_refs_lock(refs->packed_ref_store, 0, err) ||
> - ref_transaction_commit(packed_transaction, err)) {
> + if (packed_refs_lock(refs->packed_ref_store, 0, err)) {
> + ret = TRANSACTION_GENERIC_ERROR;
> + goto cleanup;
> + }
> +Still the same question: why we don't sort the `refnames_to_check` here.
Show 18 quoted lines
> + if (refs_verify_refnames_available(&refs->base, &refnames_to_check,
> + &affected_refnames, NULL, 1, err)) {
> + packed_refs_unlock(refs->packed_ref_store);
> + ret = TRANSACTION_NAME_CONFLICT;
> + goto cleanup;
> + }
> +
> + if (ref_transaction_commit(packed_transaction, err)) {
> ret = TRANSACTION_GENERIC_ERROR;
> goto cleanup;
> }
> @@ -3167,6 +3175,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,
> ref_transaction_free(packed_transaction);
> transaction->state = REF_TRANSACTION_CLOSED;
> string_list_clear(&affected_refnames, 0);
> + string_list_clear(&refnames_to_check, 0);
> return ret;
> }