{"thread":{"id":"66195","subject":"[PATCH 0/3] reftable/stack: avoid reloading the stack when locked","startedAt":"2026-08-19T13:20:03Z","lastAt":"2026-08-24T04:59:24Z","messageCount":17,"participants":["Karthik Nayak","Justin Tobler","Junio C Hamano","Patrick Steinhardt","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"550816","messageId":"20260819-740-optimize-reloading-the-reftable-stack-v1-0-6bf5305d4e43@gmail.com","threadId":"66195","inReplyTo":null,"subject":"[PATCH 0/3] reftable/stack: avoid reloading the stack when locked","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-19T13:19:36Z","receivedAt":"2026-08-19T13:20:03Z","isPatch":true,"body":"This patch series is based on the report by Jeff [1], where he noticed\nthat when creating a lot of refs within a single reference transaction,\nthe majority of the time was spent on fstat().\n\nThe issue stems from the fact that within the reftable library we do not\ntrack Git reference transactions, as such any calls within the library\nwould potentially reload the stack to ensure that there are no\nconcurrent updates made to the stack. While this makes sense outside of\na reference transaction, within one, the stack is locked, so reloading\nthe stack is a no-op. The only time we want to reload the stack is\nimmediately after locking the list file, which is to catch any\nconcurrent updates made to the stack.\n\nThe first patch in this small series, cleans up the flow of reloading\nthe stack by providing a flag explicitly. The patch argues that since\nall flows reload the stack, the flag can be safely removed. This\nsimplifies the flow of when to reload the stack.\n\nThe next two commits move the lock variable to the reftable_stack\nstructure and then use this information to decide if reloading of the\nstack is necessary.\n\nDuring benchmarking, I first tried to benchmark adding new references\nagainst HEAD. This kicks in the DWIM ref resolution, and we iterate over\nsiz difference candidate ref names before settling on a match. Each such\nlookup reloads the stack. This happens before the reference transaction\nis created. I quickly realized that this would dominate the benchmarks,\nso the benchmarks in the third patch are against a static commit OID.\n\nThe benchmarks show a consistent 1-2% improvement in clock time for\n'git-update-ref(1)', but such low values could also be chalked to being\nwithin an error rate. However, the syscall counts show that now the\ncalls to `newfstatat()` stay constant at around 55 calls regardless of\nthe number of refs to be created. Before this would grow linearly with\nthe number of refs.\n\n[1]: https://lore.kernel.org/git/20260629203527.GA1895313@coredump.intra.peff.net/\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\nKarthik Nayak (3):\n      reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`\n      reftable/stack: move list lock to `struct reftable_stack`\n      reftable/stack: avoid reloading the stack when already locked\n\n refs/reftable-backend.c         | 18 ++++-------\n reftable/reftable-stack.h       | 17 ++--------\n reftable/stack.c                | 69 +++++++++++++++++------------------------\n reftable/stack.h                |  7 ++++-\n t/unit-tests/u-reftable-stack.c | 69 ++++++++++++++++++-----------------------\n 5 files changed, 75 insertions(+), 105 deletions(-)\n\n\n---\nbase-commit: 18e66859d87fb4b76599f73460b54f0848c76b16\nchange-id: 20260814-740-optimize-reloading-the-reftable-stack-f5f3adf0a0c0\n\n\nThanks\n- Karthik\n\n"},{"id":"550817","messageId":"20260819-740-optimize-reloading-the-reftable-stack-v1-1-6bf5305d4e43@gmail.com","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-0-6bf5305d4e43@gmail.com","subject":"[PATCH 1/3] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-19T13:19:37Z","receivedAt":"2026-08-19T13:20:05Z","isPatch":true,"body":"In 80e7342ea8 (reftable/stack: allow locking of outdated stacks,\n2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so\nthat callers of `reftable_stack_init_addition()` can also reload the\nstack if there was a concurrent update made before the lock was\nobtained.\n\nThen 16684b6fae (refs/reftable: always reload stacks when creating\nlock, 2025-08-12) updated all of the remaining call-sites to propagate\nthis flag to ensure that we always reload the stack whenever there was a\nconcurrent update.\n\nAs all calls to `reftable_stack_init_addition()` inevitably propagate\nthe flag, it is safe to remove the flag and its associated code and make\nthe reloading of the stack the default flow. This makes it easier to\nfollow the flow and simplifies the logic.\n\nThe only exceptions are:\n\n  1. Unit tests, where we explicitly do not propagate the flag. These\n     tests are now modified with the new status quo.\n\n  2. `reftable_stack_clean_locked()`, which was propagating 0 to\n     `reftable_stack_new_addition()` but was then manually reloading the\n     stack after. Here the new flow will achieve the same, while also\n     allowing us to remove the manual reload.\n\nThis also makes two checks for 'REFTABLE_OUTDATED_ERROR' redundant, so\nremove them also.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n refs/reftable-backend.c         | 18 ++++-------\n reftable/reftable-stack.h       | 17 ++--------\n reftable/stack.c                | 37 ++++++----------------\n t/unit-tests/u-reftable-stack.c | 69 ++++++++++++++++++-----------------------\n 4 files changed, 49 insertions(+), 92 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 028f0211af..5c87fd2d68 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1003,8 +1003,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,\n \t\tstruct reftable_addition *addition;\n \n \t\tret = reftable_stack_new_addition(&addition, be->stack,\n-\t\t\t\t\t\t  &reftable_be_write_options(refs)->opts,\n-\t\t\t\t\t\t  REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\t\t\t\t\t\t  &reftable_be_write_options(refs)->opts);\n \t\tif (ret) {\n \t\t\tif (ret == REFTABLE_LOCK_ERROR)\n \t\t\t\tstrbuf_addstr(err, \"cannot lock references\");\n@@ -2010,8 +2009,7 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,\n \tif (ret)\n \t\tgoto done;\n \tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,\n-\t\t\t\t &reftable_be_write_options(refs)->opts,\n-\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\t\t\t\t &reftable_be_write_options(refs)->opts);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -2041,8 +2039,7 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,\n \tif (ret)\n \t\tgoto done;\n \tret = reftable_stack_add(arg.be->stack, &write_copy_table, &arg,\n-\t\t\t\t &reftable_be_write_options(refs)->opts,\n-\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\t\t\t\t &reftable_be_write_options(refs)->opts);\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n@@ -2424,8 +2421,7 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,\n \targ.stack = be->stack;\n \n \tret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg,\n-\t\t\t\t &reftable_be_write_options(refs)->opts,\n-\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\t\t\t\t &reftable_be_write_options(refs)->opts);\n \n done:\n \treturn ret;\n@@ -2499,8 +2495,7 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,\n \targ.stack = be->stack;\n \n \tret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg,\n-\t\t\t\t &reftable_be_write_options(refs)->opts,\n-\t\t\t\t REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\t\t\t\t &reftable_be_write_options(refs)->opts);\n \n \tassert(ret != REFTABLE_API_ERROR);\n \treturn ret;\n@@ -2622,8 +2617,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \t\tgoto done;\n \n \tret = reftable_stack_new_addition(&add, be->stack,\n-\t\t\t\t\t  &reftable_be_write_options(refs)->opts,\n-\t\t\t\t\t  REFTABLE_STACK_NEW_ADDITION_RELOAD);\n+\t\t\t\t\t  &reftable_be_write_options(refs)->opts);\n \tif (ret < 0)\n \t\tgoto done;\n \ndiff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\nindex 5d22d84e80..5d224f8079 100644\n--- a/reftable/reftable-stack.h\n+++ b/reftable/reftable-stack.h\n@@ -58,22 +58,13 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n /* holds a transaction to add tables at the top of a stack. */\n struct reftable_addition;\n \n-enum {\n-\t/*\n-\t * Reload the stack when the stack is out-of-date after locking it.\n-\t */\n-\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n-};\n-\n /*\n  * returns a new transaction to add reftables to the given stack. As a side\n- * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n- * flags.\n+ * effect, the ref database is locked.\n  */\n int reftable_stack_new_addition(struct reftable_addition **dest,\n \t\t\t\tstruct reftable_stack *st,\n-\t\t\t\tconst struct reftable_write_options *opts,\n-\t\t\t\tunsigned int flags);\n+\t\t\t\tconst struct reftable_write_options *opts);\n \n /* Adds a reftable to transaction. */\n int reftable_addition_add(struct reftable_addition *add,\n@@ -93,14 +84,12 @@ void reftable_addition_destroy(struct reftable_addition *add);\n /*\n  * Add a new table to the stack. The write_table function must call\n  * reftable_writer_set_limits, add refs and return an error value.\n- * The flags are passed through to `reftable_stack_new_addition()`.\n  */\n int reftable_stack_add(struct reftable_stack *st,\n \t\t       int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t  void *write_arg),\n \t\t       void *write_arg,\n-\t\t       const struct reftable_write_options *opts,\n-\t\t       unsigned flags);\n+\t\t       const struct reftable_write_options *opts);\n \n struct reftable_iterator;\n \ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 308f9578f0..540f5e77ac 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -659,8 +659,7 @@ static void reftable_addition_close(struct reftable_addition *add)\n \n static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t\t\t\tstruct reftable_stack *st,\n-\t\t\t\t\tconst struct reftable_write_options *opts,\n-\t\t\t\t\tunsigned int flags)\n+\t\t\t\t\tconst struct reftable_write_options *opts)\n {\n \tstruct reftable_buf lock_file_name = REFTABLE_BUF_INIT;\n \tint err;\n@@ -686,15 +685,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \terr = stack_uptodate(st);\n \tif (err < 0)\n \t\tgoto done;\n-\tif (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) {\n+\tif (err > 0) {\n \t\terr = reftable_stack_reload_maybe_reuse(add->stack, 1);\n \t\tif (err)\n \t\t\tgoto done;\n \t}\n-\tif (err > 0) {\n-\t\terr = REFTABLE_OUTDATED_ERROR;\n-\t\tgoto done;\n-\t}\n \n \tadd->next_update_index = reftable_stack_next_update_index(st);\n done:\n@@ -708,13 +703,12 @@ static int stack_try_add(struct reftable_stack *st,\n \t\t\t int (*write_table)(struct reftable_writer *wr,\n \t\t\t\t\t    void *arg),\n \t\t\t void *arg,\n-\t\t\t const struct reftable_write_options *opts,\n-\t\t\t unsigned flags)\n+\t\t\t const struct reftable_write_options *opts)\n {\n \tstruct reftable_addition add;\n \tint err;\n \n-\terr = reftable_stack_init_addition(&add, st, opts, flags);\n+\terr = reftable_stack_init_addition(&add, st, opts);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -731,17 +725,10 @@ static int stack_try_add(struct reftable_stack *st,\n int reftable_stack_add(struct reftable_stack *st,\n \t\t       int (*write)(struct reftable_writer *wr, void *arg),\n \t\t       void *arg,\n-\t\t       const struct reftable_write_options *opts,\n-\t\t       unsigned flags)\n+\t\t       const struct reftable_write_options *opts)\n {\n-\tint err = stack_try_add(st, write, arg, opts, flags);\n+\tint err = stack_try_add(st, write, arg, opts);\n \tif (err < 0) {\n-\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n-\t\t\t/* Ignore error return, we want to propagate\n-\t\t\t   REFTABLE_OUTDATED_ERROR.\n-\t\t\t*/\n-\t\t\treftable_stack_reload(st);\n-\t\t}\n \t\treturn err;\n \t}\n \n@@ -843,8 +830,7 @@ int reftable_addition_commit(struct reftable_addition *add)\n \n int reftable_stack_new_addition(struct reftable_addition **dest,\n \t\t\t\tstruct reftable_stack *st,\n-\t\t\t\tconst struct reftable_write_options *opts,\n-\t\t\t\tunsigned int flags)\n+\t\t\t\tconst struct reftable_write_options *opts)\n {\n \tint err;\n \n@@ -852,7 +838,7 @@ int reftable_stack_new_addition(struct reftable_addition **dest,\n \tif (!*dest)\n \t\treturn REFTABLE_OUT_OF_MEMORY_ERROR;\n \n-\terr = reftable_stack_init_addition(*dest, st, opts, flags);\n+\terr = reftable_stack_init_addition(*dest, st, opts);\n \tif (err) {\n \t\treftable_free(*dest);\n \t\t*dest = NULL;\n@@ -1840,12 +1826,7 @@ static int reftable_stack_clean_locked(struct reftable_stack *st)\n int reftable_stack_clean(struct reftable_stack *st)\n {\n \tstruct reftable_addition *add = NULL;\n-\tint err = reftable_stack_new_addition(&add, st, NULL, 0);\n-\tif (err < 0) {\n-\t\tgoto done;\n-\t}\n-\n-\terr = reftable_stack_reload(st);\n+\tint err = reftable_stack_new_addition(&add, st, NULL);\n \tif (err < 0) {\n \t\tgoto done;\n \t}\ndiff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c\nindex e6c1635940..c6254190e6 100644\n--- a/t/unit-tests/u-reftable-stack.c\n+++ b/t/unit-tests/u-reftable-stack.c\n@@ -127,7 +127,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n \t\tcl_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);\n \n \t\tcl_assert_equal_i(reftable_stack_add(st,\n-\t\t\t\t\t\t     &write_test_ref, &ref, &opts, 0), 0);\n+\t\t\t\t\t\t     &write_test_ref, &ref, &opts), 0);\n \t}\n }\n \n@@ -168,7 +168,7 @@ void test_reftable_stack__add_one(void)\n \terr = reftable_new_stack(&st, dir, NULL);\n \tcl_assert(!err);\n \n-\terr = reftable_stack_add(st, write_test_ref, &ref, &opts, 0);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, &opts);\n \tcl_assert(!err);\n \n \terr = reftable_stack_read_ref(st, ref.refname, &dest);\n@@ -231,12 +231,9 @@ void test_reftable_stack__uptodate(void)\n \tcl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0);\n \tcl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_add(st1, write_test_ref,\n-\t\t\t\t\t     &ref1, NULL, 0), 0);\n+\t\t\t\t\t     &ref1, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_add(st2, write_test_ref,\n-\t\t\t\t\t     &ref2, NULL, 0), REFTABLE_OUTDATED_ERROR);\n-\tcl_assert_equal_i(reftable_stack_reload(st2), 0);\n-\tcl_assert_equal_i(reftable_stack_add(st2, write_test_ref,\n-\t\t\t\t\t     &ref2, NULL, 0), 0);\n+\t\t\t\t\t     &ref2, NULL), 0);\n \treftable_stack_destroy(st1);\n \treftable_stack_destroy(st2);\n \tclear_dir(dir);\n@@ -260,7 +257,7 @@ void test_reftable_stack__transaction_api(void)\n \n \treftable_addition_destroy(add);\n \n-\tcl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL, 0), 0);\n+\tcl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL), 0);\n \tcl_assert_equal_i(reftable_addition_add(add, write_test_ref,\n \t\t\t\t\t\t&ref), 0);\n \tcl_assert_equal_i(reftable_addition_commit(add), 0);\n@@ -301,21 +298,17 @@ void test_reftable_stack__transaction_with_reload(void)\n \n \tcl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0);\n \tcl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0);\n-\tcl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL, 0), 0);\n+\tcl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL), 0);\n \tcl_assert_equal_i(reftable_addition_add(add, write_test_ref,\n \t\t\t\t\t\t&refs[0]), 0);\n \tcl_assert_equal_i(reftable_addition_commit(add), 0);\n \treftable_addition_destroy(add);\n \n \t/*\n-\t * The second stack is now outdated, which we should notice. We do not\n-\t * create the addition and lock the stack by default, but allow the\n-\t * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.\n+\t * The second stack is now outdated, but it should automatically reload it\n+\t * with the newer updates.\n \t */\n-\tcl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL, 0),\n-\t\t\t\t\t\t      REFTABLE_OUTDATED_ERROR);\n-\tcl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL,\n-\t\t\t\t\t\t      REFTABLE_STACK_NEW_ADDITION_RELOAD), 0);\n+\tcl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL), 0);\n \tcl_assert_equal_i(reftable_addition_add(add, write_test_ref,\n \t\t\t\t\t\t&refs[1]), 0);\n \tcl_assert_equal_i(reftable_addition_commit(add), 0);\n@@ -363,7 +356,7 @@ void test_reftable_stack__transaction_api_performs_auto_compaction(void)\n \t\t * better control over when exactly auto compaction runs.\n \t\t */\n \t\tcl_assert_equal_i(reftable_stack_new_addition(&add,\n-\t\t\t\t\t\t\t      st, &write_opts, 0), 0);\n+\t\t\t\t\t\t\t      st, &write_opts), 0);\n \t\tcl_assert_equal_i(reftable_addition_add(add,\n \t\t\t\t\t\t\twrite_test_ref, &ref), 0);\n \t\tcl_assert_equal_i(reftable_addition_commit(add), 0);\n@@ -400,7 +393,7 @@ void test_reftable_stack__auto_compaction_fails_gracefully(void)\n \n \tcl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_add(st, write_test_ref,\n-\t\t\t\t\t     &ref, NULL, 0), 0);\n+\t\t\t\t\t     &ref, NULL), 0);\n \tcl_assert_equal_i(st->merged->tables_len, 1);\n \tcl_assert_equal_i(st->stats.attempts, 0);\n \tcl_assert_equal_i(st->stats.failures, 0);\n@@ -418,7 +411,7 @@ void test_reftable_stack__auto_compaction_fails_gracefully(void)\n \twrite_file_buf(table_path.buf, \"\", 0);\n \n \tref.update_index = 2;\n-\terr = reftable_stack_add(st, write_test_ref, &ref, NULL, 0);\n+\terr = reftable_stack_add(st, write_test_ref, &ref, NULL);\n \tcl_assert(!err);\n \tcl_assert_equal_i(st->merged->tables_len, 2);\n \tcl_assert_equal_i(st->stats.attempts, 1);\n@@ -453,9 +446,9 @@ void test_reftable_stack__update_index_check(void)\n \n \tcl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_add(st, write_test_ref,\n-\t\t\t\t\t     &ref1, NULL, 0), 0);\n+\t\t\t\t\t     &ref1, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_add(st, write_test_ref,\n-\t\t\t\t\t     &ref2, NULL, 0), REFTABLE_API_ERROR);\n+\t\t\t\t\t     &ref2, NULL), REFTABLE_API_ERROR);\n \treftable_stack_destroy(st);\n \tclear_dir(dir);\n }\n@@ -469,7 +462,7 @@ void test_reftable_stack__lock_failure(void)\n \tcl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);\n \tfor (i = -1; i != REFTABLE_EMPTY_TABLE_ERROR; i--)\n \t\tcl_assert_equal_i(reftable_stack_add(st, write_error,\n-\t\t\t\t\t\t     &i, NULL, 0), i);\n+\t\t\t\t\t\t     &i, NULL), i);\n \n \treftable_stack_destroy(st);\n \tclear_dir(dir);\n@@ -513,7 +506,7 @@ void test_reftable_stack__add(void)\n \n \tfor (i = 0; i < N; i++)\n \t\tcl_assert_equal_i(reftable_stack_add(st, write_test_ref,\n-\t\t\t\t\t\t     &refs[i], &opts, 0), 0);\n+\t\t\t\t\t\t     &refs[i], &opts), 0);\n \n \tfor (i = 0; i < N; i++) {\n \t\tstruct write_log_arg arg = {\n@@ -521,7 +514,7 @@ void test_reftable_stack__add(void)\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n \t\tcl_assert_equal_i(reftable_stack_add(st, write_test_log,\n-\t\t\t\t\t\t     &arg, &opts, 0), 0);\n+\t\t\t\t\t\t     &arg, &opts), 0);\n \t}\n \n \tcl_assert_equal_i(reftable_stack_compact_all(st, &opts, NULL), 0);\n@@ -604,7 +597,7 @@ void test_reftable_stack__iterator(void)\n \n \tfor (i = 0; i < N; i++)\n \t\tcl_assert_equal_i(reftable_stack_add(st, write_test_ref,\n-\t\t\t\t\t\t     &refs[i], NULL, 0), 0);\n+\t\t\t\t\t\t     &refs[i], NULL), 0);\n \n \tfor (i = 0; i < N; i++) {\n \t\tstruct write_log_arg arg = {\n@@ -613,7 +606,7 @@ void test_reftable_stack__iterator(void)\n \t\t};\n \n \t\tcl_assert_equal_i(reftable_stack_add(st, write_test_log,\n-\t\t\t\t\t\t     &arg, NULL, 0), 0);\n+\t\t\t\t\t\t     &arg, NULL), 0);\n \t}\n \n \treftable_stack_init_ref_iterator(st, &it);\n@@ -685,11 +678,11 @@ void test_reftable_stack__log_normalize(void)\n \n \tinput.value.update.message = (char *) \"one\\ntwo\";\n \tcl_assert_equal_i(reftable_stack_add(st, write_test_log,\n-\t\t\t\t\t     &arg, NULL, 0), REFTABLE_API_ERROR);\n+\t\t\t\t\t     &arg, NULL), REFTABLE_API_ERROR);\n \n \tinput.value.update.message = (char *) \"one\";\n \tcl_assert_equal_i(reftable_stack_add(st, write_test_log,\n-\t\t\t\t\t     &arg, NULL, 0), 0);\n+\t\t\t\t\t     &arg, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_read_log(st, input.refname,\n \t\t\t\t\t\t  &dest), 0);\n \tcl_assert_equal_s(dest.value.update.message, \"one\\n\");\n@@ -697,7 +690,7 @@ void test_reftable_stack__log_normalize(void)\n \tinput.value.update.message = (char *) \"two\\n\";\n \targ.update_index = 2;\n \tcl_assert_equal_i(reftable_stack_add(st, write_test_log,\n-\t\t\t\t\t     &arg, NULL, 0), 0);\n+\t\t\t\t\t     &arg, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_read_log(st, input.refname,\n \t\t\t\t\t\t  &dest), 0);\n \tcl_assert_equal_s(dest.value.update.message, \"two\\n\");\n@@ -747,7 +740,7 @@ void test_reftable_stack__tombstone(void)\n \t}\n \tfor (i = 0; i < N; i++)\n \t\tcl_assert_equal_i(reftable_stack_add(st, write_test_ref,\n-\t\t\t\t\t\t     &refs[i], NULL, 0), 0);\n+\t\t\t\t\t\t     &refs[i], NULL), 0);\n \n \tfor (i = 0; i < N; i++) {\n \t\tstruct write_log_arg arg = {\n@@ -755,7 +748,7 @@ void test_reftable_stack__tombstone(void)\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n \t\tcl_assert_equal_i(reftable_stack_add(st, write_test_log,\n-\t\t\t\t\t\t     &arg, NULL, 0), 0);\n+\t\t\t\t\t\t     &arg, NULL), 0);\n \t}\n \n \tcl_assert_equal_i(reftable_stack_read_ref(st, \"branch\",\n@@ -801,7 +794,7 @@ void test_reftable_stack__hash_id(void)\n \n \tcl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_add(st, write_test_ref,\n-\t\t\t\t\t     &ref, NULL, 0), 0);\n+\t\t\t\t\t     &ref, NULL), 0);\n \n \t/* can't read it with the wrong hash ID. */\n \tcl_assert_equal_i(reftable_new_stack(&st32, dir,\n@@ -869,7 +862,7 @@ void test_reftable_stack__reflog_expire(void)\n \t\t\t.update_index = reftable_stack_next_update_index(st),\n \t\t};\n \t\tcl_assert_equal_i(reftable_stack_add(st, write_test_log,\n-\t\t\t\t\t\t     &arg, NULL, 0), 0);\n+\t\t\t\t\t\t     &arg, NULL), 0);\n \t}\n \n \tcl_assert_equal_i(reftable_stack_compact_all(st, NULL, NULL), 0);\n@@ -908,7 +901,7 @@ void test_reftable_stack__empty_add(void)\n \n \tcl_assert_equal_i(reftable_new_stack(&st, dir, NULL), 0);\n \tcl_assert_equal_i(reftable_stack_add(st, write_nothing,\n-\t\t\t\t\t     NULL, NULL, 0), 0);\n+\t\t\t\t\t     NULL, NULL), 0);\n \tcl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0);\n \tclear_dir(dir);\n \treftable_stack_destroy(st);\n@@ -947,7 +940,7 @@ void test_reftable_stack__auto_compaction(void)\n \t\t};\n \t\tsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, write_test_ref, &ref, &opts, 0);\n+\t\terr = reftable_stack_add(st, write_test_ref, &ref, &opts);\n \t\tcl_assert(!err);\n \n \t\terr = reftable_stack_auto_compact(st, &opts);\n@@ -983,7 +976,7 @@ void test_reftable_stack__auto_compaction_factor(void)\n \t\t};\n \t\txsnprintf(name, sizeof(name), \"branch%04\"PRIuMAX, (uintmax_t)i);\n \n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref, &opts, 0);\n+\t\terr = reftable_stack_add(st, &write_test_ref, &ref, &opts);\n \t\tcl_assert(!err);\n \n \t\tcl_assert(i < 5 || st->merged->tables_len < 5 * fastlogN(i, 5));\n@@ -1064,7 +1057,7 @@ void test_reftable_stack__add_performs_auto_compaction(void)\n \t\tref.refname = buf;\n \n \t\tcl_assert_equal_i(reftable_stack_add(st, write_test_ref,\n-\t\t\t\t\t\t     &ref, &write_opts, 0), 0);\n+\t\t\t\t\t\t     &ref, &write_opts), 0);\n \n \t\t/*\n \t\t * The stack length should grow continuously for all runs where\n@@ -1303,7 +1296,7 @@ void test_reftable_stack__invalid_limit_updates(void)\n \n \treftable_addition_destroy(add);\n \n-\tcl_assert_equal_i(reftable_stack_new_addition(&add, st, &opts, 0), 0);\n+\tcl_assert_equal_i(reftable_stack_new_addition(&add, st, &opts), 0);\n \n \t/*\n \t * write_limits_after_ref also updates the update indexes after adding\n\n-- \n2.55.GIT\n\n"},{"id":"550818","messageId":"20260819-740-optimize-reloading-the-reftable-stack-v1-2-6bf5305d4e43@gmail.com","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-0-6bf5305d4e43@gmail.com","subject":"[PATCH 2/3] reftable/stack: move list lock to `struct reftable_stack`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-19T13:19:38Z","receivedAt":"2026-08-19T13:20:06Z","isPatch":true,"body":"The struct `reftable_addition` is used to modify a given stack, as such,\nit also includes a `struct reftable_flock` used to obtain the lock to\nthe list file. While the scope of the field lies within this struct, it\ndoesn't allow for optimizations to be made on `struct reftable_stack`\nitself.\n\nMove the field to `struct reftable_stack`, allowing us to make a simple\noptimization around avoiding a stack reload when we have already\nobtained a lock. While this is currently possible in the write path, the\nwrite path also contains multiple branches to reads which only work\non top of `struct reftable_stack`, and we would miss the optimization in\nsuch paths.\n\nWhile here, remove an unused header file from 'reftable/stack.h'.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n reftable/stack.c | 15 ++++++++-------\n reftable/stack.h |  7 ++++++-\n 2 files changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 540f5e77ac..e449af9c03 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -536,6 +536,8 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n \t\tgoto out;\n \t}\n \n+\tp->list_lock = REFTABLE_FLOCK_INIT;\n+\n \terr = reftable_stack_reload_maybe_reuse(p, 1);\n \tif (err < 0)\n \t\tgoto out;\n@@ -628,7 +630,6 @@ int reftable_stack_reload(struct reftable_stack *st)\n }\n \n struct reftable_addition {\n-\tstruct reftable_flock tables_list_lock;\n \tstruct reftable_stack *stack;\n \tstruct reftable_write_options opts;\n \n@@ -653,7 +654,7 @@ static void reftable_addition_close(struct reftable_addition *add)\n \tadd->new_tables_len = 0;\n \tadd->new_tables_cap = 0;\n \n-\tflock_release(&add->tables_list_lock);\n+\tflock_release(&add->stack->list_lock);\n \treftable_buf_release(&nm);\n }\n \n@@ -669,13 +670,13 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \tif (opts)\n \t\tadd->opts = *opts;\n \n-\terr = flock_acquire(&add->tables_list_lock, st->list_file,\n+\terr = flock_acquire(&add->stack->list_lock, st->list_file,\n \t\t\t    add->opts.lock_timeout_ms);\n \tif (err < 0)\n \t\tgoto done;\n \n \tif (add->opts.default_permissions) {\n-\t\tif (chmod(add->tables_list_lock.path,\n+\t\tif (chmod(add->stack->list_lock.path,\n \t\t\t  add->opts.default_permissions) < 0) {\n \t\t\terr = REFTABLE_IO_ERROR;\n \t\t\tgoto done;\n@@ -774,7 +775,7 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\t\tgoto done;\n \t}\n \n-\terr = reftable_write_data(add->tables_list_lock.fd,\n+\terr = reftable_write_data(add->stack->list_lock.fd,\n \t\t\t\t  table_list.buf, table_list.len);\n \treftable_buf_release(&table_list);\n \tif (err < 0) {\n@@ -782,13 +783,13 @@ int reftable_addition_commit(struct reftable_addition *add)\n \t\tgoto done;\n \t}\n \n-\terr = fsync(add->tables_list_lock.fd);\n+\terr = fsync(add->stack->list_lock.fd);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\n \t}\n \n-\terr = flock_commit(&add->tables_list_lock);\n+\terr = flock_commit(&add->stack->list_lock);\n \tif (err < 0) {\n \t\terr = REFTABLE_IO_ERROR;\n \t\tgoto done;\ndiff --git a/reftable/stack.h b/reftable/stack.h\nindex f7901e6c6f..52e07ad551 100644\n--- a/reftable/stack.h\n+++ b/reftable/stack.h\n@@ -10,7 +10,6 @@\n #define STACK_H\n \n #include \"system.h\"\n-#include \"reftable-writer.h\"\n #include \"reftable-stack.h\"\n \n struct reftable_stack {\n@@ -18,6 +17,12 @@ struct reftable_stack {\n \tchar *list_file;\n \tint list_fd;\n \n+\t/*\n+\t * Set while an addition holds the stack locked. Used by\n+\t * stack_uptodate() to skip reload checks while locked.\n+\t */\n+\tstruct reftable_flock list_lock;\n+\n \tchar *reftable_dir;\n \n \tstruct reftable_stack_options opts;\n\n-- \n2.55.GIT\n\n"},{"id":"550819","messageId":"20260819-740-optimize-reloading-the-reftable-stack-v1-3-6bf5305d4e43@gmail.com","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-0-6bf5305d4e43@gmail.com","subject":"[PATCH 3/3] reftable/stack: avoid reloading the stack when already locked","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-19T13:19:39Z","receivedAt":"2026-08-19T13:20:09Z","isPatch":true,"body":"When making modifications to the reftable stack, the stack obtains a\nlock to the list file and removes the lock after the commit phase. Since\nmost operations reload the stack to ensure we have the latest state, any\nbranched operation during the locked phase could trigger a state reload.\n\nTo prevent data loss due to concurrent writes, state reload is necessary\nright after obtaining the lock. But any reloads after that are just a\nno-op. Now that the struct has access to the lock file status, simply\nskip reloading if the lock is present.\n\nBenchmarking with a fixed, non-symbolic target OID shows a modest but\nconsistent ~1-2% improvement in clock time for `update-ref` across ref\ncounts ranging from 2,000 to 100,000.\n\nWe can see better improvements in the number of syscall counts. On\nmaster, the number of calls to `newfstatat()` grows linearly with the\nnumber of refs created. With this patch, the number is now a constant:\n\n  refcount   master   patch\n  --------   ------   ------\n  1,000      1,059       55\n  5,000      5,059       55\n  10,000     10,059      55\n  20,000     20,059      55\n\nReported-by: Jeff King <peff@peff.net>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n reftable/stack.c | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex e449af9c03..433a611ed1 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -553,14 +553,21 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n \n /*\n  * Check whether the given stack is up-to-date with what we have in memory.\n+ * If skip_if_locked is set skip stack reloading if the stack is currently\n+ * locked. Stack reloading must _not_ be skipped right after obtaining the\n+ * lock, to check for concurrent updates which may have happened.\n+ *\n  * Returns 0 if so, 1 if the stack is out-of-date or a negative error code\n  * otherwise.\n  */\n-static int stack_uptodate(struct reftable_stack *st)\n+static int stack_uptodate(struct reftable_stack *st, int skip_if_locked)\n {\n \tchar **names = NULL;\n \tint err;\n \n+\tif (skip_if_locked && st->list_lock.fd != -1)\n+\t\treturn 0;\n+\n \t/*\n \t * When we have cached stat information available then we use it to\n \t * verify whether the file has been rewritten.\n@@ -623,7 +630,7 @@ static int stack_uptodate(struct reftable_stack *st)\n \n int reftable_stack_reload(struct reftable_stack *st)\n {\n-\tint err = stack_uptodate(st);\n+\tint err = stack_uptodate(st, 1);\n \tif (err > 0)\n \t\treturn reftable_stack_reload_maybe_reuse(st, 1);\n \treturn err;\n@@ -683,7 +690,7 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n \t\t}\n \t}\n \n-\terr = stack_uptodate(st);\n+\terr = stack_uptodate(st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \tif (err > 0) {\n@@ -1189,7 +1196,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * we could check that relevant tables still exist. But for now it's\n \t * good enough to just abort.\n \t */\n-\terr = stack_uptodate(st);\n+\terr = stack_uptodate(st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \tif (err > 0) {\n@@ -1308,7 +1315,7 @@ static int stack_compact_range(struct reftable_stack *st,\n \t * tables with our compacted version. If they don't, then we need to\n \t * abort.\n \t */\n-\terr = stack_uptodate(st);\n+\terr = stack_uptodate(st, 0);\n \tif (err < 0)\n \t\tgoto done;\n \tif (err > 0) {\n\n-- \n2.55.GIT\n\n"},{"id":"550828","messageId":"aoXUrsAiDvgS2s6H@denethor","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-1-6bf5305d4e43@gmail.com","subject":"Re: [PATCH 1/3] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-08-19T16:28:30Z","receivedAt":"2026-08-19T16:28:36Z","isPatch":true,"body":"On 26/08/19 03:19PM, Karthik Nayak wrote:\n> In 80e7342ea8 (reftable/stack: allow locking of outdated stacks,\n> 2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so\n> that callers of `reftable_stack_init_addition()` can also reload the\n> stack if there was a concurrent update made before the lock was\n> obtained.\n> \n> Then 16684b6fae (refs/reftable: always reload stacks when creating\n> lock, 2025-08-12) updated all of the remaining call-sites to propagate\n> this flag to ensure that we always reload the stack whenever there was a\n> concurrent update.\n\nOk, if all call sites already wire this flag, then we probably don't\nneed if anymore.\n\n> As all calls to `reftable_stack_init_addition()` inevitably propagate\n> the flag, it is safe to remove the flag and its associated code and make\n> the reloading of the stack the default flow. This makes it easier to\n> follow the flow and simplifies the logic.\n\nMakes sense.\n\n> The only exceptions are:\n> \n>   1. Unit tests, where we explicitly do not propagate the flag. These\n>      tests are now modified with the new status quo.\n\nI assume this means we no longer need to test for the case where we\ndon't reload.\n\n>   2. `reftable_stack_clean_locked()`, which was propagating 0 to\n\nDid you mean `reftable_stack_clean()`?\n\n>      `reftable_stack_new_addition()` but was then manually reloading the\n>      stack after. Here the new flow will achieve the same, while also\n>      allowing us to remove the manual reload.\n\nOut of curiousity, was this call site just forgotten previously? Or was\nthere any reason a manual reload was useful?\n\n> This also makes two checks for 'REFTABLE_OUTDATED_ERROR' redundant, so\n> remove them also.\n> \n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n[snip]\n> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\n> index 5d22d84e80..5d224f8079 100644\n> --- a/reftable/reftable-stack.h\n> +++ b/reftable/reftable-stack.h\n> @@ -58,22 +58,13 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n>  /* holds a transaction to add tables at the top of a stack. */\n>  struct reftable_addition;\n>  \n> -enum {\n> -\t/*\n> -\t * Reload the stack when the stack is out-of-date after locking it.\n> -\t */\n> -\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n> -};\n\nThe flag is dropped now that it is the only behavior.\n\n>  /*\n>   * returns a new transaction to add reftables to the given stack. As a side\n> - * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n> - * flags.\n> + * effect, the ref database is locked.\n>   */\n>  int reftable_stack_new_addition(struct reftable_addition **dest,\n>  \t\t\t\tstruct reftable_stack *st,\n> -\t\t\t\tconst struct reftable_write_options *opts,\n> -\t\t\t\tunsigned int flags);\n> +\t\t\t\tconst struct reftable_write_options *opts);\n\nSignatures updated. Ok.\n\n[snip]\n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index 308f9578f0..540f5e77ac 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -659,8 +659,7 @@ static void reftable_addition_close(struct reftable_addition *add)\n>  \n>  static int reftable_stack_init_addition(struct reftable_addition *add,\n>  \t\t\t\t\tstruct reftable_stack *st,\n> -\t\t\t\t\tconst struct reftable_write_options *opts,\n> -\t\t\t\t\tunsigned int flags)\n> +\t\t\t\t\tconst struct reftable_write_options *opts)\n>  {\n>  \tstruct reftable_buf lock_file_name = REFTABLE_BUF_INIT;\n>  \tint err;\n> @@ -686,15 +685,11 @@ static int reftable_stack_init_addition(struct reftable_addition *add,\n>  \terr = stack_uptodate(st);\n>  \tif (err < 0)\n>  \t\tgoto done;\n> -\tif (err > 0 && flags & REFTABLE_STACK_NEW_ADDITION_RELOAD) {\n> +\tif (err > 0) {\n>  \t\terr = reftable_stack_reload_maybe_reuse(add->stack, 1);\n>  \t\tif (err)\n>  \t\t\tgoto done;\n>  \t}\n> -\tif (err > 0) {\n> -\t\terr = REFTABLE_OUTDATED_ERROR;\n> -\t\tgoto done;\n> -\t}\n\n`reftable_stack_init_addition()` now reload unconditionally. Looks good.\n\n>  \tadd->next_update_index = reftable_stack_next_update_index(st);\n>  done:\n> @@ -708,13 +703,12 @@ static int stack_try_add(struct reftable_stack *st,\n>  \t\t\t int (*write_table)(struct reftable_writer *wr,\n>  \t\t\t\t\t    void *arg),\n>  \t\t\t void *arg,\n> -\t\t\t const struct reftable_write_options *opts,\n> -\t\t\t unsigned flags)\n> +\t\t\t const struct reftable_write_options *opts)\n>  {\n>  \tstruct reftable_addition add;\n>  \tint err;\n>  \n> -\terr = reftable_stack_init_addition(&add, st, opts, flags);\n> +\terr = reftable_stack_init_addition(&add, st, opts);\n>  \tif (err < 0)\n>  \t\tgoto done;\n>  \n> @@ -731,17 +725,10 @@ static int stack_try_add(struct reftable_stack *st,\n>  int reftable_stack_add(struct reftable_stack *st,\n>  \t\t       int (*write)(struct reftable_writer *wr, void *arg),\n>  \t\t       void *arg,\n> -\t\t       const struct reftable_write_options *opts,\n> -\t\t       unsigned flags)\n> +\t\t       const struct reftable_write_options *opts)\n>  {\n> -\tint err = stack_try_add(st, write, arg, opts, flags);\n> +\tint err = stack_try_add(st, write, arg, opts);\n>  \tif (err < 0) {\n> -\t\tif (err == REFTABLE_OUTDATED_ERROR) {\n> -\t\t\t/* Ignore error return, we want to propagate\n> -\t\t\t   REFTABLE_OUTDATED_ERROR.\n> -\t\t\t*/\n> -\t\t\treftable_stack_reload(st);\n> -\t\t}\n\nSince we always reload now, the REFTABLE_OUTDATED_ERROR is no longer a\npossibility and doesn't need to be handled anymore.\n\n[snip]\n> diff --git a/t/unit-tests/u-reftable-stack.c b/t/unit-tests/u-reftable-stack.c\n> index e6c1635940..c6254190e6 100644\n> --- a/t/unit-tests/u-reftable-stack.c\n> +++ b/t/unit-tests/u-reftable-stack.c\n> @@ -127,7 +127,7 @@ static void write_n_ref_tables(struct reftable_stack *st,\n>  \t\tcl_reftable_set_hash(ref.value.val1, i, REFTABLE_HASH_SHA1);\n>  \n>  \t\tcl_assert_equal_i(reftable_stack_add(st,\n> -\t\t\t\t\t\t     &write_test_ref, &ref, &opts, 0), 0);\n> +\t\t\t\t\t\t     &write_test_ref, &ref, &opts), 0);\n>  \t}\n>  }\n>  \n> @@ -168,7 +168,7 @@ void test_reftable_stack__add_one(void)\n>  \terr = reftable_new_stack(&st, dir, NULL);\n>  \tcl_assert(!err);\n>  \n> -\terr = reftable_stack_add(st, write_test_ref, &ref, &opts, 0);\n> +\terr = reftable_stack_add(st, write_test_ref, &ref, &opts);\n>  \tcl_assert(!err);\n>  \n>  \terr = reftable_stack_read_ref(st, ref.refname, &dest);\n> @@ -231,12 +231,9 @@ void test_reftable_stack__uptodate(void)\n>  \tcl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0);\n>  \tcl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0);\n>  \tcl_assert_equal_i(reftable_stack_add(st1, write_test_ref,\n> -\t\t\t\t\t     &ref1, NULL, 0), 0);\n> +\t\t\t\t\t     &ref1, NULL), 0);\n>  \tcl_assert_equal_i(reftable_stack_add(st2, write_test_ref,\n> -\t\t\t\t\t     &ref2, NULL, 0), REFTABLE_OUTDATED_ERROR);\n> -\tcl_assert_equal_i(reftable_stack_reload(st2), 0);\n> -\tcl_assert_equal_i(reftable_stack_add(st2, write_test_ref,\n> -\t\t\t\t\t     &ref2, NULL, 0), 0);\n> +\t\t\t\t\t     &ref2, NULL), 0);\n\nWe no longer need to check for REFTABLE_OUTDATED_ERROR since the stack\nis always reloaded now. Makes sense.\n\n>  \treftable_stack_destroy(st1);\n>  \treftable_stack_destroy(st2);\n>  \tclear_dir(dir);\n> @@ -260,7 +257,7 @@ void test_reftable_stack__transaction_api(void)\n>  \n>  \treftable_addition_destroy(add);\n>  \n> -\tcl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL, 0), 0);\n> +\tcl_assert_equal_i(reftable_stack_new_addition(&add, st, NULL), 0);\n>  \tcl_assert_equal_i(reftable_addition_add(add, write_test_ref,\n>  \t\t\t\t\t\t&ref), 0);\n>  \tcl_assert_equal_i(reftable_addition_commit(add), 0);\n> @@ -301,21 +298,17 @@ void test_reftable_stack__transaction_with_reload(void)\n>  \n>  \tcl_assert_equal_i(reftable_new_stack(&st1, dir, NULL), 0);\n>  \tcl_assert_equal_i(reftable_new_stack(&st2, dir, NULL), 0);\n> -\tcl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL, 0), 0);\n> +\tcl_assert_equal_i(reftable_stack_new_addition(&add, st1, NULL), 0);\n>  \tcl_assert_equal_i(reftable_addition_add(add, write_test_ref,\n>  \t\t\t\t\t\t&refs[0]), 0);\n>  \tcl_assert_equal_i(reftable_addition_commit(add), 0);\n>  \treftable_addition_destroy(add);\n>  \n>  \t/*\n> -\t * The second stack is now outdated, which we should notice. We do not\n> -\t * create the addition and lock the stack by default, but allow the\n> -\t * reload to happen when REFTABLE_STACK_NEW_ADDITION_RELOAD is set.\n> +\t * The second stack is now outdated, but it should automatically reload it\n> +\t * with the newer updates.\n>  \t */\n> -\tcl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL, 0),\n> -\t\t\t\t\t\t      REFTABLE_OUTDATED_ERROR);\n> -\tcl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL,\n> -\t\t\t\t\t\t      REFTABLE_STACK_NEW_ADDITION_RELOAD), 0);\n> +\tcl_assert_equal_i(reftable_stack_new_addition(&add, st2, NULL), 0);\n\nSame here.\n\nThe rest of this patch is just updating call sites and looks good.\n\n-Justin\n"},{"id":"550829","messageId":"aoXaDW1Ifjys8HTr@denethor","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-2-6bf5305d4e43@gmail.com","subject":"Re: [PATCH 2/3] reftable/stack: move list lock to `struct reftable_stack`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-08-19T16:39:52Z","receivedAt":"2026-08-19T16:39:56Z","isPatch":true,"body":"On 26/08/19 03:19PM, Karthik Nayak wrote:\n> The struct `reftable_addition` is used to modify a given stack, as such,\n> it also includes a `struct reftable_flock` used to obtain the lock to\n> the list file. While the scope of the field lies within this struct, it\n> doesn't allow for optimizations to be made on `struct reftable_stack`\n> itself.\n\nHmmm IIUC, there can only be a single lock for the reftable stack\ncorrect? If that is the case, it sounds like `struct reftable_stack` may\nconceptually be the better place for the field regardless.\n\n> Move the field to `struct reftable_stack`, allowing us to make a simple\n> optimization around avoiding a stack reload when we have already\n> obtained a lock. While this is currently possible in the write path, the\n> write path also contains multiple branches to reads which only work\n> on top of `struct reftable_stack`, and we would miss the optimization in\n> such paths.\n\nOk, so if we know the reftable stack is alreay locked, there is no need\nto reload it since it can't change. Makes sense.\n\n> While here, remove an unused header file from 'reftable/stack.h'.\n> \n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n[snip]\n>  struct reftable_stack {\n> @@ -18,6 +17,12 @@ struct reftable_stack {\n>  \tchar *list_file;\n>  \tint list_fd;\n>  \n> +\t/*\n> +\t * Set while an addition holds the stack locked. Used by\n> +\t * stack_uptodate() to skip reload checks while locked.\n> +\t */\n> +\tstruct reftable_flock list_lock;\n> +\n\nAs mentioned in the log message, the lock is now tracked in `struct\nreftable_stack` and the rest of this patch just wires it accordingly.\nLooks good.\n\n-Justin\n"},{"id":"550830","messageId":"aoXcvhFbUJruALIe@denethor","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-3-6bf5305d4e43@gmail.com","subject":"Re: [PATCH 3/3] reftable/stack: avoid reloading the stack when already locked","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-08-19T16:49:47Z","receivedAt":"2026-08-19T16:49:49Z","isPatch":true,"body":"On 26/08/19 03:19PM, Karthik Nayak wrote:\n> When making modifications to the reftable stack, the stack obtains a\n> lock to the list file and removes the lock after the commit phase. Since\n> most operations reload the stack to ensure we have the latest state, any\n> branched operation during the locked phase could trigger a state reload.\n> \n> To prevent data loss due to concurrent writes, state reload is necessary\n> right after obtaining the lock. But any reloads after that are just a\n> no-op. Now that the struct has access to the lock file status, simply\n> skip reloading if the lock is present.\n\nMakes sense.\n\n> Benchmarking with a fixed, non-symbolic target OID shows a modest but\n> consistent ~1-2% improvement in clock time for `update-ref` across ref\n> counts ranging from 2,000 to 100,000.\n> \n> We can see better improvements in the number of syscall counts. On\n> master, the number of calls to `newfstatat()` grows linearly with the\n> number of refs created. With this patch, the number is now a constant:\n> \n>   refcount   master   patch\n>   --------   ------   ------\n>   1,000      1,059       55\n>   5,000      5,059       55\n>   10,000     10,059      55\n>   20,000     20,059      55\n> \n> Reported-by: Jeff King <peff@peff.net>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n>  reftable/stack.c | 17 ++++++++++++-----\n>  1 file changed, 12 insertions(+), 5 deletions(-)\n> \n> diff --git a/reftable/stack.c b/reftable/stack.c\n> index e449af9c03..433a611ed1 100644\n> --- a/reftable/stack.c\n> +++ b/reftable/stack.c\n> @@ -553,14 +553,21 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n>  \n>  /*\n>   * Check whether the given stack is up-to-date with what we have in memory.\n> + * If skip_if_locked is set skip stack reloading if the stack is currently\n> + * locked. Stack reloading must _not_ be skipped right after obtaining the\n> + * lock, to check for concurrent updates which may have happened.\n> + *\n>   * Returns 0 if so, 1 if the stack is out-of-date or a negative error code\n>   * otherwise.\n>   */\n> -static int stack_uptodate(struct reftable_stack *st)\n> +static int stack_uptodate(struct reftable_stack *st, int skip_if_locked)\n>  {\n>  \tchar **names = NULL;\n>  \tint err;\n>  \n> +\tif (skip_if_locked && st->list_lock.fd != -1)\n> +\t\treturn 0;\n> +\n>  \t/*\n>  \t * When we have cached stat information available then we use it to\n>  \t * verify whether the file has been rewritten.\n> @@ -623,7 +630,7 @@ static int stack_uptodate(struct reftable_stack *st)\n>  \n>  int reftable_stack_reload(struct reftable_stack *st)\n>  {\n> -\tint err = stack_uptodate(st);\n> +\tint err = stack_uptodate(st, 1);\n\nOk, this appears to be the only call site where is actually want to skip\nif there is a lock present. Could we instead just not invoke\n`stack_uptodate()` in such cases? That way we don't have to change its\nfunction signature and can leave all other existing call sites alone.\n\n-Justin\n"},{"id":"550832","messageId":"xmqq33wavxko.fsf@gitster.g","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-2-6bf5305d4e43@gmail.com","subject":"Re: [PATCH 2/3] reftable/stack: move list lock to `struct reftable_stack`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-08-19T17:17:59Z","receivedAt":"2026-08-19T17:18:05Z","isPatch":true,"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> The struct `reftable_addition` is used to modify a given stack, as such,\n> it also includes a `struct reftable_flock` used to obtain the lock to\n> the list file. While the scope of the field lies within this struct, it\n> doesn't allow for optimizations to be made on `struct reftable_stack`\n> itself.\n>\n> Move the field to `struct reftable_stack`, allowing us to make a simple\n> optimization around avoiding a stack reload when we have already\n> obtained a lock. While this is currently possible in the write path, the\n> write path also contains multiple branches to reads which only work\n> on top of `struct reftable_stack`, and we would miss the optimization in\n> such paths.\n\nAs long as nobody tries to open a nested or concurrent addition on\nthe same 'struct reftable_stack', this should be safe, but do we\ngive enough tools to help the API users avoid doing so?\n\nI may be misreading the code completely, but when a caller already\nholds a lock after calling reftable_stack_init_addition() on an\ninstance of reftable_stack, and then adds another reftable_addition\non the same reftable_stack, flock_acquire(add->stack->list_lock)\nwould fail because the lock is per stack now, unlike the original\ncode where the lock was per reftable_addition.  We jump to the\ndone: label and call reftable_addition_close(), which would release\nthe lock, which is now shared with other reftable_addition\ninstances that work on the same stack, which in turn would get the\nholders of the lock into trouble, no?\n\n\n"},{"id":"550864","messageId":"aoaV1GBPWwvTsYRm@pks.im","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-1-6bf5305d4e43@gmail.com","subject":"Re: [PATCH 1/3] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-08-20T05:51:16Z","receivedAt":"2026-08-20T05:51:23Z","isPatch":true,"body":"On Wed, Aug 19, 2026 at 03:19:37PM +0200, Karthik Nayak wrote:\n> In 80e7342ea8 (reftable/stack: allow locking of outdated stacks,\n> 2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so\n> that callers of `reftable_stack_init_addition()` can also reload the\n> stack if there was a concurrent update made before the lock was\n> obtained.\n> \n> Then 16684b6fae (refs/reftable: always reload stacks when creating\n> lock, 2025-08-12) updated all of the remaining call-sites to propagate\n> this flag to ensure that we always reload the stack whenever there was a\n> concurrent update.\n> \n> As all calls to `reftable_stack_init_addition()` inevitably propagate\n> the flag, it is safe to remove the flag and its associated code and make\n> the reloading of the stack the default flow. This makes it easier to\n> follow the flow and simplifies the logic.\n> \n> The only exceptions are:\n> \n>   1. Unit tests, where we explicitly do not propagate the flag. These\n>      tests are now modified with the new status quo.\n> \n>   2. `reftable_stack_clean_locked()`, which was propagating 0 to\n>      `reftable_stack_new_addition()` but was then manually reloading the\n>      stack after. Here the new flow will achieve the same, while also\n>      allowing us to remove the manual reload.\n\nlibgit2 uses this flag though, so we'd have to adapt it, too. As far as\nI can see though all of the calls to `reftable_stack_add()` it has pass\nthis flag.\n\n> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\n> index 5d22d84e80..5d224f8079 100644\n> --- a/reftable/reftable-stack.h\n> +++ b/reftable/reftable-stack.h\n> @@ -58,22 +58,13 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n>  /* holds a transaction to add tables at the top of a stack. */\n>  struct reftable_addition;\n>  \n> -enum {\n> -\t/*\n> -\t * Reload the stack when the stack is out-of-date after locking it.\n> -\t */\n> -\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n> -};\n> -\n>  /*\n>   * returns a new transaction to add reftables to the given stack. As a side\n> - * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n> - * flags.\n> + * effect, the ref database is locked.\n>   */\n>  int reftable_stack_new_addition(struct reftable_addition **dest,\n>  \t\t\t\tstruct reftable_stack *st,\n> -\t\t\t\tconst struct reftable_write_options *opts,\n> -\t\t\t\tunsigned int flags);\n> +\t\t\t\tconst struct reftable_write_options *opts);\n>  \n>  /* Adds a reftable to transaction. */\n>  int reftable_addition_add(struct reftable_addition *add,\n\nWe're already busy adapting this function anyway, so do we maybe want to\nfix its name to `reftable_stack_addition_new` while at it?\n\nPatrick\n"},{"id":"550870","messageId":"20260820075342.GA2761530@coredump.intra.peff.net","threadId":"66195","inReplyTo":"20260819-740-optimize-reloading-the-reftable-stack-v1-3-6bf5305d4e43@gmail.com","subject":"Re: [PATCH 3/3] reftable/stack: avoid reloading the stack when already locked","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-20T07:53:42Z","receivedAt":"2026-08-20T07:53:51Z","isPatch":true,"body":"On Wed, Aug 19, 2026 at 03:19:39PM +0200, Karthik Nayak wrote:\n\n> Benchmarking with a fixed, non-symbolic target OID shows a modest but\n> consistent ~1-2% improvement in clock time for `update-ref` across ref\n> counts ranging from 2,000 to 100,000.\n\nInteresting. I get ~25% speedup with this patch, doing this:\n\n  git init --ref-format=reftable\n  cp -a .git/reftable reftable.orig\n  seq -f \"create refs/tags/foo-%g $blob\" 50000 >input\n  hyperfine -p 'rm -rf .git/reftable; cp -a reftable.orig .git/reftable' \\\n           -L v old,new \\\n\t   './git.{v} update-ref --stdin <input'\n\n(where git.old and git.new are builds before and after your series).\nWith 50,000 refs I get:\n\n  Benchmark 1: ./git.old update-ref --stdin <input\n    Time (mean ± σ):     125.8 ms ±   4.4 ms    [User: 91.2 ms, System: 34.5 ms]\n    Range (min … max):   121.0 ms … 135.2 ms    21 runs\n  \n  Benchmark 2: ./git.new update-ref --stdin <input\n    Time (mean ± σ):     100.4 ms ±   3.1 ms    [User: 90.9 ms, System: 9.4 ms]\n    Range (min … max):    95.0 ms … 106.0 ms    29 runs\n  \n  Summary\n    ./git.new update-ref --stdin <input ran\n      1.25 ± 0.06 times faster than ./git.old update-ref --stdin <input\n\nAnd it seems to scale down linearly. With 10,000 it's:\n\n  Benchmark 1: ./git.old update-ref --stdin <input\n    Time (mean ± σ):      24.2 ms ±   1.4 ms    [User: 17.1 ms, System: 7.1 ms]\n    Range (min … max):    22.6 ms …  32.8 ms    83 runs\n  \n  Benchmark 2: ./git.new update-ref --stdin <input\n    Time (mean ± σ):      19.2 ms ±   1.0 ms    [User: 16.9 ms, System: 2.4 ms]\n    Range (min … max):    17.9 ms …  25.8 ms    135 runs\n  \n  Summary\n    ./git.new update-ref --stdin <input ran\n      1.26 ± 0.10 times faster than ./git.old update-ref --stdin <input\n\nSo 1/5 as much work took 1/5 as much time, but we still saved 25% of the\nrelative time with the patch.\n\nI'm a little curious why we such get different numbers, but it may not\nbe worth digging too deep. Avoiding unnecessary syscalls seems worth it\nto me regardless, as they can sometimes be more expensive you expect\n(say, on a networked filesystem).\n\n-Peff\n"},{"id":"550948","messageId":"CAOLa=ZRa9Dfdoj4XCpjhDJRn_mqyZ+91LNi5KTocyi6OrbsHbQ@mail.gmail.com","threadId":"66195","inReplyTo":"aoXUrsAiDvgS2s6H@denethor","subject":"Re: [PATCH 1/3] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-20T21:15:58Z","receivedAt":"2026-08-20T21:16:01Z","isPatch":true,"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> On 26/08/19 03:19PM, Karthik Nayak wrote:\n>> In 80e7342ea8 (reftable/stack: allow locking of outdated stacks,\n>> 2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so\n>> that callers of `reftable_stack_init_addition()` can also reload the\n>> stack if there was a concurrent update made before the lock was\n>> obtained.\n>>\n>> Then 16684b6fae (refs/reftable: always reload stacks when creating\n>> lock, 2025-08-12) updated all of the remaining call-sites to propagate\n>> this flag to ensure that we always reload the stack whenever there was a\n>> concurrent update.\n>\n> Ok, if all call sites already wire this flag, then we probably don't\n> need if anymore.\n>\n>> As all calls to `reftable_stack_init_addition()` inevitably propagate\n>> the flag, it is safe to remove the flag and its associated code and make\n>> the reloading of the stack the default flow. This makes it easier to\n>> follow the flow and simplifies the logic.\n>\n> Makes sense.\n>\n>> The only exceptions are:\n>>\n>>   1. Unit tests, where we explicitly do not propagate the flag. These\n>>      tests are now modified with the new status quo.\n>\n> I assume this means we no longer need to test for the case where we\n> don't reload.\n\nThere is no longer a 'don't reload' flow.\n\n>\n>>   2. `reftable_stack_clean_locked()`, which was propagating 0 to\n>\n> Did you mean `reftable_stack_clean()`?\n>\n\nGood catch, `reftable_stack_clean()` calls\n`reftable_stack_clean_locked()`, but I should have mentioned\n`reftable_stack_clean()`.\n\n>>      `reftable_stack_new_addition()` but was then manually reloading the\n>>      stack after. Here the new flow will achieve the same, while also\n>>      allowing us to remove the manual reload.\n>\n> Out of curiousity, was this call site just forgotten previously? Or was\n> there any reason a manual reload was useful?\n>\n\nMy understanding was we added the flag at first for a few sites and then\nexpanded it. I guess reftable_stack_clean() sending in 0 was the\nblocker, but we do indeed reload the stack manually there.\n\n[snip]\n"},{"id":"550949","messageId":"CAOLa=ZRPwdsWNV_YUDNmUY2J839=SbkBbtqrbfgBxjDVZ6PrxA@mail.gmail.com","threadId":"66195","inReplyTo":"aoaV1GBPWwvTsYRm@pks.im","subject":"Re: [PATCH 1/3] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-20T21:20:36Z","receivedAt":"2026-08-20T21:20:40Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Wed, Aug 19, 2026 at 03:19:37PM +0200, Karthik Nayak wrote:\n>> In 80e7342ea8 (reftable/stack: allow locking of outdated stacks,\n>> 2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so\n>> that callers of `reftable_stack_init_addition()` can also reload the\n>> stack if there was a concurrent update made before the lock was\n>> obtained.\n>>\n>> Then 16684b6fae (refs/reftable: always reload stacks when creating\n>> lock, 2025-08-12) updated all of the remaining call-sites to propagate\n>> this flag to ensure that we always reload the stack whenever there was a\n>> concurrent update.\n>>\n>> As all calls to `reftable_stack_init_addition()` inevitably propagate\n>> the flag, it is safe to remove the flag and its associated code and make\n>> the reloading of the stack the default flow. This makes it easier to\n>> follow the flow and simplifies the logic.\n>>\n>> The only exceptions are:\n>>\n>>   1. Unit tests, where we explicitly do not propagate the flag. These\n>>      tests are now modified with the new status quo.\n>>\n>>   2. `reftable_stack_clean_locked()`, which was propagating 0 to\n>>      `reftable_stack_new_addition()` but was then manually reloading the\n>>      stack after. Here the new flow will achieve the same, while also\n>>      allowing us to remove the manual reload.\n>\n> libgit2 uses this flag though, so we'd have to adapt it, too. As far as\n> I can see though all of the calls to `reftable_stack_add()` it has pass\n> this flag.\n>\n\nOkay, that should be simple then! I can send in a pull request when/if\nthis lands.\n\n>> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\n>> index 5d22d84e80..5d224f8079 100644\n>> --- a/reftable/reftable-stack.h\n>> +++ b/reftable/reftable-stack.h\n>> @@ -58,22 +58,13 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n>>  /* holds a transaction to add tables at the top of a stack. */\n>>  struct reftable_addition;\n>>\n>> -enum {\n>> -\t/*\n>> -\t * Reload the stack when the stack is out-of-date after locking it.\n>> -\t */\n>> -\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n>> -};\n>> -\n>>  /*\n>>   * returns a new transaction to add reftables to the given stack. As a side\n>> - * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n>> - * flags.\n>> + * effect, the ref database is locked.\n>>   */\n>>  int reftable_stack_new_addition(struct reftable_addition **dest,\n>>  \t\t\t\tstruct reftable_stack *st,\n>> -\t\t\t\tconst struct reftable_write_options *opts,\n>> -\t\t\t\tunsigned int flags);\n>> +\t\t\t\tconst struct reftable_write_options *opts);\n>>\n>>  /* Adds a reftable to transaction. */\n>>  int reftable_addition_add(struct reftable_addition *add,\n>\n> We're already busy adapting this function anyway, so do we maybe want to\n> fix its name to `reftable_stack_addition_new` while at it?\n>\n> Patrick\n\nI'm assuming you're talking about `reftable_stack_new_addition`? We\ncould, I could add another commit here.\n"},{"id":"550950","messageId":"CAOLa=ZTP66UT0Az5F2MBQ2aNPpcnKD+LOo8xwKJ2Skj4RdPEug@mail.gmail.com","threadId":"66195","inReplyTo":"aoaV1GBPWwvTsYRm@pks.im","subject":"Re: [PATCH 1/3] reftable/stack: remove `REFTABLE_STACK_NEW_ADDITION_RELOAD`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-20T21:23:45Z","receivedAt":"2026-08-20T21:23:48Z","isPatch":true,"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> On Wed, Aug 19, 2026 at 03:19:37PM +0200, Karthik Nayak wrote:\n>> In 80e7342ea8 (reftable/stack: allow locking of outdated stacks,\n>> 2024-09-24), the `REFTABLE_STACK_NEW_ADDITION_RELOAD` was introduced so\n>> that callers of `reftable_stack_init_addition()` can also reload the\n>> stack if there was a concurrent update made before the lock was\n>> obtained.\n>>\n>> Then 16684b6fae (refs/reftable: always reload stacks when creating\n>> lock, 2025-08-12) updated all of the remaining call-sites to propagate\n>> this flag to ensure that we always reload the stack whenever there was a\n>> concurrent update.\n>>\n>> As all calls to `reftable_stack_init_addition()` inevitably propagate\n>> the flag, it is safe to remove the flag and its associated code and make\n>> the reloading of the stack the default flow. This makes it easier to\n>> follow the flow and simplifies the logic.\n>>\n>> The only exceptions are:\n>>\n>>   1. Unit tests, where we explicitly do not propagate the flag. These\n>>      tests are now modified with the new status quo.\n>>\n>>   2. `reftable_stack_clean_locked()`, which was propagating 0 to\n>>      `reftable_stack_new_addition()` but was then manually reloading the\n>>      stack after. Here the new flow will achieve the same, while also\n>>      allowing us to remove the manual reload.\n>\n> libgit2 uses this flag though, so we'd have to adapt it, too. As far as\n> I can see though all of the calls to `reftable_stack_add()` it has pass\n> this flag.\n>\n\nOkay, that should be simple then! I can send in a pull request when/if\nthis lands.\n\n>> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h\n>> index 5d22d84e80..5d224f8079 100644\n>> --- a/reftable/reftable-stack.h\n>> +++ b/reftable/reftable-stack.h\n>> @@ -58,22 +58,13 @@ uint64_t reftable_stack_next_update_index(struct reftable_stack *st);\n>>  /* holds a transaction to add tables at the top of a stack. */\n>>  struct reftable_addition;\n>>\n>> -enum {\n>> -\t/*\n>> -\t * Reload the stack when the stack is out-of-date after locking it.\n>> -\t */\n>> -\tREFTABLE_STACK_NEW_ADDITION_RELOAD = (1 << 0),\n>> -};\n>> -\n>>  /*\n>>   * returns a new transaction to add reftables to the given stack. As a side\n>> - * effect, the ref database is locked. Accepts REFTABLE_STACK_NEW_ADDITION_*\n>> - * flags.\n>> + * effect, the ref database is locked.\n>>   */\n>>  int reftable_stack_new_addition(struct reftable_addition **dest,\n>>  \t\t\t\tstruct reftable_stack *st,\n>> -\t\t\t\tconst struct reftable_write_options *opts,\n>> -\t\t\t\tunsigned int flags);\n>> +\t\t\t\tconst struct reftable_write_options *opts);\n>>\n>>  /* Adds a reftable to transaction. */\n>>  int reftable_addition_add(struct reftable_addition *add,\n>\n> We're already busy adapting this function anyway, so do we maybe want to\n> fix its name to `reftable_stack_addition_new` while at it?\n>\n> Patrick\n\nI'm assuming you're talking about `reftable_stack_new_addition`? We\ncould, I could add another commit here.\n"},{"id":"551078","messageId":"CAOLa=ZR2Krkxpz_2wWygosV4NSLCuc1m33-iHqdyAXADDvyaSg@mail.gmail.com","threadId":"66195","inReplyTo":"xmqq33wavxko.fsf@gitster.g","subject":"Re: [PATCH 2/3] reftable/stack: move list lock to `struct reftable_stack`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-23T15:28:15Z","receivedAt":"2026-08-23T15:28:17Z","isPatch":true,"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> The struct `reftable_addition` is used to modify a given stack, as such,\n>> it also includes a `struct reftable_flock` used to obtain the lock to\n>> the list file. While the scope of the field lies within this struct, it\n>> doesn't allow for optimizations to be made on `struct reftable_stack`\n>> itself.\n>>\n>> Move the field to `struct reftable_stack`, allowing us to make a simple\n>> optimization around avoiding a stack reload when we have already\n>> obtained a lock. While this is currently possible in the write path, the\n>> write path also contains multiple branches to reads which only work\n>> on top of `struct reftable_stack`, and we would miss the optimization in\n>> such paths.\n>\n> As long as nobody tries to open a nested or concurrent addition on\n> the same 'struct reftable_stack', this should be safe, but do we\n> give enough tools to help the API users avoid doing so?\n>\n> I may be misreading the code completely, but when a caller already\n> holds a lock after calling reftable_stack_init_addition() on an\n> instance of reftable_stack, and then adds another reftable_addition\n> on the same reftable_stack, flock_acquire(add->stack->list_lock)\n> would fail because the lock is per stack now, unlike the original\n> code where the lock was per reftable_addition.  We jump to the\n> done: label and call reftable_addition_close(), which would release\n> the lock, which is now shared with other reftable_addition\n> instances that work on the same stack, which in turn would get the\n> holders of the lock into trouble, no?\n\nThat's a good line of thought and something I didn't think of.\n\nWith the current version, this wouldn't work, as you mentioned, the\nsecond `reftable_addition` would free the first's lock. The only way I\ncan think of is each `reftable_addition` also holding it's own bit\nindicating if it acquired the lock and only release the stack lock based\non this bit. This works, I will write a unit test to also validate this\nbehavior. But I'm wondering if there is a better design.\n"},{"id":"551079","messageId":"CAOLa=ZT4WuBh2eansJhzMm5F39UCTiOP1vgQ+yfQK0syzbm1uw@mail.gmail.com","threadId":"66195","inReplyTo":"aoXcvhFbUJruALIe@denethor","subject":"Re: [PATCH 3/3] reftable/stack: avoid reloading the stack when already locked","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-23T15:39:08Z","receivedAt":"2026-08-23T15:39:12Z","isPatch":true,"body":"Justin Tobler <jltobler@gmail.com> writes:\n\n> On 26/08/19 03:19PM, Karthik Nayak wrote:\n>> When making modifications to the reftable stack, the stack obtains a\n>> lock to the list file and removes the lock after the commit phase. Since\n>> most operations reload the stack to ensure we have the latest state, any\n>> branched operation during the locked phase could trigger a state reload.\n>>\n>> To prevent data loss due to concurrent writes, state reload is necessary\n>> right after obtaining the lock. But any reloads after that are just a\n>> no-op. Now that the struct has access to the lock file status, simply\n>> skip reloading if the lock is present.\n>\n> Makes sense.\n>\n>> Benchmarking with a fixed, non-symbolic target OID shows a modest but\n>> consistent ~1-2% improvement in clock time for `update-ref` across ref\n>> counts ranging from 2,000 to 100,000.\n>>\n>> We can see better improvements in the number of syscall counts. On\n>> master, the number of calls to `newfstatat()` grows linearly with the\n>> number of refs created. With this patch, the number is now a constant:\n>>\n>>   refcount   master   patch\n>>   --------   ------   ------\n>>   1,000      1,059       55\n>>   5,000      5,059       55\n>>   10,000     10,059      55\n>>   20,000     20,059      55\n>>\n>> Reported-by: Jeff King <peff@peff.net>\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> ---\n>>  reftable/stack.c | 17 ++++++++++++-----\n>>  1 file changed, 12 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/reftable/stack.c b/reftable/stack.c\n>> index e449af9c03..433a611ed1 100644\n>> --- a/reftable/stack.c\n>> +++ b/reftable/stack.c\n>> @@ -553,14 +553,21 @@ int reftable_new_stack(struct reftable_stack **dest, const char *dir,\n>>\n>>  /*\n>>   * Check whether the given stack is up-to-date with what we have in memory.\n>> + * If skip_if_locked is set skip stack reloading if the stack is currently\n>> + * locked. Stack reloading must _not_ be skipped right after obtaining the\n>> + * lock, to check for concurrent updates which may have happened.\n>> + *\n>>   * Returns 0 if so, 1 if the stack is out-of-date or a negative error code\n>>   * otherwise.\n>>   */\n>> -static int stack_uptodate(struct reftable_stack *st)\n>> +static int stack_uptodate(struct reftable_stack *st, int skip_if_locked)\n>>  {\n>>  \tchar **names = NULL;\n>>  \tint err;\n>>\n>> +\tif (skip_if_locked && st->list_lock.fd != -1)\n>> +\t\treturn 0;\n>> +\n>>  \t/*\n>>  \t * When we have cached stat information available then we use it to\n>>  \t * verify whether the file has been rewritten.\n>> @@ -623,7 +630,7 @@ static int stack_uptodate(struct reftable_stack *st)\n>>\n>>  int reftable_stack_reload(struct reftable_stack *st)\n>>  {\n>> -\tint err = stack_uptodate(st);\n>> +\tint err = stack_uptodate(st, 1);\n>\n> Ok, this appears to be the only call site where is actually want to skip\n> if there is a lock present. Could we instead just not invoke\n> `stack_uptodate()` in such cases? That way we don't have to change its\n> function signature and can leave all other existing call sites alone.\n>\n> -Justin\n\nWe want to selectively invoke `stack_uptodate()` based on if the lock\nfile exists. If we move that logic outside of `stack_uptodate()` further\ncallees would have to replicate that logic. While that's not an issue,\nmissing it becomes easier. So I made this explicit choice.\n"},{"id":"551098","messageId":"CAOLa=ZQpeCKzQ3EVXQEhfxL1khUH0YD6_Kc1qDQhxoN926rsBw@mail.gmail.com","threadId":"66195","inReplyTo":"20260820075342.GA2761530@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] reftable/stack: avoid reloading the stack when already locked","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-08-23T17:39:48Z","receivedAt":"2026-08-23T17:39:50Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Aug 19, 2026 at 03:19:39PM +0200, Karthik Nayak wrote:\n>\n>> Benchmarking with a fixed, non-symbolic target OID shows a modest but\n>> consistent ~1-2% improvement in clock time for `update-ref` across ref\n>> counts ranging from 2,000 to 100,000.\n>\n> Interesting. I get ~25% speedup with this patch, doing this:\n>\n>   git init --ref-format=reftable\n>   cp -a .git/reftable reftable.orig\n>   seq -f \"create refs/tags/foo-%g $blob\" 50000 >input\n>   hyperfine -p 'rm -rf .git/reftable; cp -a reftable.orig .git/reftable' \\\n>            -L v old,new \\\n> \t   './git.{v} update-ref --stdin <input'\n>\n> (where git.old and git.new are builds before and after your series).\n> With 50,000 refs I get:\n>\n>   Benchmark 1: ./git.old update-ref --stdin <input\n>     Time (mean ± σ):     125.8 ms ±   4.4 ms    [User: 91.2 ms, System: 34.5 ms]\n>     Range (min … max):   121.0 ms … 135.2 ms    21 runs\n>\n>   Benchmark 2: ./git.new update-ref --stdin <input\n>     Time (mean ± σ):     100.4 ms ±   3.1 ms    [User: 90.9 ms, System: 9.4 ms]\n>     Range (min … max):    95.0 ms … 106.0 ms    29 runs\n>\n>   Summary\n>     ./git.new update-ref --stdin <input ran\n>       1.25 ± 0.06 times faster than ./git.old update-ref --stdin <input\n>\n> And it seems to scale down linearly. With 10,000 it's:\n>\n>   Benchmark 1: ./git.old update-ref --stdin <input\n>     Time (mean ± σ):      24.2 ms ±   1.4 ms    [User: 17.1 ms, System: 7.1 ms]\n>     Range (min … max):    22.6 ms …  32.8 ms    83 runs\n>\n>   Benchmark 2: ./git.new update-ref --stdin <input\n>     Time (mean ± σ):      19.2 ms ±   1.0 ms    [User: 16.9 ms, System: 2.4 ms]\n>     Range (min … max):    17.9 ms …  25.8 ms    135 runs\n>\n>   Summary\n>     ./git.new update-ref --stdin <input ran\n>       1.26 ± 0.10 times faster than ./git.old update-ref --stdin <input\n>\n> So 1/5 as much work took 1/5 as much time, but we still saved 25% of the\n> relative time with the patch.\n>\n> I'm a little curious why we such get different numbers, but it may not\n> be worth digging too deep. Avoiding unnecessary syscalls seems worth it\n> to me regardless, as they can sometimes be more expensive you expect\n> (say, on a networked filesystem).\n>\n> -Peff\n\nI can reproduce your results locally too. I was a bit stumbled why,\nI was using a modified version of our benchmarks repository [1], which\nwas using a fixed static target.\n\nThe difference was I was updating 'refs/heads/*' and your script does\n'refs/tags/*'. The difference is in `should_write_log()`, where for\nLOG_REFS_NORMAL and 'refs/heads/*' we shortcut to creating the logs.\nWhile for tags, we do a check to see reflog already exists. This causes\na stack reload (before my patches). This shows the significant\ndifference in our benchmarks.\n\nFunnily, I use 'refs/tags/*' for strace, so you do see the diff there.\nWill modify my commit message to reflect the benchmark :)\n\n[1]: https://gitlab.com/gitlab-org/data-access/git/benchmarks/\n"},{"id":"551104","messageId":"20260824045922.GC142844@coredump.intra.peff.net","threadId":"66195","inReplyTo":"CAOLa=ZQpeCKzQ3EVXQEhfxL1khUH0YD6_Kc1qDQhxoN926rsBw@mail.gmail.com","subject":"Re: [PATCH 3/3] reftable/stack: avoid reloading the stack when already locked","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-08-24T04:59:22Z","receivedAt":"2026-08-24T04:59:24Z","isPatch":true,"body":"On Sun, Aug 23, 2026 at 01:39:48PM -0400, Karthik Nayak wrote:\n\n> I can reproduce your results locally too. I was a bit stumbled why,\n> I was using a modified version of our benchmarks repository [1], which\n> was using a fixed static target.\n> \n> The difference was I was updating 'refs/heads/*' and your script does\n> 'refs/tags/*'. The difference is in `should_write_log()`, where for\n> LOG_REFS_NORMAL and 'refs/heads/*' we shortcut to creating the logs.\n> While for tags, we do a check to see reflog already exists. This causes\n> a stack reload (before my patches). This shows the significant\n> difference in our benchmarks.\n\nAh, yeah. The refs/heads/ case should already have been fast, at least\nfor the default config. Makes sense.\n\n-Peff\n"}]}