Patrick Steinhardt <ps@xxxxxx> writes:
> 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.
> 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@xxxxxx>
> ---
> 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;
> }
