Re: [PATCH 09/12] reftable/record: reuse refname when decoding

From: Patrick Steinhardt <ps@pks.im>
Date: Mar 4, 2024, 10:39 UTC
Message-ID: <ZeWk34iSgTQllpFc@tanuki>
In-Reply-To: <CZG9VJKP1EA8.432Y9U39737O@jamesliu.io>

On Wed, Feb 28, 2024 at 11:06:52AM +1100, James Liu wrote:

Show 23 quoted lines

> On Wed Feb 14, 2024 at 6:46 PM AEDT, Patrick Steinhardt wrote:
> > This refactoring is safe to do because all functions that assigning to
> > the refname will first call `release_reftable_record()`, which will zero
> > out the complete record after releasing memory.
> 
> s/release_reftable_record/reftable_ref_record_release
> 
> > Furthermore, with this change we now perform a mostly constant number of
> > allocations when iterating.
> 
> That's awesome!
> 
> > +	SWAP(refname, r->refname);
> > +	SWAP(refname_cap, r->refname_cap);
> >  	reftable_ref_record_release(r);
> > +	SWAP(refname, r->refname);
> > +	SWAP(refname_cap, r->refname_cap);
> 
> What do you think about reversing the order of the `SWAP` arguments in
> the last two invocations? If my understanding is correct that we're
> preserving the `refname` and `refname_cap` fields so we can set them back
> into a freshly initialised `r`, reversing the args might make that intent
> a bit clearer.

Yeah, fair enough.

> Also, since we're unconditionally `memcpy`ing the key into `r->refname`
> below, can we avoid the `SWAP(refname, r->refname)` call altogether?

No, otherwise `reftable_ref_record_release()` would have already released the underlying pointer of `r->refname` and the call to `REFTABLE_ALLOC_GROW()` would always have to reallocate it.

Patrick

Show 7 quoted lines

> > -	assert(hash_size > 0);
> > -
> > -	r->refname = reftable_malloc(key.len + 1);
> > +	REFTABLE_ALLOC_GROW(r->refname, key.len + 1, r->refname_cap);
> >  	memcpy(r->refname, key.buf, key.len);
> >  	r->refname[key.len] = 0;
>