[PATCH v2 0/4] reftable/stack: avoid reloading the stack when locked

[Karthik Nayak <karthik.188@gmail.com>]

This patch series is based on the report by Jeff [1], where he noticed
that when creating a lot of refs within a single reference transaction,
the majority of the time was spent on fstat().

The issue stems from the fact that within the reftable library we do not
track Git reference transactions, as such any calls within the library
would potentially reload the stack to ensure that there are no
concurrent updates made to the stack. While this makes sense outside of
a reference transaction, within one, the stack is locked, so reloading
the stack is a no-op. The only time we want to reload the stack is
immediately after locking the list file, which is to catch any
concurrent updates made to the stack.

The first patch in this small series, cleans up the flow of reloading
the stack by providing a flag explicitly. The patch argues that since
all flows reload the stack, the flag can be safely removed. This
simplifies the flow of when to reload the stack.

The next two commits move the lock variable to the reftable_stack
structure and then use this information to decide if reloading of the
stack is necessary.

During benchmarking, I first tried to benchmark adding new references
against HEAD. This kicks in the DWIM ref resolution, and we iterate over
siz difference candidate ref names before settling on a match. Each such
lookup reloads the stack. This happens before the reference transaction
is created. I quickly realized that this would dominate the benchmarks,
so the benchmarks in the third patch are against a static commit OID.

Benchmarking with a fixed, non-symbolic target OID in the 'refs/tags/'
namespace (since it triggers a stack reload when checking if reflog
exists for the given tag name), shows a consistent 15-20% improvement
with these patches:

   refcount   master     patch     speedup
   --------   -------    -------   -------
   2,000       18.5 ms    16.6 ms   1.11x
   20,000     120.7 ms   102.8 ms   1.17x
   50,000     296.5 ms   247.1 ms   1.20x

The syscall counts show that now the calls to `newfstatat()` stay
constant at around 55 calls regardless of the number of refs to be
created. Before this would grow linearly with the number of refs.

[1]: https://lore.kernel.org/git/20260629203527.GA1895313@coredump.intra.peff.net/

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v2:
- Fix typos in commit messages and add additional information.
- To prevent multiple `reftable_addition`'s from releasing each others
  list lock, add a bit field to track ownership. Add related unit test.
- Add correct benchmark results in the last commit, my earlier results
  were based on using 'refs/heads' which doesn't check for reflogs when
  using the default config. Use 'refs/tags' as suggested by Peff. This
  shows a good 15-20% improvement.
- Link to v1: https://patch.msgid.link/20260819-740-optimize-reloading-the-reftable-stack-v1-0-6bf5305d4e43@gmail.com

---
Karthik Nayak (4):
      reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`
      reftable/stack: rename reftable_stack_new_addition()
      reftable/stack: move list lock to `struct reftable_stack`
      reftable/stack: avoid reloading the stack when already locked

 refs/reftable-backend.c         | 22 ++++-----
 reftable/reftable-stack.h       | 19 ++------
 reftable/stack.c                | 82 +++++++++++++++++-----------------
 reftable/stack.h                |  7 ++-
 t/unit-tests/u-reftable-stack.c | 99 +++++++++++++++++++++++++----------------
 5 files changed, 119 insertions(+), 110 deletions(-)

Range-diff versus v1:

1:  63f46a1517 ! 1:  8ec5578141 reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`
    @@ Commit message
           1. Unit tests, where we explicitly do not propagate the flag. These
              tests are now modified with the new status quo.
     
    -      2. `reftable_stack_clean_locked()`, which was propagating 0 to
    +      2. `reftable_stack_clean()`, which was propagating 0 to
              `reftable_stack_new_addition()` but was then manually reloading the
              stack after. Here the new flow will achieve the same, while also
              allowing us to remove the manual reload.
