# [PATCH v2 1/4] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD` 9 messages from 2026-08-24 to 2026-08-26. Participants: Karthik Nayak, Jeff King, Patrick Steinhardt. Thread: https://gitlist.dev/t/66211 ## Karthik Nayak, 2026-08-24 09:30 Subject: [PATCH v2 1/4] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD` Message-ID: <20260824-740-optimize-reloading-the-reftable-stack-v2-1-9c9de2eb0af7@gmail.com> In-Reply-To: <20260824-740-optimize-reloading-the-reftable-stack-v2-0-9c9de2eb0af7@gmail.com> ``` In 80e7342ea8 (reftable/stack: allow locking of outdated stacks, 2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so that callers of `reftable_stack_init_addition()` can also reload the stack if there was a concurrent update made before the lock was obtained. Then 16684b6fae (refs/reftable: always reload stacks when creating lock, 2025-08-12) updated all of the remaining call-sites to propagate this flag to ensure that we always reload the stack whenever there was a concurrent update. As all calls to `reftable_stack_init_addition()` inevitably propagate the flag, it is safe to remove the flag and its associated code and make the reloading of the stack the default flow. This makes it easier to follow the flow and simplifies the logic. The only exceptions are: 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()`, 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. This also makes two checks for 'REFTABLE_OUTDATED_ERROR' redundant, so remove them also. Signed-off-by: Karthik Nayak --- refs/reftable-backend.c | 18 ++++------- reftable/reftable-stack.h | 17 ++-------- reftable/stack.c | 37 ++++++---------------- t/unit-tests/u-reftable-stack.c | 69 ++++++++++++++++++----------------------- 4 files changed, 49 insertions(+), 92 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 028f0211af..5c87fd2d68 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1003,8 +1003,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out, struct reftable_addition *addition; ret = reftable_stack_new_addition(&addition, be->stack, - &reftable_be_write_options(refs)->opts, - REFTABLE_STACK_NEW_ADDITION_RELOAD); + &reftable_be_write_options(refs)->opts); if (ret) { if (ret == REFTABLE_LOCK_ERROR) strbuf_addstr(err, "cannot lock references"); @@ -2010,8 +2009,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store, if (ret) goto done; ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, - &reftable_be_write_options(refs)->opts, - REFTABLE_STACK_NEW_ADDITION_RELOAD); + &reftable_be_write_options(refs)->opts); done: assert(ret != REFTABLE_API_ERROR); @@ -2041,8 +2039,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store, if (ret) goto done; ret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg, - &reftable_be_write_options(refs)->opts, - REFTABLE_STACK_NEW_ADDITION_RELOAD); + &reftable_be_write_options(refs)->opts); done: assert(ret != REFTABLE_API_ERROR); @@ -2424,8 +2421,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store, arg.stack = be->stack; ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg, - &reftable_be_write_options(refs)->opts, - REFTABLE_STACK_NEW_ADDITION_RELOAD); + &reftable_be_write_options(refs)->opts); done: return ret; @@ -2499,8 +2495,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store, arg.stack = be->stack; ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg, - &reftable_be_write_options(refs)->opts, - REFTABLE_STACK_NEW_ADDITION_RELOAD); + &reftable_be_write_options(refs)->opts); assert(ret != REFTABLE_API_ERROR); return ret; @@ -2622,8 +2617,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, goto done; ret = reftable_stack_new_addition(&add, be->stack, - &reftable_be_write_options(refs)->opts, - REFTABLE_STACK_NEW_ADDITION_RELOAD); + &reftable_be_write_options(refs)->opts); if (ret < 0) goto done; diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h index 5d22d84e80..5d224f8079 100644 --- a/reftable/reftable-stack.h +++ b/reftable/reftable-stack.h @@ -58,22 +58,13 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st); /* holds a transaction to add tables at the top of a stack. */ struct reftable_addition; -enum { - /* - * Reload the stack when the stack is out-of-date after locking it. - */ - REFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0), -}; - /* * returns a new transaction to add reftables to the given stack. As a side - * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_* - * flags. + * effect, the ref database is locked. */ int reftable_stack_new_addition(struct reftable_addition **dest, struct reftable_stack *st, - const struct reftable_write_options *opts, - unsigned int flags); + const struct reftable_write_options *opts); /* Adds a reftable to transaction. */ int reftable_addition_add(struct reftable_addition *add, @@ -93,14 +84,12 @@ void reftable_addition_destroy(struct reftable_addition *add); /* * Add a new table to the stack. The write_table function must call * reftable_writer_set_limits, add refs and return an error value. - * The flags are passed through to `reftable_stack_new_addition()`. */ int reftable_stack_add(struct reftable_stack *st, int (*write_table)(struct reftable_writer *wr, void *write_arg), void *write_arg, - const struct reftable_write_options *opts, - unsigned flags); + const struct reftable_write_options *opts); struct reftable_iterator; diff --git a/reftable/stack.c b/reftable/stack.c index 308f9578f0..540f5e77ac 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -659,8 +659,7 @@ static void reftable_addition_close(struct reftable_addition *add) static int reftable_stack_init_addition(struct reftable_addition *add, struct reftable_stack *st, - const struct reftable_write_options *opts, - unsigned int flags) + const struct reftable_write_options *opts) { struct reftable_buf lock_file_name = REFTABLE_BUF_INIT; int err; @@ -686,15 +685,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add, err = stack_uptodate(st); if (err < 0) goto done; - if (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) { + if (err > 0) { err = reftable_stack_reload_maybe_reuse(add->stack, 1); if (err) goto done; } - if (err > 0) { - err = REFTABLE_OUTDATED_ERROR; - goto done; - } add->next_update_index = reftable_stack_next_update_index(st); done: @@ -708,13 +703,12 @@ static int stack_try_add(struct reftable_stack *st, int (*write_table)(struct reftable_writer *wr, void *arg), void *arg, - const struct reftable_write_options *opts, - unsigned flags) + const struct reftable_write_options *opts) { struct reftable_addition add; int err; - err = reftable_stack_init_addition(&add, st, opts, flags); + err = reftable_stack_init_addition(&add, st, opts); if (err < 0) goto done; @@ -731,17 +725,10 @@ static int stack_try_add(struct reftable_stack *st, int reftable_stack_add(struct reftable_stack *st, int (*write)(struct reftable_writer *wr, void *arg), void *arg, - const struct reftable_write_options *opts, - unsigned flags) + const struct reftable_write_options *opts) { - int err = stack_try_add(st, write, arg, opts, flags); + int err = stack_try_add(st, write, arg, opts); if (err < 0) { - if (err == REFTABLE_OUTDATED_ERROR) { - /* Ignore error return, we want to propagate - REFTABLE_OUTDATED_ERROR. - */ - reftable_stack_reload(st); - } return err; } @@ -843,8 +830,7 @@ int reftable_addition_commit(struct reftable_addition *add) int reftable_stack_new_addition(struct reftable_addition **dest, struct reftable_stack *st, - const struct reftable_write_options *opts, - unsigned int flags) + const struct reftable_write_options *opts) { int err; @@ -852,7 +838,7 @@ int reftable_stack_new_addition(struct reftable_addition **dest, if (!*dest) return REFTABLE_OUT_OF_MEMORY_ERROR; - err = reftable_stack_init_addition(*dest, st, opts, flags); + err = reftable_stack_init_addition(*dest, st, opts); if (err) { reftable_free(*dest); *dest = NULL; @@ -1840,12 +1826,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st) int reftable_stack_clean(struct reftable_stack *st) { struct reftable_addition *add = NULL; - int err = reftable_stack_new_addition(&add, st, NULL, 0); - if (err < 0) { - goto done; - } - - err = reftable_stack_reload(st); + int err = reftable_stack_new_addition(&add, st, NULL); if (err < 0) { goto done; } diff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c index e6c1635940..c6254190e6 100644 --- a/t/unit-tests/u-reftable-stack.c +++ b/t/unit-tests/u-reftable-stack.c @@ -127,7 +127,7 @@ static void write_n_ref_tables(struct reftable_stack *st, cl_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1); cl_assert_equal_i(reftable_stack_add(st, - &write_test_ref, &ref, &opts, 0), 0); + &write_test_ref, &ref, &opts), 0); } } @@ -168,7 +168,7 @@ void test_reftable_stack__add_one(void) err = reftable_new_stack(&st, dir, NULL); cl_assert(!err); - err = reftable_stack_add(st, write_test_ref, &ref, &opts, 0); + err = reftable_stack_add(st, write_test_ref, &ref, &opts); cl_assert(!err); err = reftable_stack_read_ref(st, ref.refname, &dest); @@ -231,12 +231,9 @@ void test_reftable_stack__uptodate(void) cl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0); cl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0); cl_assert_equal_i(reftable_stack_add(st1, write_test_ref, - &ref1, NULL, 0), 0); + &ref1, NULL), 0); cl_assert_equal_i(reftable_stack_add(st2, write_test_ref, - &ref2, NULL, 0), REFTABLE_OUTDATED_ERROR); - cl_assert_equal_i(reftable_stack_reload(st2), 0); - cl_assert_equal_i(reftable_stack_add(st2, write_test_ref, - &ref2, NULL, 0), 0); + &ref2, NULL), 0); reftable_stack_destroy(st1); reftable_stack_destroy(st2); clear_dir(dir); @@ -260,7 +257,7 @@ void test_reftable_stack__transaction_api(void) reftable_addition_destroy(add); - cl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL, 0), 0); + cl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL), 0); cl_assert_equal_i(reftable_addition_add(add, write_test_ref, &ref), 0); cl_assert_equal_i(reftable_addition_commit(add), 0); @@ -301,21 +298,17 @@ void test_reftable_stack__transaction_with_reload(void) cl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0); cl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0); - cl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL, 0), 0); + cl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL), 0); cl_assert_equal_i(reftable_addition_add(add, write_test_ref, &refs[0]), 0); cl_assert_equal_i(reftable_addition_commit(add), 0); reftable_addition_destroy(add); /* - * The second stack is now outdated, which we should notice. We do not - * create the addition and lock the stack by default, but allow the - * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set. + * The second stack is now outdated, but it should automatically reload it + * with the newer updates. */ - cl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL, 0), - REFTABLE_OUTDATED_ERROR); - cl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL, - REFTABLE_STACK_NEW_ADDITION_RELOAD), 0); + cl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL), 0); cl_assert_equal_i(reftable_addition_add(add, write_test_ref, &refs[1]), 0); cl_assert_equal_i(reftable_addition_commit(add), 0); @@ -363,7 +356,7 @@ void test_reftable_stack__transaction_api_performs_auto_compaction(void) * better control over when exactly auto compaction runs. */ cl_assert_equal_i(reftable_stack_new_addition(&add, - st, &write_opts, 0), 0); + st, &write_opts), 0); cl_assert_equal_i(reftable_addition_add(add, write_test_ref, &ref), 0); cl_assert_equal_i(reftable_addition_commit(add), 0); @@ -400,7 +393,7 @@ void test_reftable_stack__auto_compaction_fails_gracefully(void) cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0); cl_assert_equal_i(reftable_stack_add(st, write_test_ref, - &ref, NULL, 0), 0); + &ref, NULL), 0); cl_assert_equal_i(st->merged->tables_len, 1); cl_assert_equal_i(st->stats.attempts, 0); cl_assert_equal_i(st->stats.failures, 0); @@ -418,7 +411,7 @@ void test_reftable_stack__auto_compaction_fails_gracefully(void) write_file_buf(table_path.buf, "", 0); ref.update_index = 2; - err = reftable_stack_add(st, write_test_ref, &ref, NULL, 0); + err = reftable_stack_add(st, write_test_ref, &ref, NULL); cl_assert(!err); cl_assert_equal_i(st->merged->tables_len, 2); cl_assert_equal_i(st->stats.attempts, 1); @@ -453,9 +446,9 @@ void test_reftable_stack__update_index_check(void) cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0); cl_assert_equal_i(reftable_stack_add(st, write_test_ref, - &ref1, NULL, 0), 0); + &ref1, NULL), 0); cl_assert_equal_i(reftable_stack_add(st, write_test_ref, - &ref2, NULL, 0), REFTABLE_API_ERROR); + &ref2, NULL), REFTABLE_API_ERROR); reftable_stack_destroy(st); clear_dir(dir); } @@ -469,7 +462,7 @@ void test_reftable_stack__lock_failure(void) cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0); for (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--) cl_assert_equal_i(reftable_stack_add(st, write_error, - &i, NULL, 0), i); + &i, NULL), i); reftable_stack_destroy(st); clear_dir(dir); @@ -513,7 +506,7 @@ void test_reftable_stack__add(void) for (i = 0; i < N; i++) cl_assert_equal_i(reftable_stack_add(st, write_test_ref, - &refs[i], &opts, 0), 0); + &refs[i], &opts), 0); for (i = 0; i < N; i++) { struct write_log_arg arg = { @@ -521,7 +514,7 @@ void test_reftable_stack__add(void) .update_index = reftable_stack_next_update_index(st), }; cl_assert_equal_i(reftable_stack_add(st, write_test_log, - &arg, &opts, 0), 0); + &arg, &opts), 0); } cl_assert_equal_i(reftable_stack_compact_all(st, &opts, NULL), 0); @@ -604,7 +597,7 @@ void test_reftable_stack__iterator(void) for (i = 0; i < N; i++) cl_assert_equal_i(reftable_stack_add(st, write_test_ref, - &refs[i], NULL, 0), 0); + &refs[i], NULL), 0); for (i = 0; i < N; i++) { struct write_log_arg arg = { @@ -613,7 +606,7 @@ void test_reftable_stack__iterator(void) }; cl_assert_equal_i(reftable_stack_add(st, write_test_log, - &arg, NULL, 0), 0); + &arg, NULL), 0); } reftable_stack_init_ref_iterator(st, &it); @@ -685,11 +678,11 @@ void test_reftable_stack__log_normalize(void) input.value.update.message = (char *) "one\ntwo"; cl_assert_equal_i(reftable_stack_add(st, write_test_log, - &arg, NULL, 0), REFTABLE_API_ERROR); + &arg, NULL), REFTABLE_API_ERROR); input.value.update.message = (char *) "one"; cl_assert_equal_i(reftable_stack_add(st, write_test_log, - &arg, NULL, 0), 0); + &arg, NULL), 0); cl_assert_equal_i(reftable_stack_read_log(st, input.refname, &dest), 0); cl_assert_equal_s(dest.value.update.message, "one\n"); @@ -697,7 +690,7 @@ void test_reftable_stack__log_normalize(void) input.value.update.message = (char *) "two\n"; arg.update_index = 2; cl_assert_equal_i(reftable_stack_add(st, write_test_log, - &arg, NULL, 0), 0); + &arg, NULL), 0); cl_assert_equal_i(reftable_stack_read_log(st, input.refname, &dest), 0); cl_assert_equal_s(dest.value.update.message, "two\n"); @@ -747,7 +740,7 @@ void test_reftable_stack__tombstone(void) } for (i = 0; i < N; i++) cl_assert_equal_i(reftable_stack_add(st, write_test_ref, - &refs[i], NULL, 0), 0); + &refs[i], NULL), 0); for (i = 0; i < N; i++) { struct write_log_arg arg = { @@ -755,7 +748,7 @@ void test_reftable_stack__tombstone(void) .update_index = reftable_stack_next_update_index(st), }; cl_assert_equal_i(reftable_stack_add(st, write_test_log, - &arg, NULL, 0), 0); + &arg, NULL), 0); } cl_assert_equal_i(reftable_stack_read_ref(st, "branch", @@ -801,7 +794,7 @@ void test_reftable_stack__hash_id(void) cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0); cl_assert_equal_i(reftable_stack_add(st, write_test_ref, - &ref, NULL, 0), 0); + &ref, NULL), 0); /* can't read it with the wrong hash ID. */ cl_assert_equal_i(reftable_new_stack(&st32, dir, @@ -869,7 +862,7 @@ void test_reftable_stack__reflog_expire(void) .update_index = reftable_stack_next_update_index(st), }; cl_assert_equal_i(reftable_stack_add(st, write_test_log, - &arg, NULL, 0), 0); + &arg, NULL), 0); } cl_assert_equal_i(reftable_stack_compact_all(st, NULL, NULL), 0); @@ -908,7 +901,7 @@ void test_reftable_stack__empty_add(void) cl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0); cl_assert_equal_i(reftable_stack_add(st, write_nothing, - NULL, NULL, 0), 0); + NULL, NULL), 0); cl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0); clear_dir(dir); reftable_stack_destroy(st); @@ -947,7 +940,7 @@ void test_reftable_stack__auto_compaction(void) }; snprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i); - err = reftable_stack_add(st, write_test_ref, &ref, &opts, 0); + err = reftable_stack_add(st, write_test_ref, &ref, &opts); cl_assert(!err); err = reftable_stack_auto_compact(st, &opts); @@ -983,7 +976,7 @@ void test_reftable_stack__auto_compaction_factor(void) }; xsnprintf(name, sizeof(name), "branch%04"PRIuMAX, (uintmax_t)i); - err = reftable_stack_add(st, &write_test_ref, &ref, &opts, 0); + err = reftable_stack_add(st, &write_test_ref, &ref, &opts); cl_assert(!err); cl_assert(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5)); @@ -1064,7 +1057,7 @@ void test_reftable_stack__add_performs_auto_compaction(void) ref.refname = buf; cl_assert_equal_i(reftable_stack_add(st, write_test_ref, - &ref, &write_opts, 0), 0); + &ref, &write_opts), 0); /* * The stack length should grow continuously for all runs where @@ -1303,7 +1296,7 @@ void test_reftable_stack__invalid_limit_updates(void) reftable_addition_destroy(add); - cl_assert_equal_i(reftable_stack_new_addition(&add, st, &opts, 0), 0); + cl_assert_equal_i(reftable_stack_new_addition(&add, st, &opts), 0); /* * write_limits_after_ref also updates the update indexes after adding -- 2.55.GIT ``` ## Karthik Nayak, 2026-08-24 09:31 Subject: [PATCH v2 2/4] reftable/stack: rename reftable_stack_new_addition() Message-ID: <20260824-740-optimize-reloading-the-reftable-stack-v2-2-9c9de2eb0af7@gmail.com> In-Reply-To: <20260824-740-optimize-reloading-the-reftable-stack-v2-0-9c9de2eb0af7@gmail.com> ``` Rename the function `reftable_stack_new_addition()` to `reftable_stack_addition_new()` to be more inline with our naming scheme. Signed-off-by: Karthik Nayak --- refs/reftable-backend.c | 4 ++-- reftable/reftable-stack.h | 2 +- reftable/stack.c | 4 ++-- t/unit-tests/u-reftable-stack.c | 10 +++++----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c index 5c87fd2d68..73cd794fc6 100644 --- a/refs/reftable-backend.c +++ b/refs/reftable-backend.c @@ -1002,7 +1002,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out, if (!arg) { struct reftable_addition *addition; - ret = reftable_stack_new_addition(&addition, be->stack, + ret = reftable_stack_addition_new(&addition, be->stack, &reftable_be_write_options(refs)->opts); if (ret) { if (ret == REFTABLE_LOCK_ERROR) @@ -2616,7 +2616,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store, if (ret < 0) goto done; - ret = reftable_stack_new_addition(&add, be->stack, + ret = reftable_stack_addition_new(&add, be->stack, &reftable_be_write_options(refs)->opts); if (ret < 0) goto done; diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h index 5d224f8079..875d09d241 100644 --- a/reftable/reftable-stack.h +++ b/reftable/reftable-stack.h @@ -62,7 +62,7 @@ struct reftable_addition; * returns a new transaction to add reftables to the given stack. As a side * effect, the ref database is locked. */ -int reftable_stack_new_addition(struct reftable_addition **dest, +int reftable_stack_addition_new(struct reftable_addition **dest, struct reftable_stack *st, const struct reftable_write_options *opts); diff --git a/reftable/stack.c b/reftable/stack.c index 540f5e77ac..703548417c 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -828,7 +828,7 @@ int reftable_addition_commit(struct reftable_addition *add) return err; } -int reftable_stack_new_addition(struct reftable_addition **dest, +int reftable_stack_addition_new(struct reftable_addition **dest, struct reftable_stack *st, const struct reftable_write_options *opts) { @@ -1826,7 +1826,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st) int reftable_stack_clean(struct reftable_stack *st) { struct reftable_addition *add = NULL; - int err = reftable_stack_new_addition(&add, st, NULL); + int err = reftable_stack_addition_new(&add, st, NULL); if (err < 0) { goto done; } diff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c index c6254190e6..04927113c2 100644 --- a/t/unit-tests/u-reftable-stack.c +++ b/t/unit-tests/u-reftable-stack.c @@ -257,7 +257,7 @@ void test_reftable_stack__transaction_api(void) reftable_addition_destroy(add); - cl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL), 0); + cl_assert_equal_i(reftable_stack_addition_new(&add, st, NULL), 0); cl_assert_equal_i(reftable_addition_add(add, write_test_ref, &ref), 0); cl_assert_equal_i(reftable_addition_commit(add), 0); @@ -298,7 +298,7 @@ void test_reftable_stack__transaction_with_reload(void) cl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0); cl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0); - cl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL), 0); + cl_assert_equal_i(reftable_stack_addition_new(&add, st1, NULL), 0); cl_assert_equal_i(reftable_addition_add(add, write_test_ref, &refs[0]), 0); cl_assert_equal_i(reftable_addition_commit(add), 0); @@ -308,7 +308,7 @@ void test_reftable_stack__transaction_with_reload(void) * The second stack is now outdated, but it should automatically reload it * with the newer updates. */ - cl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL), 0); + cl_assert_equal_i(reftable_stack_addition_new(&add, st2, NULL), 0); cl_assert_equal_i(reftable_addition_add(add, write_test_ref, &refs[1]), 0); cl_assert_equal_i(reftable_addition_commit(add), 0); @@ -355,7 +355,7 @@ void test_reftable_stack__transaction_api_performs_auto_compaction(void) * we can ensure that we indeed honor this setting and have * better control over when exactly auto compaction runs. */ - cl_assert_equal_i(reftable_stack_new_addition(&add, + cl_assert_equal_i(reftable_stack_addition_new(&add, st, &write_opts), 0); cl_assert_equal_i(reftable_addition_add(add, write_test_ref, &ref), 0); @@ -1296,7 +1296,7 @@ void test_reftable_stack__invalid_limit_updates(void) reftable_addition_destroy(add); - cl_assert_equal_i(reftable_stack_new_addition(&add, st, &opts), 0); + cl_assert_equal_i(reftable_stack_addition_new(&add, st, &opts), 0); /* * write_limits_after_ref also updates the update indexes after adding -- 2.55.GIT ``` ## Karthik Nayak, 2026-08-24 09:30 Subject: [PATCH v2 0/4] reftable/stack: avoid reloading the stack when locked Message-ID: <20260824-740-optimize-reloading-the-reftable-stack-v2-0-9c9de2eb0af7@gmail.com> In-Reply-To: <20260819-740-optimize-reloading-the-reftable-stack-v1-0-6bf5305d4e43@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 --- 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 @@ 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 ``` ## Karthik Nayak, 2026-08-24 09:31 Subject: [PATCH v2 3/4] reftable/stack: move list lock to `struct reftable_stack` Message-ID: <20260824-740-optimize-reloading-the-reftable-stack-v2-3-9c9de2eb0af7@gmail.com> In-Reply-To: <20260824-740-optimize-reloading-the-reftable-stack-v2-0-9c9de2eb0af7@gmail.com> ``` The struct `reftable_addition` is used to modify a given stack, as such, it also includes a `struct reftable_flock` used to obtain the lock to the list file. While the scope of the field lies within this struct, it doesn't allow for optimizations to be made on `struct reftable_stack` itself. Move the field to `struct reftable_stack`, allowing us to make a simple optimization around avoiding a stack reload when we have already obtained a lock. While this is currently possible in the write path, the write path also contains multiple branches to reads which only work 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 --- reftable/stack.c | 26 +++++++++++++++++++------- reftable/stack.h | 7 ++++++- t/unit-tests/u-reftable-stack.c | 28 ++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 8 deletions(-) diff --git a/reftable/stack.c b/reftable/stack.c index 703548417c..c3d4deff29 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -536,6 +536,8 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir, goto out; } + p->list_lock = REFTABLE_FLOCK_INIT; + err = reftable_stack_reload_maybe_reuse(p, 1); if (err < 0) goto out; @@ -628,10 +630,16 @@ int reftable_stack_reload(struct reftable_stack *st) } struct reftable_addition { - struct reftable_flock tables_list_lock; 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; @@ -653,7 +661,9 @@ 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); + if (add->locked) + flock_release(&add->stack->list_lock); + add->locked = 0; reftable_buf_release(&nm); } @@ -669,13 +679,14 @@ static int reftable_stack_init_addition(struct reftable_addition *add, if (opts) add->opts = *opts; - err = flock_acquire(&add->tables_list_lock, st->list_file, + err = flock_acquire(&add->stack->list_lock, st->list_file, 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, + if (chmod(add->stack->list_lock.path, add->opts.default_permissions) < 0) { err = REFTABLE_IO_ERROR; goto done; @@ -774,7 +785,7 @@ int reftable_addition_commit(struct reftable_addition *add) goto done; } - err = reftable_write_data(add->tables_list_lock.fd, + err = reftable_write_data(add->stack->list_lock.fd, table_list.buf, table_list.len); reftable_buf_release(&table_list); if (err < 0) { @@ -782,17 +793,18 @@ int reftable_addition_commit(struct reftable_addition *add) goto done; } - err = fsync(add->tables_list_lock.fd); + err = fsync(add->stack->list_lock.fd); if (err < 0) { err = REFTABLE_IO_ERROR; goto done; } - err = flock_commit(&add->tables_list_lock); + err = flock_commit(&add->stack->list_lock); 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++) diff --git a/reftable/stack.h b/reftable/stack.h index f7901e6c6f..52e07ad551 100644 --- a/reftable/stack.h +++ b/reftable/stack.h @@ -10,7 +10,6 @@ #define STACK_H #include "system.h" -#include "reftable-writer.h" #include "reftable-stack.h" struct reftable_stack { @@ -18,6 +17,12 @@ struct reftable_stack { char *list_file; int list_fd; + /* + * Set while an addition holds the stack locked. Used by + * stack_uptodate() to skip reload checks while locked. + */ + struct reftable_flock list_lock; + char *reftable_dir; struct reftable_stack_options opts; diff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c index 04927113c2..b6f1c6cc52 100644 --- a/t/unit-tests/u-reftable-stack.c +++ b/t/unit-tests/u-reftable-stack.c @@ -1310,3 +1310,31 @@ 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); +} -- 2.55.GIT ``` ## Karthik Nayak, 2026-08-24 09:31 Subject: [PATCH v2 4/4] reftable/stack: avoid reloading the stack when already locked Message-ID: <20260824-740-optimize-reloading-the-reftable-stack-v2-4-9c9de2eb0af7@gmail.com> In-Reply-To: <20260824-740-optimize-reloading-the-reftable-stack-v2-0-9c9de2eb0af7@gmail.com> ``` 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. 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 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: 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 Signed-off-by: Karthik Nayak --- reftable/stack.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/reftable/stack.c b/reftable/stack.c index c3d4deff29..47a60db079 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); if (err > 0) return reftable_stack_reload_maybe_reuse(st, 1); return err; @@ -693,7 +700,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add, } } - err = stack_uptodate(st); + err = stack_uptodate(st, 0); if (err < 0) goto done; if (err > 0) { @@ -1200,7 +1207,7 @@ static int stack_compact_range(struct reftable_stack *st, * we could check that relevant tables still exist. But for now it's * good enough to just abort. */ - err = stack_uptodate(st); + err = stack_uptodate(st, 0); if (err < 0) goto done; if (err > 0) { @@ -1319,7 +1326,7 @@ static int stack_compact_range(struct reftable_stack *st, * tables with our compacted version. If they don't, then we need to * abort. */ - err = stack_uptodate(st); + err = stack_uptodate(st, 0); if (err < 0) goto done; if (err > 0) { -- 2.55.GIT ``` ## Jeff King, 2026-08-24 22:52 Subject: Re: [PATCH v2 4/4] reftable/stack: avoid reloading the stack when already locked Message-ID: <20260824225202.GA190620@coredump.intra.peff.net> In-Reply-To: <20260824-740-optimize-reloading-the-reftable-stack-v2-4-9c9de2eb0af7@gmail.com> ``` On Mon, Aug 24, 2026 at 11:31:02AM +0200, Karthik Nayak wrote: > 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 Much nicer. :) I'm not very familiar with the reftable code, so you can take my review with a huge grain of salt. But I think the idea is sound, and I didn't see anything obviously wrong. Thanks for working on this. -Peff ``` ## Patrick Steinhardt, 2026-08-25 10:30 Subject: Re: [PATCH v2 3/4] reftable/stack: move list lock to `struct reftable_stack` Message-ID: In-Reply-To: <20260824-740-optimize-reloading-the-reftable-stack-v2-3-9c9de2eb0af7@gmail.com> ``` On Mon, Aug 24, 2026 at 11:31:01AM +0200, Karthik Nayak wrote: > diff --git a/reftable/stack.c b/reftable/stack.c > index 703548417c..c3d4deff29 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -628,10 +630,16 @@ int reftable_stack_reload(struct reftable_stack *st) > } > > struct reftable_addition { > - struct reftable_flock tables_list_lock; > 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; This feels somewhat fragile, as tracking state via a bit is very easy to get wrong. I would have preferred a solution where we didn't have to have this field and instead a more direct proxy. But there's not that many spots where we have to do this, so this may be okay-ish. Other than that this series looks good to me, thanks! Patrick ``` ## Karthik Nayak, 2026-08-26 09:46 Subject: Re: [PATCH v2 3/4] reftable/stack: move list lock to `struct reftable_stack` Message-ID: In-Reply-To: ``` Patrick Steinhardt writes: > On Mon, Aug 24, 2026 at 11:31:01AM +0200, Karthik Nayak wrote: >> diff --git a/reftable/stack.c b/reftable/stack.c >> index 703548417c..c3d4deff29 100644 >> --- a/reftable/stack.c >> +++ b/reftable/stack.c >> @@ -628,10 +630,16 @@ int reftable_stack_reload(struct reftable_stack *st) >> } >> >> struct reftable_addition { >> - struct reftable_flock tables_list_lock; >> 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; > > This feels somewhat fragile, as tracking state via a bit is very easy to > get wrong. I would have preferred a solution where we didn't have to > have this field and instead a more direct proxy. But there's not that > many spots where we have to do this, so this may be okay-ish. > > Other than that this series looks good to me, thanks! > > Patrick I agree, I couldn't find another solution. The alternative being, we move the bitfield to the 'reftable_stack', but that is more fragile since there are more touch fields. So I think this is okay too. Thanks ``` ## Karthik Nayak, 2026-08-26 09:47 Subject: Re: [PATCH v2 4/4] reftable/stack: avoid reloading the stack when already locked Message-ID: In-Reply-To: <20260824225202.GA190620@coredump.intra.peff.net> ``` Jeff King writes: > On Mon, Aug 24, 2026 at 11:31:02AM +0200, Karthik Nayak wrote: > >> 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 > > Much nicer. :) > > I'm not very familiar with the reftable code, so you can take my review > with a huge grain of salt. But I think the idea is sound, and I didn't > see anything obviously wrong. Thanks for working on this. > > -Peff Thanks for the report and the review :) ```