reftable: fixes and optimizations (pt.2)

Hi,

this is the third version of my patch series that contains additional fixes and optimizations for the reftable library.

The only changes in this iteration are improvements to the commit messages to hopefully make them easier to understand. Thanks Junio for your suggestions!

Patrick

Patrick Steinhardt (8):
reftable/stack: do not overwrite errors when compacting reftable/stack: do not auto-compact twice in reftable_stack_add() reftable/writer: fix index corruption when writing multiple indices reftable/record: constify some parts of the interface reftable/record: store "val1" hashes as static arrays reftable/record: store "val2" hashes as static arrays reftable/merged: really reuse buffers to compute record keys reftable/merged: transfer ownership of records when iterating

 reftable/block_test.c      |   4 +-
 reftable/merged.c          |   8 +--
 reftable/merged_test.c     |  16 +++---
 reftable/readwrite_test.c  | 102 +++++++++++++++++++++++++++++++------
 reftable/record.c          |  17 ++-----
 reftable/record_test.c     |   5 --
 reftable/reftable-record.h |  11 ++--
 reftable/stack.c           |  23 +++------
 reftable/stack_test.c      |   2 -
 reftable/writer.c          |   4 +-
 10 files changed, 117 insertions(+), 75 deletions(-)

Range-diff against v2:

1:  22a57020c6 = 1:  1dc8ddf04a reftable/stack: do not overwrite errors when compacting
2:  a08f7efc99 = 2:  eccc34a4b6 reftable/stack: do not auto-compact twice in `reftable_stack_add()`
3:  c00e08d97f = 3:  15e12b8f29 reftable/writer: fix index corruption when writing multiple indices
4:  3416268087 = 4:  017e8943c7 reftable/record: constify some parts of the interface
5:  46ca3a37f8 ! 5:  f1d2190489 reftable/record: store "val1" hashes as static arrays

@@ Metadata
## Commit message ##
reftable/record: store "val1" hashes as static arrays

- When reading ref records of type "val1" we store its object ID in an
+ When reading ref records of type "val1", we store its object ID in an
allocated array. This results in an additional allocation for every single ref record we read, which is rather inefficient especially when iterating over refs.

- Refactor the code to instead use a static array of GIT_MAX_RAWSZ
+ Refactor the code to instead use an embedded array of GIT_MAX_RAWSZ
bytes. While this means that struct ref_record is bigger now, we typically do not store all refs in an array anyway and instead only handle a limited number of records at the same point in time.

6:  c8a36917b1 = 6:  6ec02d6775 reftable/record: store "val2" hashes as static arrays
7:  6313f8affd ! 7:  845dec2390 reftable/merged: really reuse buffers to compute record keys

@@ Commit message
reftable/merged: really reuse buffers to compute record keys

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.
+ 2023-12-11), we have refactored the merged iterator to reuse a pair of
+ long-living strbufs by relying on the fact that reftable_record_key()
+ tries to reuse already allocated strbufs by calling strbuf_reset(),
+ which should give us significantly fewer reallocations 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 on each iteration, defeating
+ the optimization.

Fix this performance issue by not releasing those buffers on iteration anymore, where we instead rely on merged_iter_close() to release the
8: 25a3919e58 ! 8: eea0e161ad reftable/merged: transfer ownership of records when iterating
@@ Metadata
## Commit message ##
reftable/merged: transfer ownership of records when iterating

- When iterating over recods with the merged iterator we put the records
+ When iterating over records with the merged iterator we put the records
into a priority queue before yielding them to the caller. This means that we need to allocate the contents of these records before we can pass them over to the caller.

base-commit: a26002b62827b89a19b1084bd75d9371d565d03c

2.43.GIT