Re: [PATCH 3/3] reftable/stack: avoid reloading the stack when already locked
From: Justin Tobler <hidden>
Date: 2026-08-19 16:49:49
On 26/08/19 03:19PM, Karthik Nayak wrote:
When making modifications to the reftable stack, the stack obtains a lock to the list file and removes the lock after the commit phase. Since most operations reload the stack to ensure we have the latest state, any branched operation during the locked phase could trigger a state reload. To prevent data loss due to concurrent writes, state reload is necessary right after obtaining the lock. But any reloads after that are just a no-op. Now that the struct has access to the lock file status, simply skip reloading if the lock is present.
Makes sense.
quoted hunk ↗ jump to hunk
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. We can see better 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: refcount master patch -------- ------ ------ 1,000 1,059 55 5,000 5,059 55 10,000 10,059 55 20,000 20,059 55 Reported-by: Jeff King <redacted> Signed-off-by: Karthik Nayak <redacted> --- reftable/stack.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)diff --git a/reftable/stack.c b/reftable/stack.c index e449af9c03..433a611ed1 100644 --- a/reftable/stack.c +++ b/reftable/stack.c@@ -553,14 +553,21 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir, /* * Check whether the given stack is up-to-date with what we have in memory. + * If skip_if_locked is set skip stack reloading if the stack is currently + * locked. Stack reloading must _not_ be skipped right after obtaining the + * lock, to check for concurrent updates which may have happened. + * * Returns 0 if so, 1 if the stack is out-of-date or a negative error code * otherwise. */ -static int stack_uptodate(struct reftable_stack *st) +static int stack_uptodate(struct reftable_stack *st, int skip_if_locked) { char **names = NULL; int err; + if (skip_if_locked && st->list_lock.fd != -1) + return 0; + /* * When we have cached stat information available then we use it to * verify whether the file has been rewritten.@@ -623,7 +630,7 @@ static int stack_uptodate(struct reftable_stack *st) int reftable_stack_reload(struct reftable_stack *st) { - int err = stack_uptodate(st); + int err = stack_uptodate(st, 1);
Ok, this appears to be the only call site where is actually want to skip if there is a lock present. Could we instead just not invoke `stack_uptodate()` in such cases? That way we don't have to change its function signature and can leave all other existing call sites alone. -Justin