-:  ---------- > 2:  45d965abd3 reftable/stack: rename reftable_stack_new_addition()
2:  2796229716 ! 3:  434bba1bee reftable/stack: move list lock to `struct reftable_stack`
    @@ Commit message
         on top of `struct reftable_stack`, and we would miss the optimization in
         such paths.
     
    +    Since the lock is now shared across all additions on the same stack, a
    +    second `reftable_addition` that fails to acquire the already held lock
    +    would still call `reftable_addition_close()`, which will release the
    +    `stack->list_lock` which is still held by the first addition. To avoid
    +    this, add a new bit field `locked` to `reftable_addition` that tracks
    +    whether a particular addition is the one holding the lock, and only
    +    release it in that case. Add a unit test to validate this behavior.
    +
         While here, remove an unused header file from 'reftable/stack.h'.
     
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
    @@ reftable/stack.c: int reftable_stack_reload(struct reftable_stack *st)
      	struct reftable_stack *stack;
      	struct reftable_write_options opts;
      
    ++	/*
    ++	 * While the list lock is acquired on the stack, we need to distinguish
    ++	 * which 'reftable_addition' is responsible for the lock. This avoids
    ++	 * clearing the lock of another 'reftable_addition'.
    ++	 */
    ++	unsigned int locked : 1;
    ++
    + 	char **new_tables;
    + 	size_t new_tables_len, new_tables_cap;
    + 	uint64_t next_update_index;
     @@ reftable/stack.c: static void reftable_addition_close(struct reftable_addition *add)
      	add->new_tables_len = 0;
      	add->new_tables_cap = 0;
      
     -	flock_release(&add->tables_list_lock);
    -+	flock_release(&add->stack->list_lock);
    ++	if (add->locked)
    ++		flock_release(&add->stack->list_lock);
    ++	add->locked = 0;
      	reftable_buf_release(&nm);
      }
      
    @@ reftable/stack.c: static int reftable_stack_init_addition(struct reftable_additi
      			    add->opts.lock_timeout_ms);
      	if (err < 0)
      		goto done;
    ++	add->locked = 1;
      
      	if (add->opts.default_permissions) {
     -		if (chmod(add->tables_list_lock.path,
    @@ reftable/stack.c: int reftable_addition_commit(struct reftable_addition *add)
      	if (err < 0) {
      		err = REFTABLE_IO_ERROR;
      		goto done;
    + 	}
    ++	add->locked = 0;
    + 
    + 	/* success, no more state to clean up. */
    + 	for (i = 0; i < add->new_tables_len; i++)
     
      ## reftable/stack.h ##
     @@
    @@ reftable/stack.h: struct reftable_stack {
      	char *reftable_dir;
      
      	struct reftable_stack_options opts;
    +
    + ## t/unit-tests/u-reftable-stack.c ##
    +@@ t/unit-tests/u-reftable-stack.c: void test_reftable_stack__invalid_limit_updates(void)
    + 	reftable_stack_destroy(st);
    + 	clear_dir(dir);
    + }
    ++
    ++void test_reftable_stack__two_additions(void)
    ++{
    ++	struct reftable_stack *st = NULL;
    ++	char *dir = get_tmp_dir(__LINE__);
    ++	struct reftable_addition *add1 = NULL;
    ++	struct reftable_addition *add2 = NULL;
    ++
    ++	struct reftable_ref_record ref = {
    ++		.refname = (char *) "HEAD",
    ++		.update_index = 1,
    ++		.value_type = REFTABLE_REF_SYMREF,
    ++		.value.symref = (char *) "master",
    ++	};
    ++
    ++	cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);
    ++
    ++	cl_assert_equal_i(reftable_stack_addition_new(&add1, st, NULL), 0);
    ++	cl_assert_equal_i(reftable_stack_addition_new(&add2, st, NULL), REFTABLE_LOCK_ERROR);
    ++
    ++	cl_assert_equal_i(reftable_addition_add(add1, write_test_ref, &ref), 0);
    ++
    ++	cl_assert_equal_i(reftable_addition_commit(add1), 0);
    ++
    ++	reftable_addition_destroy(add1);
    ++	reftable_stack_destroy(st);
    ++	clear_dir(dir);
    ++}
3:  7d7e4a7a33 ! 4:  bd038809b9 reftable/stack: avoid reloading the stack when already locked
    @@ Commit message
         no-op. Now that the struct has access to the lock file status, simply
         skip reloading if the lock is present.
     
    -    Benchmarking with a fixed, non-symbolic target OID shows a modest but
    -    consistent ~1-2% improvement in clock time for `update-ref` across ref
    -    counts ranging from 2,000 to 100,000.
    +    Benchmarking with a fixed, non-symbolic target OID in the 'refs/tags/'
    +    namespace (since it triggers a stack reload when checking if reflog
    +    exists for the given tag name), shows a consistent 15-20% improvement
    +    with these patches:
     
    -    We can see better improvements in the number of syscall counts. On
    +      refcount   master     patch     speedup
    +      --------   -------    -------   -------
    +      2,000       18.5 ms    16.6 ms   1.11x
    +      20,000     120.7 ms   102.8 ms   1.17x
    +      50,000     296.5 ms   247.1 ms   1.20x
    +
    +    We can also see the improvements in the number of syscall counts. On
         master, the number of calls to `newfstatat()` grows linearly with the
         number of refs created. With this patch, the number is now a constant:
     

---
base-commit: 18e66859d87fb4b76599f73460b54f0848c76b16
change-id: 20260814-740-optimize-reloading-the-reftable-stack-f5f3adf0a0c0


Thanks
- Karthik