Re: [PATCH v2 7/8] reftable/merged: really reuse buffers to compute record keys

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 28, 2023, 17:03 UTC
Message-ID: <xmqq8r5ei7jv.fsf@gitster.g>
In-Reply-To: <6313f8affdc136b183c1bd411d481efe5c676aee.1703743174.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 6 quoted lines

> In 829231dc20 (reftable/merged: reuse buffer to compute record keys,
> 2023-12-11), we have refactored the merged iterator to reuse a set of
> buffers that it would otherwise have to reallocate on every single
> iteration. Unfortunately, there was a brown-paper-bag-style bug here as
> we continue to release these buffers after the iteration, and thus we
> have essentially gained nothing.

s/-style// perhaps. It took me more than just reading of the above but I needed to see the code before noticed that you are talking about strbuf_release(). Only after that I think I understood what you meant as a bug.

    With the change, instead of using a new "entry_key" strbuf for
    each iteration, the code now passes mi->entry_key to
    reftable_record_key(), which will reuse the existing .buf member
    of the strbuf to avoid reallcation.  But releasing the strbuf in
    each iteration defeats such optimization.

I suspect that a Git developer who will be reading "git log" output in 6 months and finds the above paragraph understands the problem and its fix better if the description hinted strbuf_reset() near where it mentions "release", something like:

    ... to reuse a pair of long-living strbufs by relying on the
    fact that reftable_record_key() tries to reuse its already
    allocated .buf member by calling strbuf_reset(), which should
    give us significantly fewer reallocation compared to the old
    code that used on-stack strbufs that are allocated for each and
    every iteration.  Unfortunately, we called strbuf_release() on
    these long-living strbufs that we meant to reuse, defeating the
    optimization.

or along that line, perhaps?

Other than that, a very reasonable fix.  Thanks for a pleasant read.

Show 34 quoted lines

> Fix this performance issue by not releasing those buffers on iteration
> anymore, where we instead rely on `merged_iter_close()` to release the
> buffers for us.
>
> Using `git show-ref --quiet` in a repository with ~350k refs this leads
> to a significant drop in allocations. Before:
>
>     HEAP SUMMARY:
>         in use at exit: 21,163 bytes in 193 blocks
>       total heap usage: 1,410,148 allocs, 1,409,955 frees, 61,976,068 bytes allocated
>
> After:
>
>     HEAP SUMMARY:
>         in use at exit: 21,163 bytes in 193 blocks
>       total heap usage: 708,058 allocs, 707,865 frees, 36,783,255 bytes allocated
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/merged.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/reftable/merged.c b/reftable/merged.c
> index 556bb5c556..a28bb99aaf 100644
> --- a/reftable/merged.c
> +++ b/reftable/merged.c
> @@ -128,8 +128,6 @@ static int merged_iter_next_entry(struct merged_iter *mi,
>  
>  done:
>  	reftable_record_release(&entry.rec);
> -	strbuf_release(&mi->entry_key);
> -	strbuf_release(&mi->key);
>  	return err;
>  }