Re: [PATCH v4 7/8] refs: support rejection in batch updates during F/D checks

[Patrick Steinhardt <ps@pks.im>]

On Mon, Mar 24, 2025 at 05:48:32PM +0000, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Thu, Mar 20, 2025 at 12:44:02PM +0100, Karthik Nayak wrote:
> >> diff --git a/refs/files-backend.c b/refs/files-backend.c
> >> index be758ffff5..1d50d4013c 100644
> >> --- a/refs/files-backend.c
> >> +++ b/refs/files-backend.c
> >> @@ -864,7 +868,9 @@ static enum ref_transaction_error lock_raw_ref(struct files_ref_store *refs,
> >>  		 * make sure there is no existing packed ref that conflicts
> >>  		 * with refname. This check is deferred so that we can batch it.
> >>  		 */
> >> -		string_list_append(refnames_to_check, refname);
> >> +		item = string_list_append(refnames_to_check, refname);
> >> +		item->util = xmalloc(sizeof(update_idx));
> >> +		memcpy(item->util, &update_idx, sizeof(update_idx));
> >>  	}
> >>
> >>  	ret = 0;
> >
> > Hm, so we have to allocate the `util` field now to store the update
> > index, which is a bit unfortunate because all of this is part of the hot
> > loop. We cannot store a direct pointer though because the array of
> > updates may be reallocated, which would invalidate any pointers pointing
> > into the array.
> >
> 
> Yes, your inference is on point.
> 
> > I was wondering whether we could abuse an `uintptr_t` and use it to
> > store the update index as a pointer. It does feel somewhat dirty though.
> >
> 
> We can do something like this, it would be _clever_ but does feel very
> hacky.
> 
> I did so some benchmarking here
> 
> Benchmark 1: update-ref: create many refs (refformat = files, refcount
> = 100000, revision = master)
>   Time (mean ± σ):      7.396 s ±  0.175 s    [User: 0.962 s, System: 6.312 s]
>   Range (min … max):    7.145 s …  7.688 s    10 runs
> 
> Benchmark 2: update-ref: create many refs (refformat = files, refcount
> = 100000, revision = b4/245-partially-atomic-ref-updates)
>   Time (mean ± σ):      7.514 s ±  0.144 s    [User: 0.919 s, System: 6.438 s]
>   Range (min … max):    7.297 s …  7.750 s    10 runs
> 
> Summary
>   update-ref: create many refs (refformat = files, refcount = 100000,
> revision = master) ran
>     1.02 ± 0.03 times faster than update-ref: create many refs
> (refformat = files, refcount = 100000, revision =
> b4/245-partially-atomic-ref-updates)
> 
> Overall the perf degradation is very minimal. I also looked at the
> flamegraph to see if there is something. But most of the time seems to
> be spent in IO (reading refs and creating locks).
> 
> So I think we should be ok here, wdyt?

Okay. I'm not a huge fan of using the `->util` pointer like this, but I
don't have a better idea either, and the performance regression seems
acceptable. So let's roll with it until somebody has a better idea.

Thanks for doing the benchmark!

Patrick