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

From: James Liu <james@jamesliu.io>
Date: Feb 28, 2024, 00:06 UTC
Message-ID: <CZG9VJKP1EA8.432Y9U39737O@jamesliu.io>
In-Reply-To: <3ba697036c1db3837f46775823a7bd55602b4bac.1707895758.git.ps@pks.im>

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!

Show 5 quoted lines

> +	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.

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

Show 6 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;