[00/10] reftable: drop generic `reftable_table` interface

Message

Patrick Steinhardt Aug. 13, 2024, 6:24 a.m. UTC

Hi,

this patch series performs some simplification of the reftable library
by dropping the generic `reftable_table` interface. The intent of this
interface is to abstract away whether the underlying table is either a
single reftable, or a merged reftable. Arguably though, it is not needed
anymore starting with my recent refactorings [1] that made the reftable
iterator itself seekable, as that now provides such a generic interface
already.

So this patch series rips out the `reftable_table` interface, both to
remove unused clutter, but more importantly to make the reftable library
easier to understand overall. There is also a tiny performance gain of
1-3% by requiring one less vtable function call. But while that speedup
is consistent, I didn't include any benchmarks as it is rather close to
noise and not the primary motivation of this patch series.

Patrick

[1]: <[e-mail]>

Patrick Steinhardt (10):
  reftable/merged: expose functions to initialize iterators
  reftable/merged: rename `reftable_new_merged_table()`
  reftable/merged: stop using generic tables in the merged table
  reftable/stack: open-code reading refs
  reftable/iter: drop double-checking logic
  reftable/generic: move generic iterator code into iterator interface
  reftable/dump: drop unused `compact_stack()`
  reftable/dump: drop unused printing functionality
  reftable/dump: move code into "t/helper/test-reftable.c"
  reftable/generic: drop interface

 Makefile                         |   2 -
 reftable/dump.c                  | 111 ---------------
 reftable/generic.c               | 229 -------------------------------
 reftable/generic.h               |  37 -----
 reftable/iter.c                  | 127 ++++++++++++++---
 reftable/iter.h                  |  30 +++-
 reftable/merged.c                |  72 ++++------
 reftable/merged.h                |   4 +-
 reftable/reader.c                |  70 +---------
 reftable/reader.h                |   4 +
 reftable/record.c                | 127 -----------------
 reftable/record.h                |   4 -
 reftable/reftable-generic.h      |  47 -------
 reftable/reftable-merged.h       |  26 ++--
 reftable/reftable-reader.h       |   9 --
 reftable/reftable-record.h       |   8 --
 reftable/reftable-stack.h        |   3 -
 reftable/stack.c                 |  94 ++++++-------
 reftable/stack_test.c            |  29 ++--
 t/helper/test-reftable.c         |  47 ++++++-
 t/unit-tests/t-reftable-merged.c |  17 +--
 21 files changed, 281 insertions(+), 816 deletions(-)
 delete mode 100644 reftable/dump.c
 delete mode 100644 reftable/generic.c
 delete mode 100644 reftable/generic.h
 delete mode 100644 reftable/reftable-generic.h

Comments

Karthik Nayak Aug. 13, 2024, 10:17 a.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

> Hi,
>
> this patch series performs some simplification of the reftable library
> by dropping the generic `reftable_table` interface. The intent of this
> interface is to abstract away whether the underlying table is either a
> single reftable, or a merged reftable. Arguably though, it is not needed
> anymore starting with my recent refactorings [1] that made the reftable
> iterator itself seekable, as that now provides such a generic interface
> already.
>
> So this patch series rips out the `reftable_table` interface, both to
> remove unused clutter, but more importantly to make the reftable library
> easier to understand overall. There is also a tiny performance gain of
> 1-3% by requiring one less vtable function call. But while that speedup
> is consistent, I didn't include any benchmarks as it is rather close to
> noise and not the primary motivation of this patch series.
>
> Patrick
>

This is a very welcome change, the indirection was a cause for confusion
before and this clears it up.

My only concern is the loss of dumping reftables for debug purposes in
patch 8, I suggest we keep them until we have alternatives. Apart from
that everything looks great.

Thanks!