Re: [PATCH 6/7] reftable/record: use scratch buffer when decoding records
- From
Josh Steadmon <steadmon@google.com>
- Date
- Mar 11, 2024, 19:31 UTC
- Message-ID
- <Ze9cLWCZoBqcfr2B@google.com>
- In-Reply-To
- <e0a9057593f761917a3e6fb9a77045400e2b9d1c.1709640322.git.ps@pks.im>
On 2024.03.05 13:11, Patrick Steinhardt wrote:
Show 31 quoted lines
> When decoding log records we need a temporary buffer to decode the > reflog entry's name, mail address and message. As this buffer is local > to the function we thus have to reallocate it for every single log > record which we're about to decode, which is inefficient. > > Refactor the code such that callers need to pass in a scratch buffer, > which allows us to reuse it for multiple decodes. This reduces the > number of allocations when iterating through reflogs. Before: > > HEAP SUMMARY: > in use at exit: 13,473 bytes in 122 blocks > total heap usage: 2,068,487 allocs, 2,068,365 frees, 305,122,946 bytes allocated > > After: > > HEAP SUMMARY: > in use at exit: 13,473 bytes in 122 blocks > total heap usage: 1,068,485 allocs, 1,068,363 frees, 281,122,886 bytes allocated > > Note that this commit also drop some redundant calls to `strbuf_reset()` > right before calling `decode_string()`. The latter already knows to > reset the buffer, so there is no need for these. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > reftable/block.c | 4 ++- > reftable/block.h | 2 ++ > reftable/record.c | 52 ++++++++++++++++++-------------------- > reftable/record.h | 5 ++-- > reftable/record_test.c | 57 ++++++++++++++++++++++++++---------------- > 5 files changed, 68 insertions(+), 52 deletions(-)
[snip]
Show 27 quoted lines
> diff --git a/reftable/record.c b/reftable/record.c
> index 7c86877586..060244337f 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> @@ -374,7 +374,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,
>
> static int reftable_ref_record_decode(void *rec, struct strbuf key,
> uint8_t val_type, struct string_view in,
> - int hash_size)
> + int hash_size, struct strbuf *scratch)
> {
> struct reftable_ref_record *r = rec;
> struct string_view start = in;
> @@ -425,13 +425,12 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,
> break;
>
> case REFTABLE_REF_SYMREF: {
> - struct strbuf dest = STRBUF_INIT;
> - int n = decode_string(&dest, in);
> + int n = decode_string(scratch, in);
> if (n < 0) {
> return -1;
> }
> string_view_consume(&in, n);
> - r->value.symref = dest.buf;
> + r->value.symref = strbuf_detach(scratch, NULL);
> } break;I had to dig into this to convince myself that we aren't leaking memory here, but IIUC this gets cleaned up eventually by reftable_ref_record_release(), right?