| Message ID | 20250228-pks-update-ref-optimization-v4-5-6425c04268b5@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | refs: batch refname availability checks | expand |
Patrick Steinhardt <ps@pks.im> writes: > Refactor the "reftable" backend to batch the availability check for > refnames. This does not yet have an effect on performance as we > essentially still call `refs_verify_refname_available()` in a loop, but > this will change in subsequent commits. > I thought this patch removes it from the loop. Which loop are you talking about? > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > 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); > > /* > * 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); > + 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; > } > > -- > 2.49.0.rc0.375.gae4b89d849.dirty
Karthik Nayak <karthik.188@gmail.com> writes: > Patrick Steinhardt <ps@pks.im> writes: > >> Refactor the "reftable" backend to batch the availability check for >> refnames. This does not yet have an effect on performance as we >> essentially still call `refs_verify_refname_available()` in a loop, but >> this will change in subsequent commits. >> > > I thought this patch removes it from the loop. Which loop are you > talking about? > Looking at future patches, maybe this 'loop' is a reference to how 'refs_verify_refnames_available()' still loops over all references, which we start optimizing in patch 08 and onward? [snip]
On Thu, Mar 06, 2025 at 09:12:41AM -0500, Karthik Nayak wrote: > Karthik Nayak <karthik.188@gmail.com> writes: > > > Patrick Steinhardt <ps@pks.im> writes: > > > >> Refactor the "reftable" backend to batch the availability check for > >> refnames. This does not yet have an effect on performance as we > >> essentially still call `refs_verify_refname_available()` in a loop, but > >> this will change in subsequent commits. > >> > > > > I thought this patch removes it from the loop. Which loop are you > > talking about? > > > > Looking at future patches, maybe this 'loop' is a reference to how > 'refs_verify_refnames_available()' still loops over all references, > which we start optimizing in patch 08 and onward? Yes, exactly. I'll clarify. Patrick
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); /* * 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); + 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; }
Refactor the "reftable" backend to batch the availability check for refnames. This does not yet have an effect on performance as we essentially still call `refs_verify_refname_available()` in a loop, but this will change in subsequent commits. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- refs/reftable-backend.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)