{"thread":{"id":"61255","subject":"[PATCH 0/9] reftable: optimize write performance","startedAt":"2024-04-02T17:29:53Z","lastAt":"2024-04-09T03:16:39Z","messageCount":49,"participants":["Patrick Steinhardt","Junio C Hamano","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"492083","messageId":"cover.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":null,"subject":"[PATCH 0/9] reftable: optimize write performance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:29:47Z","receivedAt":"2024-04-02T17:29:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is my first patch series taking an actual look at write performance\nfor the reftable backend. This series addresses two major pain points:\n\n - Duplicate directory/file conflict checks when writing refs.\n\n - Allocation churn when compressing log blocks.\n\nOverall though I found that there is not much of a point to investigate\nwrite performance in the reftable library itself, at least not right\nnow. This is mostly because the write performance is heavily dominated\nby random ref reads. And while past patch series have optimized scanning\nthrough refs linearly, seeking random refs isn't well-optimized yet. So\nonce all in-flight series relating to reftable performance have landed I\nwill focus on random ref reads next.\n\nFor the bigger picture, the following benchmarks show perfomance\ncompared to the \"files\" backend after applying this patch series.\n\nWriting many refs in a single transaction:\n\n Benchmark 1: update-ref: create many refs (refformat = files, refcount = 100000)\n Time (mean ± σ): 10.085 s ± 0.057 s [User: 1.876 s, System: 8.161 s]\n Range (min … max): 10.013 s … 10.202 s 10 runs\n\n Benchmark 2: update-ref: create many refs (refformat = reftable, refcount = 100000)\n Time (mean ± σ): 2.768 s ± 0.018 s [User: 1.381 s, System: 1.383 s]\n Range (min … max): 2.745 s … 2.804 s 10 runs\n\n Summary\n update-ref: create many refs (refformat = reftable, refcount = 100000) ran\n 3.64 ± 0.03 times faster than update-ref: create many refs (refformat = files, refcount = 100000)\n\nAnd for writing many refs sequentially in separate transactions:\n\n Benchmark 1: update-ref: create refs sequentially (refformat = files, refcount = 10000)\n Time (mean ± σ): 40.286 s ± 0.086 s [User: 22.241 s, System: 17.912 s]\n Range (min … max): 40.166 s … 40.410 s 10 runs\n\n Benchmark 2: update-ref: create refs sequentially (refformat = reftable, refcount = 10000)\n Time (mean ± σ): 44.046 s ± 0.137 s [User: 23.790 s, System: 20.146 s]\n Range (min … max): 43.813 s … 44.301 s 10 runs\n\n Summary\n update-ref: create refs sequentially (refformat = files, refcount = 10000) ran\n 1.09 ± 0.00 times faster than update-ref: create refs sequentially (refformat = reftable, refcount = 10000)\n\nThis is to the best of my knowledge last area where the \"files\" backend\noutperforms the \"reftable\" backend. This is partially also due to the\nfact that writes perform auto-compaction with the \"reftable\" backend.\n\nPatrick\n\nPatrick Steinhardt (9):\n refs/reftable: fix D/F conflict error message on ref copy\n refs/reftable: perform explicit D/F check when writing symrefs\n refs/reftable: skip duplicate name checks\n refs/reftable: don't recompute committer ident\n reftable/writer: refactorings for `writer_add_record()`\n reftable/writer: refactorings for `writer_flush_nonempty_block()`\n reftable/block: reuse zstream when writing log blocks\n reftable/block: reuse compressed array\n reftable/writer: reset `last_key` instead of releasing it\n\n refs/reftable-backend.c | 80 ++++++++++++++++++-------\n reftable/block.c | 83 ++++++++++++++++----------\n reftable/block.h | 4 ++\n reftable/writer.c | 119 ++++++++++++++++++++++++-------------\n t/t0610-reftable-basics.sh | 35 ++++++++++-\n 5 files changed, 227 insertions(+), 94 deletions(-)\n\n-- \n2.44.GIT\n\n"},{"id":"492084","messageId":"14b4dacd731a7d9c19029cd8a0c3b6170c31ae25.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 1/9] refs/reftable: fix D/F conflict error message on ref copy","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:29:51Z","receivedAt":"2024-04-02T17:29:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `write_copy_table()` function is shared between the reftable\nimplementations for renaming and copying refs. The only difference\nbetween those two cases is that the rename will also delete the old\nreference, whereas copying won't.\n\nThis has resulted in a bug though where we don't properly verify refname\navailability. When calling `refs_verify_refname_available()`, we always\nadd the old ref name to the list of refs to be skipped when computing\navailability, which indicates that the name would be available even if\nit already exists at the current point in time. This is only the right\nthing to do for renames though, not for copies.\n\nThe consequence of this bug is quite harmless because the reftable\nbackend has its own checks for D/F conflicts further down in the call\nstack, and thus we refuse the update regardless of the bug. But all the\nuser gets in this case is an uninformative message that copying the ref\nhas failed, without any further details.\n\nFix the bug and only add the old name to the skip-list in case we rename\nthe ref. Consequently, this error case will now be handled by\n`refs_verify_refname_available()`, which knows to provide a proper error\nmessage.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 3 ++-\n t/t0610-reftable-basics.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex e206d5a073..0358da14db 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1351,7 +1351,8 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \t/*\n \t * Verify that the new refname is available.\n \t */\n-\tstring_list_insert(&skip, arg->oldname);\n+\tif (arg->delete_old)\n+\t\tstring_list_insert(&skip, arg->oldname);\n \tret = refs_verify_refname_available(&arg->refs->base, arg->newname,\n \t\t\t\t\t NULL, &skip, &errbuf);\n \tif (ret < 0) {\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 686781192e..055231a707 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -730,6 +730,39 @@ test_expect_success 'reflog: updates via HEAD update HEAD reflog' '\n \t)\n '\n \n+test_expect_success 'branch: copying branch with D/F conflict' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit branch branch &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${SQ}refs/heads/branch${SQ} exists; cannot create ${SQ}refs/heads/branch/moved${SQ}\n+\t\tfatal: branch copy failed\n+\t\tEOF\n+\t\ttest_must_fail git branch -c branch branch/moved 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'branch: moving branch with D/F conflict' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit branch branch &&\n+\t\tgit branch conflict &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${SQ}refs/heads/conflict${SQ} exists; cannot create ${SQ}refs/heads/conflict/moved${SQ}\n+\t\tfatal: branch rename failed\n+\t\tEOF\n+\t\ttest_must_fail git branch -m branch conflict/moved 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'worktree: adding worktree creates separate stack' '\n \ttest_when_finished \"rm -rf repo worktree\" &&\n \tgit init repo &&\n-- \n2.44.GIT\n\n"},{"id":"492085","messageId":"55db366e61c9292fef0e1d0c61be1da105023bab.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 2/9] refs/reftable: perform explicit D/F check when writing symrefs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:29:56Z","receivedAt":"2024-04-02T17:29:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We already perform explicit D/F checks in all reftable callbacks which\nwrite refs, except when writing symrefs. For one this leads to an error\nmessage which isn't perfectly actionable because we only tell the user\nthat there was a D/F conflict, but not which refs conflicted with each\nother. But second, once all ref updating callbacks explicitly check for\nD/F conflicts, we can disable the D/F checks in the reftable library\nitself and thus avoid some duplicated efforts.\n\nRefactor the code that writes symref tables to explicitly call into\n`refs_verify_refname_available()` when writing symrefs.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 20 +++++++++++++++++---\n t/t0610-reftable-basics.sh | 2 +-\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 0358da14db..8a54b0d8b2 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1217,6 +1217,7 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,\n struct write_create_symref_arg {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_stack *stack;\n+\tstruct strbuf *err;\n \tconst char *refname;\n \tconst char *target;\n \tconst char *logmsg;\n@@ -1239,6 +1240,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \n \treftable_writer_set_limits(writer, ts, ts);\n \n+\tret = refs_verify_refname_available(&create->refs->base, create->refname,\n+\t\t\t\t\t NULL, NULL, create->err);\n+\tif (ret < 0)\n+\t\treturn ret;\n+\n \tret = reftable_writer_add_ref(writer, &ref);\n \tif (ret)\n \t\treturn ret;\n@@ -1280,12 +1286,14 @@ static int reftable_be_create_symref(struct ref_store *ref_store,\n \tstruct reftable_ref_store *refs =\n \t\treftable_be_downcast(ref_store, REF_STORE_WRITE, \"create_symref\");\n \tstruct reftable_stack *stack = stack_for(refs, refname, &refname);\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct write_create_symref_arg arg = {\n \t\t.refs = refs,\n \t\t.stack = stack,\n \t\t.refname = refname,\n \t\t.target = target,\n \t\t.logmsg = logmsg,\n+\t\t.err = &err,\n \t};\n \tint ret;\n \n@@ -1301,9 +1309,15 @@ static int reftable_be_create_symref(struct ref_store *ref_store,\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n-\tif (ret)\n-\t\terror(\"unable to write symref for %s: %s\", refname,\n-\t\t reftable_error_str(ret));\n+\tif (ret) {\n+\t\tif (err.len)\n+\t\t\terror(\"%s\", err.buf);\n+\t\telse\n+\t\t\terror(\"unable to write symref for %s: %s\", refname,\n+\t\t\t reftable_error_str(ret));\n+\t}\n+\n+\tstrbuf_release(&err);\n \treturn ret;\n }\n \ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 055231a707..12b0004781 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -255,7 +255,7 @@ test_expect_success 'ref transaction: creating symbolic ref fails with F/D confl\n \tgit init repo &&\n \ttest_commit -C repo A &&\n \tcat >expect <<-EOF &&\n-\terror: unable to write symref for refs/heads: file/directory conflict\n+\terror: ${SQ}refs/heads/main${SQ} exists; cannot create ${SQ}refs/heads${SQ}\n \tEOF\n \ttest_must_fail git -C repo symbolic-ref refs/heads refs/heads/foo 2>err &&\n \ttest_cmp expect err\n-- \n2.44.GIT\n\n"},{"id":"492086","messageId":"ad8210ec65562452e5a3b3beadb2453941120b43.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 3/9] refs/reftable: skip duplicate name checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:30:00Z","receivedAt":"2024-04-02T17:30:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"All the callback functions which write refs in the reftable backend\nperform D/F conflict checks via `refs_verify_refname_available()`. But\nin reality we perform these D/F conflict checks a second time in the\nreftable library via `stack_check_addition()`.\n\nInterestingly, the code in the reftable library is inferior compared to\nthe generic function:\n\n - It is slower than `refs_verify_refname_available()`, even though\n this can probably be optimized.\n\n - It does not provide a proper error message to the caller, and thus\n all the user would see is a generic \"file/directory conflict\"\n message.\n\nDisable the D/F conflict checks in the reftable library by setting the\n`skip_name_check` write option. This results in a non-negligible speedup\nwhen writing many refs. The following benchmark writes 100k refs in a\nsingle transaction:\n\n Benchmark 1: update-ref: create many refs (HEAD~)\n Time (mean ± σ): 3.241 s ± 0.040 s [User: 1.854 s, System: 1.381 s]\n Range (min … max): 3.185 s … 3.454 s 100 runs\n\n Benchmark 2: update-ref: create many refs (HEAD)\n Time (mean ± σ): 2.878 s ± 0.024 s [User: 1.506 s, System: 1.367 s]\n Range (min … max): 2.838 s … 2.960 s 100 runs\n\n Summary\n update-ref: create many refs (HEAD~) ran\n 1.13 ± 0.02 times faster than update-ref: create many refs (HEAD)\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 8a54b0d8b2..7515dd3019 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -247,6 +247,11 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.block_size = 4096;\n \trefs->write_options.hash_id = repo->hash_algo->format_id;\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n+\t/*\n+\t * We verify names via `refs_verify_refname_available()`, so there is\n+\t * no need to do the same checks in the reftable library again.\n+\t */\n+\trefs->write_options.skip_name_check = 1;\n \n \t/*\n \t * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.\n-- \n2.44.GIT\n\n"},{"id":"492087","messageId":"a9a6795c025b23035bfdd3e23b0113df9f6c5e4b.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 4/9] refs/reftable: don't recompute committer ident","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:30:04Z","receivedAt":"2024-04-02T17:30:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to write reflog entries we need to compute the committer's\nidentity as it becomes encoded in the log record itself. In the reftable\nbackend, computing the identity is repeated for every single reflog\nentry which we are about to write in a transaction. Needless to say,\nthis can be quite a waste of effort when writing many refs with reflog\nentries in a single transaction.\n\nRefactor the code to pre-compute the committer information. This results\nin a small speedup when writing 100000 refs in a single transaction:\n\n Benchmark 1: update-ref: create many refs (HEAD~)\n Time (mean ± σ): 2.895 s ± 0.020 s [User: 1.516 s, System: 1.374 s]\n Range (min … max): 2.868 s … 2.983 s 100 runs\n\n Benchmark 2: update-ref: create many refs (HEAD)\n Time (mean ± σ): 2.845 s ± 0.017 s [User: 1.461 s, System: 1.379 s]\n Range (min … max): 2.803 s … 2.913 s 100 runs\n\n Summary\n update-ref: create many refs (HEAD) ran\n 1.02 ± 0.01 times faster than update-ref: create many refs (HEAD~)\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 52 +++++++++++++++++++++++++++--------------\n 1 file changed, 34 insertions(+), 18 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 7515dd3019..9e8967e82f 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -171,32 +171,30 @@ static int should_write_log(struct ref_store *refs, const char *refname)\n \t}\n }\n \n-static void fill_reftable_log_record(struct reftable_log_record *log)\n+static void fill_reftable_log_record(struct reftable_log_record *log, const struct ident_split *split)\n {\n-\tconst char *info = git_committer_info(0);\n-\tstruct ident_split split = {0};\n+\tconst char *tz_begin;\n \tint sign = 1;\n \n-\tif (split_ident_line(&split, info, strlen(info)))\n-\t\tBUG(\"failed splitting committer info\");\n-\n \treftable_log_record_release(log);\n \tlog->value_type = REFTABLE_LOG_UPDATE;\n \tlog->value.update.name =\n-\t\txstrndup(split.name_begin, split.name_end - split.name_begin);\n+\t\txstrndup(split->name_begin, split->name_end - split->name_begin);\n \tlog->value.update.email =\n-\t\txstrndup(split.mail_begin, split.mail_end - split.mail_begin);\n-\tlog->value.update.time = atol(split.date_begin);\n-\tif (*split.tz_begin == '-') {\n+\t\txstrndup(split->mail_begin, split->mail_end - split->mail_begin);\n+\tlog->value.update.time = atol(split->date_begin);\n+\n+\ttz_begin = split->tz_begin;\n+\tif (*tz_begin == '-') {\n \t\tsign = -1;\n-\t\tsplit.tz_begin++;\n+\t\ttz_begin++;\n \t}\n-\tif (*split.tz_begin == '+') {\n+\tif (*tz_begin == '+') {\n \t\tsign = 1;\n-\t\tsplit.tz_begin++;\n+\t\ttz_begin++;\n \t}\n \n-\tlog->value.update.tz_offset = sign * atoi(split.tz_begin);\n+\tlog->value.update.tz_offset = sign * atoi(tz_begin);\n }\n \n static int read_ref_without_reload(struct reftable_stack *stack,\n@@ -1023,9 +1021,15 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\treftable_stack_merged_table(arg->stack);\n \tuint64_t ts = reftable_stack_next_update_index(arg->stack);\n \tstruct reftable_log_record *logs = NULL;\n+\tstruct ident_split committer_ident = {0};\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n+\tconst char *committer_info;\n \tint ret = 0;\n \n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n \tQSORT(arg->updates, arg->updates_nr, transaction_update_cmp);\n \n \treftable_writer_set_limits(writer, ts, ts);\n@@ -1091,7 +1095,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\t\tlog = &logs[logs_nr++];\n \t\t\tmemset(log, 0, sizeof(*log));\n \n-\t\t\tfill_reftable_log_record(log);\n+\t\t\tfill_reftable_log_record(log, &committer_ident);\n \t\t\tlog->update_index = ts;\n \t\t\tlog->refname = xstrdup(u->refname);\n \t\t\tmemcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ);\n@@ -1238,9 +1242,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \t\t.value.symref = (char *)create->target,\n \t\t.update_index = ts,\n \t};\n+\tstruct ident_split committer_ident = {0};\n \tstruct reftable_log_record log = {0};\n \tstruct object_id new_oid;\n \tstruct object_id old_oid;\n+\tconst char *committer_info;\n \tint ret;\n \n \treftable_writer_set_limits(writer, ts, ts);\n@@ -1268,7 +1274,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \t !should_write_log(&create->refs->base, create->refname))\n \t\treturn 0;\n \n-\tfill_reftable_log_record(&log);\n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n+\tfill_reftable_log_record(&log, &committer_ident);\n \tlog.refname = xstrdup(create->refname);\n \tlog.update_index = ts;\n \tlog.value.update.message = xstrndup(create->logmsg,\n@@ -1344,10 +1354,16 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \tstruct reftable_log_record old_log = {0}, *logs = NULL;\n \tstruct reftable_iterator it = {0};\n \tstruct string_list skip = STRING_LIST_INIT_NODUP;\n+\tstruct ident_split committer_ident = {0};\n \tstruct strbuf errbuf = STRBUF_INIT;\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n+\tconst char *committer_info;\n \tint ret;\n \n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n \tif (reftable_stack_read_ref(arg->stack, arg->oldname, &old_ref)) {\n \t\tret = error(_(\"refname %s not found\"), arg->oldname);\n \t\tgoto done;\n@@ -1422,7 +1438,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \n \t\tALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n \t\tmemset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));\n-\t\tfill_reftable_log_record(&logs[logs_nr]);\n+\t\tfill_reftable_log_record(&logs[logs_nr], &committer_ident);\n \t\tlogs[logs_nr].refname = (char *)arg->newname;\n \t\tlogs[logs_nr].update_index = deletion_ts;\n \t\tlogs[logs_nr].value.update.message =\n@@ -1454,7 +1470,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \t */\n \tALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n \tmemset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));\n-\tfill_reftable_log_record(&logs[logs_nr]);\n+\tfill_reftable_log_record(&logs[logs_nr], &committer_ident);\n \tlogs[logs_nr].refname = (char *)arg->newname;\n \tlogs[logs_nr].update_index = creation_ts;\n \tlogs[logs_nr].value.update.message =\n-- \n2.44.GIT\n\n"},{"id":"492088","messageId":"8e9d69e9e6685b097f4d184f9a7e2a2d753c95f1.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 5/9] reftable/writer: refactorings for `writer_add_record()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:30:08Z","receivedAt":"2024-04-02T17:30:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Large parts of the reftable library do not conform to Git's typical code\nstyle. Refactor `writer_add_record()` such that it conforms better to it\nand add some documentation that explains some of its more intricate\nbehaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 38 +++++++++++++++++++++++++++-----------\n 1 file changed, 27 insertions(+), 11 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 1d9ff0fbfa..0ad5eb8887 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -209,7 +209,8 @@ static int writer_add_record(struct reftable_writer *w,\n \t\t\t struct reftable_record *rec)\n {\n \tstruct strbuf key = STRBUF_INIT;\n-\tint err = -1;\n+\tint err;\n+\n \treftable_record_key(rec, &key);\n \tif (strbuf_cmp(&w->last_key, &key) >= 0) {\n \t\terr = REFTABLE_API_ERROR;\n@@ -218,27 +219,42 @@ static int writer_add_record(struct reftable_writer *w,\n \n \tstrbuf_reset(&w->last_key);\n \tstrbuf_addbuf(&w->last_key, &key);\n-\tif (!w->block_writer) {\n+\tif (!w->block_writer)\n \t\twriter_reinit_block_writer(w, reftable_record_type(rec));\n-\t}\n \n-\tassert(block_writer_type(w->block_writer) == reftable_record_type(rec));\n+\tif (block_writer_type(w->block_writer) != reftable_record_type(rec))\n+\t\tBUG(\"record of type %d added to writer of type %d\",\n+\t\t reftable_record_type(rec), block_writer_type(w->block_writer));\n \n-\tif (block_writer_add(w->block_writer, rec) == 0) {\n+\t/*\n+\t * Try to add the record to the writer. If this succeeds then we're\n+\t * done. Otherwise the block writer may have hit the block size limit\n+\t * and needs to be flushed.\n+\t */\n+\tif (!block_writer_add(w->block_writer, rec)) {\n \t\terr = 0;\n \t\tgoto done;\n \t}\n \n+\t/*\n+\t * The current block is full, so we need to flush and reinitialize the\n+\t * writer to start writing the next block.\n+\t */\n \terr = writer_flush_block(w);\n-\tif (err < 0) {\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n-\n \twriter_reinit_block_writer(w, reftable_record_type(rec));\n+\n+\t/*\n+\t * Try to add the record to the writer again. If this still fails then\n+\t * the record does not fit into the block size.\n+\t *\n+\t * TODO: it would be great to have `block_writer_add()` return proper\n+\t * error codes so that we don't have to second-guess the failure\n+\t * mode here.\n+\t */\n \terr = block_writer_add(w->block_writer, rec);\n-\tif (err == -1) {\n-\t\t/* we are writing into memory, so an error can only mean it\n-\t\t * doesn't fit. */\n+\tif (err) {\n \t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tgoto done;\n \t}\n-- \n2.44.GIT\n\n"},{"id":"492089","messageId":"1f903afdda229ae3e6b73c5612d77f4647079690.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 6/9] reftable/writer: refactorings for `writer_flush_nonempty_block()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:30:12Z","receivedAt":"2024-04-02T17:30:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Large parts of the reftable library do not conform to Git's typical code\nstyle. Refactor `writer_flush_nonempty_block()` such that it conforms\nbetter to it and add some documentation that explains some of its more\nintricate behaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 72 +++++++++++++++++++++++++++++------------------\n 1 file changed, 44 insertions(+), 28 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 0ad5eb8887..d347ec4cc6 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -659,58 +659,74 @@ static void writer_clear_index(struct reftable_writer *w)\n \tw->index_cap = 0;\n }\n \n-static const int debug = 0;\n-\n static int writer_flush_nonempty_block(struct reftable_writer *w)\n {\n+\tstruct reftable_index_record index_record = {\n+\t\t.last_key = STRBUF_INIT,\n+\t};\n \tuint8_t typ = block_writer_type(w->block_writer);\n-\tstruct reftable_block_stats *bstats =\n-\t\twriter_reftable_block_stats(w, typ);\n-\tuint64_t block_typ_off = (bstats->blocks == 0) ? w->next : 0;\n-\tint raw_bytes = block_writer_finish(w->block_writer);\n-\tint padding = 0;\n-\tint err = 0;\n-\tstruct reftable_index_record ir = { .last_key = STRBUF_INIT };\n+\tstruct reftable_block_stats *bstats;\n+\tint raw_bytes, padding = 0, err;\n+\tuint64_t block_typ_off;\n+\n+\t/*\n+\t * Finish the current block. This will cause the block writer to emit\n+\t * restart points and potentially compress records in case we are\n+\t * writing a log block.\n+\t *\n+\t * Note that this is still happening in memory.\n+\t */\n+\traw_bytes = block_writer_finish(w->block_writer);\n \tif (raw_bytes < 0)\n \t\treturn raw_bytes;\n \n-\tif (!w->opts.unpadded && typ != BLOCK_TYPE_LOG) {\n+\t/*\n+\t * By default, all records except for log records are padded to the\n+\t * block size.\n+\t */\n+\tif (!w->opts.unpadded && typ != BLOCK_TYPE_LOG)\n \t\tpadding = w->opts.block_size - raw_bytes;\n-\t}\n \n-\tif (block_typ_off > 0) {\n+\tbstats = writer_reftable_block_stats(w, typ);\n+\tblock_typ_off = (bstats->blocks == 0) ? w->next : 0;\n+\tif (block_typ_off > 0)\n \t\tbstats->offset = block_typ_off;\n-\t}\n-\n \tbstats->entries += w->block_writer->entries;\n \tbstats->restarts += w->block_writer->restart_len;\n \tbstats->blocks++;\n \tw->stats.blocks++;\n \n-\tif (debug) {\n-\t\tfprintf(stderr, \"block %c off %\" PRIu64 \" sz %d (%d)\\n\", typ,\n-\t\t\tw->next, raw_bytes,\n-\t\t\tget_be24(w->block + w->block_writer->header_off + 1));\n-\t}\n-\n-\tif (w->next == 0) {\n+\t/*\n+\t * If this is the first block we're writing to the table then we need\n+\t * to also write the reftable header.\n+\t */\n+\tif (!w->next)\n \t\twriter_write_header(w, w->block);\n-\t}\n \n \terr = padded_write(w, w->block, raw_bytes, padding);\n \tif (err < 0)\n \t\treturn err;\n \n+\t/*\n+\t * Add an index record for every block that we're writing. If we end up\n+\t * having more than a threshold of index records we will end up writing\n+\t * an index section in `writer_finish_section()`. Each index record\n+\t * contains the last record key of the block it is indexing as well as\n+\t * the offset of that block.\n+\t *\n+\t * Note that this also applies when flushing index blocks, in which\n+\t * case we will end up with a multi-level index.\n+\t */\n \tREFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);\n-\n-\tir.offset = w->next;\n-\tstrbuf_reset(&ir.last_key);\n-\tstrbuf_addbuf(&ir.last_key, &w->block_writer->last_key);\n-\tw->index[w->index_len] = ir;\n-\n+\tindex_record.offset = w->next;\n+\tstrbuf_reset(&index_record.last_key);\n+\tstrbuf_addbuf(&index_record.last_key, &w->block_writer->last_key);\n+\tw->index[w->index_len] = index_record;\n \tw->index_len++;\n+\n \tw->next += padding + raw_bytes;\n \tw->block_writer = NULL;\n+\n \treturn 0;\n }\n \n-- \n2.44.GIT\n\n"},{"id":"492090","messageId":"86dab54dfe4501dfa5e50e5a01513c890b62bb4d.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 7/9] reftable/block: reuse zstream when writing log blocks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:30:16Z","receivedAt":"2024-04-02T17:30:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While most reftable blocks are written to disk as-is, blocks for log\nrecords are compressed with zlib. To compress them we use `compress2()`,\nwhich is a simple wrapper around the more complex `zstream` interface\nthat would require multiple function invocations.\n\nOne downside of this interface is that `compress2()` will reallocate\ninternal state of the `zstream` interface on every single invocation.\nConsequently, as we call `compress2()` for every single log block which\nwe are about to write, this can lead to quite some memory allocation\nchurn.\n\nRefactor the code so that the block writer reuses a `zstream`. This\nsignificantly reduces the number of bytes allocated when writing many\nrefs in a single transaction, as demonstrated by the following benchmark\nthat writes 100k refs in a single transaction.\n\nBefore:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,631,887 allocs, 22,631,736 frees, 1,854,670,793 bytes allocated\n\nAfter:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,620,528 allocs, 22,620,377 frees, 1,245,549,984 bytes allocated\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 83 +++++++++++++++++++++++++++++++----------------\n reftable/block.h | 1 +\n reftable/writer.c | 4 +++\n 3 files changed, 60 insertions(+), 28 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex e2a2cee58d..1fa74d418f 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -76,6 +76,10 @@ void block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *buf,\n \tbw->entries = 0;\n \tbw->restart_len = 0;\n \tbw->last_key.len = 0;\n+\tif (!bw->zstream) {\n+\t\tREFTABLE_CALLOC_ARRAY(bw->zstream, 1);\n+\t\tdeflateInit(bw->zstream, 9);\n+\t}\n }\n \n uint8_t block_writer_type(struct block_writer *bw)\n@@ -139,39 +143,60 @@ int block_writer_finish(struct block_writer *w)\n \tw->next += 2;\n \tput_be24(w->buf + 1 + w->header_off, w->next);\n \n+\t/*\n+\t * Log records are stored zlib-compressed. Note that the compression\n+\t * also spans over the restart points we have just written.\n+\t */\n \tif (block_writer_type(w) == BLOCK_TYPE_LOG) {\n \t\tint block_header_skip = 4 + w->header_off;\n-\t\tuLongf src_len = w->next - block_header_skip;\n-\t\tuLongf dest_cap = src_len * 1.001 + 12;\n-\t\tuint8_t *compressed;\n-\n-\t\tREFTABLE_ALLOC_ARRAY(compressed, dest_cap);\n-\n-\t\twhile (1) {\n-\t\t\tuLongf out_dest_len = dest_cap;\n-\t\t\tint zresult = compress2(compressed, &out_dest_len,\n-\t\t\t\t\t\tw->buf + block_header_skip,\n-\t\t\t\t\t\tsrc_len, 9);\n-\t\t\tif (zresult == Z_BUF_ERROR && dest_cap < LONG_MAX) {\n-\t\t\t\tdest_cap *= 2;\n-\t\t\t\tcompressed =\n-\t\t\t\t\treftable_realloc(compressed, dest_cap);\n-\t\t\t\tif (compressed)\n-\t\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\tif (Z_OK != zresult) {\n-\t\t\t\treftable_free(compressed);\n-\t\t\t\treturn REFTABLE_ZLIB_ERROR;\n-\t\t\t}\n-\n-\t\t\tmemcpy(w->buf + block_header_skip, compressed,\n-\t\t\t out_dest_len);\n-\t\t\tw->next = out_dest_len + block_header_skip;\n+\t\tuLongf src_len = w->next - block_header_skip, compressed_len;\n+\t\tunsigned char *compressed;\n+\t\tint ret;\n+\n+\t\tret = deflateReset(w->zstream);\n+\t\tif (ret != Z_OK)\n+\t\t\treturn REFTABLE_ZLIB_ERROR;\n+\n+\t\t/*\n+\t\t * Precompute the upper bound of how many bytes the compressed\n+\t\t * data may end up with. Combined with `Z_FINISH`, `deflate()`\n+\t\t * is guaranteed to return `Z_STREAM_END`.\n+\t\t */\n+\t\tcompressed_len = deflateBound(w->zstream, src_len);\n+\t\tREFTABLE_ALLOC_ARRAY(compressed, compressed_len);\n+\n+\t\tw->zstream->next_out = compressed;\n+\t\tw->zstream->avail_out = compressed_len;\n+\t\tw->zstream->next_in = w->buf + block_header_skip;\n+\t\tw->zstream->avail_in = src_len;\n+\n+\t\t/*\n+\t\t * We want to perform all decompression in a single\n+\t\t * step, which is why we can pass Z_FINISH here. Note\n+\t\t * that both `Z_OK` and `Z_BUF_ERROR` indicate that we\n+\t\t * need to retry according to documentation.\n+\t\t *\n+\t\t * If the call fails we retry with a bigger output\n+\t\t * buffer.\n+\t\t */\n+\t\tret = deflate(w->zstream, Z_FINISH);\n+\t\tif (ret != Z_STREAM_END) {\n \t\t\treftable_free(compressed);\n-\t\t\tbreak;\n+\t\t\treturn REFTABLE_ZLIB_ERROR;\n \t\t}\n+\n+\t\t/*\n+\t\t * Overwrite the uncompressed data we have already written and\n+\t\t * adjust the `next` pointer to point right after the\n+\t\t * compressed data.\n+\t\t */\n+\t\tmemcpy(w->buf + block_header_skip, compressed,\n+\t\t w->zstream->total_out);\n+\t\tw->next = w->zstream->total_out + block_header_skip;\n+\n+\t\treftable_free(compressed);\n \t}\n+\n \treturn w->next;\n }\n \n@@ -425,6 +450,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \n void block_writer_release(struct block_writer *bw)\n {\n+\tdeflateEnd(bw->zstream);\n+\tFREE_AND_NULL(bw->zstream);\n \tFREE_AND_NULL(bw->restarts);\n \tstrbuf_release(&bw->last_key);\n \t/* the block is not owned. */\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 47acc62c0a..1375957fc8 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -18,6 +18,7 @@ license that can be found in the LICENSE file or at\n * allocation overhead.\n */\n struct block_writer {\n+\tz_stream *zstream;\n \tuint8_t *buf;\n \tuint32_t block_size;\n \ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex d347ec4cc6..51e663bb19 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -153,6 +153,10 @@ void reftable_writer_free(struct reftable_writer *w)\n {\n \tif (!w)\n \t\treturn;\n+\tif (w->block_writer) {\n+\t\tblock_writer_release(w->block_writer);\n+\t\tw->block_writer = NULL;\n+\t}\n \treftable_free(w->block);\n \treftable_free(w);\n }\n-- \n2.44.GIT\n\n"},{"id":"492091","messageId":"9899b58dcff6a8380eb7cf3622c7dfff51a10a2c.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 8/9] reftable/block: reuse compressed array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:30:20Z","receivedAt":"2024-04-02T17:30:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Similar to the preceding commit, let's reuse the `compressed` array that\nwe use to store compressed data in. This results in a small reduction in\nmemory allocations when writing many refs.\n\nBefore:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,620,528 allocs, 22,620,377 frees, 1,245,549,984 bytes allocated\n\nAfter:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,618,257 allocs, 22,618,106 frees, 1,236,351,528 bytes allocated\n\nSo while the reduction in allocations isn't really all that big, it's a\nlow hanging fruit and thus there isn't much of a reason not to pick it.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 14 +++++---------\n reftable/block.h | 3 +++\n 2 files changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 1fa74d418f..870d88b58d 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -150,7 +150,6 @@ int block_writer_finish(struct block_writer *w)\n \tif (block_writer_type(w) == BLOCK_TYPE_LOG) {\n \t\tint block_header_skip = 4 + w->header_off;\n \t\tuLongf src_len = w->next - block_header_skip, compressed_len;\n-\t\tunsigned char *compressed;\n \t\tint ret;\n \n \t\tret = deflateReset(w->zstream);\n@@ -163,9 +162,9 @@ int block_writer_finish(struct block_writer *w)\n \t\t * is guaranteed to return `Z_STREAM_END`.\n \t\t */\n \t\tcompressed_len = deflateBound(w->zstream, src_len);\n-\t\tREFTABLE_ALLOC_ARRAY(compressed, compressed_len);\n+\t\tREFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);\n \n-\t\tw->zstream->next_out = compressed;\n+\t\tw->zstream->next_out = w->compressed;\n \t\tw->zstream->avail_out = compressed_len;\n \t\tw->zstream->next_in = w->buf + block_header_skip;\n \t\tw->zstream->avail_in = src_len;\n@@ -180,21 +179,17 @@ int block_writer_finish(struct block_writer *w)\n \t\t * buffer.\n \t\t */\n \t\tret = deflate(w->zstream, Z_FINISH);\n-\t\tif (ret != Z_STREAM_END) {\n-\t\t\treftable_free(compressed);\n+\t\tif (ret != Z_STREAM_END)\n \t\t\treturn REFTABLE_ZLIB_ERROR;\n-\t\t}\n \n \t\t/*\n \t\t * Overwrite the uncompressed data we have already written and\n \t\t * adjust the `next` pointer to point right after the\n \t\t * compressed data.\n \t\t */\n-\t\tmemcpy(w->buf + block_header_skip, compressed,\n+\t\tmemcpy(w->buf + block_header_skip, w->compressed,\n \t\t w->zstream->total_out);\n \t\tw->next = w->zstream->total_out + block_header_skip;\n-\n-\t\treftable_free(compressed);\n \t}\n \n \treturn w->next;\n@@ -453,6 +448,7 @@ void block_writer_release(struct block_writer *bw)\n \tdeflateEnd(bw->zstream);\n \tFREE_AND_NULL(bw->zstream);\n \tFREE_AND_NULL(bw->restarts);\n+\tFREE_AND_NULL(bw->compressed);\n \tstrbuf_release(&bw->last_key);\n \t/* the block is not owned. */\n }\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 1375957fc8..657498014c 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -19,6 +19,9 @@ license that can be found in the LICENSE file or at\n */\n struct block_writer {\n \tz_stream *zstream;\n+\tunsigned char *compressed;\n+\tsize_t compressed_cap;\n+\n \tuint8_t *buf;\n \tuint32_t block_size;\n \n-- \n2.44.GIT\n\n"},{"id":"492092","messageId":"6950ae4ea758a92ba87f73d5ce66c5dfb896234c.1712078736.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH 9/9] reftable/writer: reset `last_key` instead of releasing it","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-02T17:30:24Z","receivedAt":"2024-04-02T17:30:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable writer tracks the last key that it has written so that it\ncan properly compute the compressed prefix for the next record it is\nabout to write. This last key must be reset whenever we move on to write\nthe next block, which is done in `writer_reinit_block_writer()`. We do\nthis by calling `strbuf_release()` though, which needlessly deallocates\nthe underlying buffer.\n\nConvert the code to use `strbuf_reset()` instead, which saves one\nallocation per block we're about to write. This requires us to also\namend `reftable_writer_free()` to release the buffer's memory now as we\npreviously seemingly relied on `writer_reinit_block_writer()` to release\nthe memory for us. Releasing memory here is the right thing to do\nanyway.\n\nWhile at it, convert a callsite where we truncate the buffer by setting\nits length to zero to instead use `strbuf_reset()`, too.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 51e663bb19..4b3a4e3f3c 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -109,7 +109,7 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)\n \t\tblock_start = header_size(writer_version(w));\n \t}\n \n-\tstrbuf_release(&w->last_key);\n+\tstrbuf_reset(&w->last_key);\n \tblock_writer_init(&w->block_writer_data, typ, w->block,\n \t\t\t w->opts.block_size, block_start,\n \t\t\t hash_size(w->opts.hash_id));\n@@ -157,6 +157,7 @@ void reftable_writer_free(struct reftable_writer *w)\n \t\tblock_writer_release(w->block_writer);\n \t\tw->block_writer = NULL;\n \t}\n+\tstrbuf_release(&w->last_key);\n \treftable_free(w->block);\n \treftable_free(w);\n }\n@@ -472,7 +473,7 @@ static int writer_finish_section(struct reftable_writer *w)\n \tbstats->max_index_level = max_level;\n \n \t/* Reinit lastKey, as the next section can start with any key. */\n-\tw->last_key.len = 0;\n+\tstrbuf_reset(&w->last_key);\n \n \treturn 0;\n }\n-- \n2.44.GIT\n\n"},{"id":"492180","messageId":"xmqqr0fm713f.fsf@gitster.g","threadId":"61255","inReplyTo":"14b4dacd731a7d9c19029cd8a0c3b6170c31ae25.1712078736.git.ps@pks.im","subject":"Re: [PATCH 1/9] refs/reftable: fix D/F conflict error message on ref copy","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-03T18:28:04Z","receivedAt":"2024-04-03T18:28:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The `write_copy_table()` function is shared between the reftable\n> implementations for renaming and copying refs. The only difference\n> between those two cases is that the rename will also delete the old\n> reference, whereas copying won't.\n>\n> This has resulted in a bug though where we don't properly verify refname\n> availability. When calling `refs_verify_refname_available()`, we always\n> add the old ref name to the list of refs to be skipped when computing\n> availability, which indicates that the name would be available even if\n> it already exists at the current point in time. This is only the right\n> thing to do for renames though, not for copies.\n>\n> The consequence of this bug is quite harmless because the reftable\n> backend has its own checks for D/F conflicts further down in the call\n> stack, and thus we refuse the update regardless of the bug. But all the\n> user gets in this case is an uninformative message that copying the ref\n> has failed, without any further details.\n>\n> Fix the bug and only add the old name to the skip-list in case we rename\n> the ref. Consequently, this error case will now be handled by\n> `refs_verify_refname_available()`, which knows to provide a proper error\n> message.\n\nOK. Nicely described. Instead of letting the reftable code\ndownstream to notice an update that was left uncaught by an extra\nelement in &skip, we will let refs_verify_refname_available() to\ncatch it at the right place. Makes sense.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> refs/reftable-backend.c | 3 ++-\n> t/t0610-reftable-basics.sh | 33 +++++++++++++++++++++++++++++++++\n> 2 files changed, 35 insertions(+), 1 deletion(-)\n>\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index e206d5a073..0358da14db 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1351,7 +1351,8 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n> \t/*\n> \t * Verify that the new refname is available.\n> \t */\n> -\tstring_list_insert(&skip, arg->oldname);\n> +\tif (arg->delete_old)\n> +\t\tstring_list_insert(&skip, arg->oldname);\n> \tret = refs_verify_refname_available(&arg->refs->base, arg->newname,\n> \t\t\t\t\t NULL, &skip, &errbuf);\n> \tif (ret < 0) {\n> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\n> index 686781192e..055231a707 100755\n> --- a/t/t0610-reftable-basics.sh\n> +++ b/t/t0610-reftable-basics.sh\n> @@ -730,6 +730,39 @@ test_expect_success 'reflog: updates via HEAD update HEAD reflog' '\n> \t)\n> '\n> \n> +test_expect_success 'branch: copying branch with D/F conflict' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit A &&\n> +\t\tgit branch branch &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\terror: ${SQ}refs/heads/branch${SQ} exists; cannot create ${SQ}refs/heads/branch/moved${SQ}\n> +\t\tfatal: branch copy failed\n> +\t\tEOF\n> +\t\ttest_must_fail git branch -c branch branch/moved 2>err &&\n> +\t\ttest_cmp expect err\n> +\t)\n> +'\n> +\n> +test_expect_success 'branch: moving branch with D/F conflict' '\n> +\ttest_when_finished \"rm -rf repo\" &&\n> +\tgit init repo &&\n> +\t(\n> +\t\tcd repo &&\n> +\t\ttest_commit A &&\n> +\t\tgit branch branch &&\n> +\t\tgit branch conflict &&\n> +\t\tcat >expect <<-EOF &&\n> +\t\terror: ${SQ}refs/heads/conflict${SQ} exists; cannot create ${SQ}refs/heads/conflict/moved${SQ}\n> +\t\tfatal: branch rename failed\n> +\t\tEOF\n> +\t\ttest_must_fail git branch -m branch conflict/moved 2>err &&\n> +\t\ttest_cmp expect err\n> +\t)\n> +'\n> +\n> test_expect_success 'worktree: adding worktree creates separate stack' '\n> \ttest_when_finished \"rm -rf repo worktree\" &&\n> \tgit init repo &&\n"},{"id":"492182","messageId":"xmqqedbm6zoj.fsf@gitster.g","threadId":"61255","inReplyTo":"a9a6795c025b23035bfdd3e23b0113df9f6c5e4b.1712078736.git.ps@pks.im","subject":"Re: [PATCH 4/9] refs/reftable: don't recompute committer ident","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-03T18:58:36Z","receivedAt":"2024-04-03T18:58:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> In order to write reflog entries we need to compute the committer's\n> identity as it becomes encoded in the log record itself. In the reftable\n> backend, computing the identity is repeated for every single reflog\n> entry which we are about to write in a transaction. Needless to say,\n> this can be quite a waste of effort when writing many refs with reflog\n> entries in a single transaction.\n\nIt would have been nice to mention which caller benefits from this\nrewrite in the above.\n\nThere are four callers of the fill_reftable_log_record() function.\nThe patch moves the split_ident() call from the callee to these four\ncallers. The write_transaction_table() function calls it in a loop,\nwhich should give us a big boost. For other three callers, they\ncall it at most twice (i.e. write_copy_table() when deleting the old\none), so their contribution to the boost should be minimal.\n\nMakes sense.\n\n"},{"id":"492185","messageId":"xmqqplv65jet.fsf@gitster.g","threadId":"61255","inReplyTo":"86dab54dfe4501dfa5e50e5a01513c890b62bb4d.1712078736.git.ps@pks.im","subject":"Re: [PATCH 7/9] reftable/block: reuse zstream when writing log blocks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-03T19:35:22Z","receivedAt":"2024-04-03T19:35:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> @@ -139,39 +143,60 @@ int block_writer_finish(struct block_writer *w)\n> \tw->next += 2;\n> \tput_be24(w->buf + 1 + w->header_off, w->next);\n> \n> +\t/*\n> +\t * Log records are stored zlib-compressed. Note that the compression\n> +\t * also spans over the restart points we have just written.\n> +\t */\n> \tif (block_writer_type(w) == BLOCK_TYPE_LOG) {\n> \t\tint block_header_skip = 4 + w->header_off;\n> +\t\tuLongf src_len = w->next - block_header_skip, compressed_len;\n> +\t\tunsigned char *compressed;\n> +\t\tint ret;\n> +\n> +\t\tret = deflateReset(w->zstream);\n> +\t\tif (ret != Z_OK)\n> +\t\t\treturn REFTABLE_ZLIB_ERROR;\n> +\n> +\t\t/*\n> +\t\t * Precompute the upper bound of how many bytes the compressed\n> +\t\t * data may end up with. Combined with `Z_FINISH`, `deflate()`\n> +\t\t * is guaranteed to return `Z_STREAM_END`.\n> +\t\t */\n> +\t\tcompressed_len = deflateBound(w->zstream, src_len);\n> +\t\tREFTABLE_ALLOC_ARRAY(compressed, compressed_len);\n\nOK.\n\n> +\t\tw->zstream->next_out = compressed;\n> +\t\tw->zstream->avail_out = compressed_len;\n> +\t\tw->zstream->next_in = w->buf + block_header_skip;\n> +\t\tw->zstream->avail_in = src_len;\n> +\n> +\t\t/*\n> +\t\t * We want to perform all decompression in a single\n> +\t\t * step, which is why we can pass Z_FINISH here. Note\n> +\t\t * that both `Z_OK` and `Z_BUF_ERROR` indicate that we\n> +\t\t * need to retry according to documentation.\n> +\t\t *\n> +\t\t * If the call fails we retry with a bigger output\n> +\t\t * buffer.\n> +\t\t */\n\nI am not sure where the retry is happening, though.\n\nblock_writer_finish() is called by writer_flush_nonempty_block()\nwhich returns a negative return to its caller, which is\nwriter_flush_block(). writer_flush_block() in turn returns a\nnegative return to its callers from writer_add_record(),\nwrite_finish_section(), and write_object_record(). Nobody seems to\nreact to REFTABLE_ZLIB_ERROR (other than the reftable/error.c that\nstringifies the error for messages).\n\nBut we have asked deflateBound() so if we did not get Z_STREAM_END,\nwouldn't it mean some data corruption that retrying would not help?\n\n> +\t\tret = deflate(w->zstream, Z_FINISH);\n> +\t\tif (ret != Z_STREAM_END) {\n> \t\t\treftable_free(compressed);\n> -\t\t\tbreak;\n> +\t\t\treturn REFTABLE_ZLIB_ERROR;\n> \t\t}\n> +\n> +\t\t/*\n> +\t\t * Overwrite the uncompressed data we have already written and\n> +\t\t * adjust the `next` pointer to point right after the\n> +\t\t * compressed data.\n> +\t\t */\n> +\t\tmemcpy(w->buf + block_header_skip, compressed,\n> +\t\t w->zstream->total_out);\n> +\t\tw->next = w->zstream->total_out + block_header_skip;\n> +\n> +\t\treftable_free(compressed);\n> \t}\n> +\n> \treturn w->next;\n> }\n\nOK.\n\n> @@ -425,6 +450,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n> \n> void block_writer_release(struct block_writer *bw)\n> {\n> +\tdeflateEnd(bw->zstream);\n> +\tFREE_AND_NULL(bw->zstream);\n> \tFREE_AND_NULL(bw->restarts);\n> \tstrbuf_release(&bw->last_key);\n> \t/* the block is not owned. */\n> diff --git a/reftable/block.h b/reftable/block.h\n> index 47acc62c0a..1375957fc8 100644\n> --- a/reftable/block.h\n> +++ b/reftable/block.h\n> @@ -18,6 +18,7 @@ license that can be found in the LICENSE file or at\n> * allocation overhead.\n> */\n> struct block_writer {\n> +\tz_stream *zstream;\n> \tuint8_t *buf;\n> \tuint32_t block_size;\n> \n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index d347ec4cc6..51e663bb19 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -153,6 +153,10 @@ void reftable_writer_free(struct reftable_writer *w)\n> {\n> \tif (!w)\n> \t\treturn;\n> +\tif (w->block_writer) {\n> +\t\tblock_writer_release(w->block_writer);\n> +\t\tw->block_writer = NULL;\n> +\t}\n\nThis smells like an orthogonal fix to an unrelated resource leakage?\n\n> \treftable_free(w->block);\n> \treftable_free(w);\n> }\n\nThanks.\n"},{"id":"492200","messageId":"Zg48Z-jcJ8YiF04b@tanuki","threadId":"61255","inReplyTo":"xmqqedbm6zoj.fsf@gitster.g","subject":"Re: [PATCH 4/9] refs/reftable: don't recompute committer ident","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:36:39Z","receivedAt":"2024-04-04T05:36:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 03, 2024 at 11:58:36AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > In order to write reflog entries we need to compute the committer's\n> > identity as it becomes encoded in the log record itself. In the reftable\n> > backend, computing the identity is repeated for every single reflog\n> > entry which we are about to write in a transaction. Needless to say,\n> > this can be quite a waste of effort when writing many refs with reflog\n> > entries in a single transaction.\n> \n> It would have been nice to mention which caller benefits from this\n> rewrite in the above.\n> \n> There are four callers of the fill_reftable_log_record() function.\n> The patch moves the split_ident() call from the callee to these four\n> callers. The write_transaction_table() function calls it in a loop,\n> which should give us a big boost. For other three callers, they\n> call it at most twice (i.e. write_copy_table() when deleting the old\n> one), so their contribution to the boost should be minimal.\n> \n> Makes sense.\n\nI do say it by saying \"write in a transaction\", but I agree that this is\nnot exactly obvious. Will rephrase.\n\nPatrick\n"},{"id":"492201","messageId":"Zg48ersnCJqsk-Ey@tanuki","threadId":"61255","inReplyTo":"xmqqplv65jet.fsf@gitster.g","subject":"Re: [PATCH 7/9] reftable/block: reuse zstream when writing log blocks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:36:58Z","receivedAt":"2024-04-04T05:37:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Apr 03, 2024 at 12:35:22PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > @@ -139,39 +143,60 @@ int block_writer_finish(struct block_writer *w)\n> > \tw->next += 2;\n> > \tput_be24(w->buf + 1 + w->header_off, w->next);\n> > \n> > +\t/*\n> > +\t * Log records are stored zlib-compressed. Note that the compression\n> > +\t * also spans over the restart points we have just written.\n> > +\t */\n> > \tif (block_writer_type(w) == BLOCK_TYPE_LOG) {\n> > \t\tint block_header_skip = 4 + w->header_off;\n> > +\t\tuLongf src_len = w->next - block_header_skip, compressed_len;\n> > +\t\tunsigned char *compressed;\n> > +\t\tint ret;\n> > +\n> > +\t\tret = deflateReset(w->zstream);\n> > +\t\tif (ret != Z_OK)\n> > +\t\t\treturn REFTABLE_ZLIB_ERROR;\n> > +\n> > +\t\t/*\n> > +\t\t * Precompute the upper bound of how many bytes the compressed\n> > +\t\t * data may end up with. Combined with `Z_FINISH`, `deflate()`\n> > +\t\t * is guaranteed to return `Z_STREAM_END`.\n> > +\t\t */\n> > +\t\tcompressed_len = deflateBound(w->zstream, src_len);\n> > +\t\tREFTABLE_ALLOC_ARRAY(compressed, compressed_len);\n> \n> OK.\n> \n> > +\t\tw->zstream->next_out = compressed;\n> > +\t\tw->zstream->avail_out = compressed_len;\n> > +\t\tw->zstream->next_in = w->buf + block_header_skip;\n> > +\t\tw->zstream->avail_in = src_len;\n> > +\n> > +\t\t/*\n> > +\t\t * We want to perform all decompression in a single\n> > +\t\t * step, which is why we can pass Z_FINISH here. Note\n> > +\t\t * that both `Z_OK` and `Z_BUF_ERROR` indicate that we\n> > +\t\t * need to retry according to documentation.\n> > +\t\t *\n> > +\t\t * If the call fails we retry with a bigger output\n> > +\t\t * buffer.\n> > +\t\t */\n> \n> I am not sure where the retry is happening, though.\n> \n> block_writer_finish() is called by writer_flush_nonempty_block()\n> which returns a negative return to its caller, which is\n> writer_flush_block(). writer_flush_block() in turn returns a\n> negative return to its callers from writer_add_record(),\n> write_finish_section(), and write_object_record(). Nobody seems to\n> react to REFTABLE_ZLIB_ERROR (other than the reftable/error.c that\n> stringifies the error for messages).\n> \n> But we have asked deflateBound() so if we did not get Z_STREAM_END,\n> wouldn't it mean some data corruption that retrying would not help?\n\nYeha, this comment is stale from a previous iteration.\n\n> > +\t\tret = deflate(w->zstream, Z_FINISH);\n> > +\t\tif (ret != Z_STREAM_END) {\n> > \t\t\treftable_free(compressed);\n> > -\t\t\tbreak;\n> > +\t\t\treturn REFTABLE_ZLIB_ERROR;\n> > \t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * Overwrite the uncompressed data we have already written and\n> > +\t\t * adjust the `next` pointer to point right after the\n> > +\t\t * compressed data.\n> > +\t\t */\n> > +\t\tmemcpy(w->buf + block_header_skip, compressed,\n> > +\t\t w->zstream->total_out);\n> > +\t\tw->next = w->zstream->total_out + block_header_skip;\n> > +\n> > +\t\treftable_free(compressed);\n> > \t}\n> > +\n> > \treturn w->next;\n> > }\n> \n> OK.\n> \n> > @@ -425,6 +450,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n> > \n> > void block_writer_release(struct block_writer *bw)\n> > {\n> > +\tdeflateEnd(bw->zstream);\n> > +\tFREE_AND_NULL(bw->zstream);\n> > \tFREE_AND_NULL(bw->restarts);\n> > \tstrbuf_release(&bw->last_key);\n> > \t/* the block is not owned. */\n> > diff --git a/reftable/block.h b/reftable/block.h\n> > index 47acc62c0a..1375957fc8 100644\n> > --- a/reftable/block.h\n> > +++ b/reftable/block.h\n> > @@ -18,6 +18,7 @@ license that can be found in the LICENSE file or at\n> > * allocation overhead.\n> > */\n> > struct block_writer {\n> > +\tz_stream *zstream;\n> > \tuint8_t *buf;\n> > \tuint32_t block_size;\n> > \n> > diff --git a/reftable/writer.c b/reftable/writer.c\n> > index d347ec4cc6..51e663bb19 100644\n> > --- a/reftable/writer.c\n> > +++ b/reftable/writer.c\n> > @@ -153,6 +153,10 @@ void reftable_writer_free(struct reftable_writer *w)\n> > {\n> > \tif (!w)\n> > \t\treturn;\n> > +\tif (w->block_writer) {\n> > +\t\tblock_writer_release(w->block_writer);\n> > +\t\tw->block_writer = NULL;\n> > +\t}\n> \n> This smells like an orthogonal fix to an unrelated resource leakage?\n\nTrue. The memory leak simply never occurred before this change, but in\ntheory it could have happened. Will move into a separate commit.\n\nPatrick\n\n> > \treftable_free(w->block);\n> > \treftable_free(w);\n> > }\n> \n> Thanks.\n"},{"id":"492202","messageId":"cover.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH v2 00/11] reftable: optimize write performance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:05Z","receivedAt":"2024-04-04T05:48:09Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that aims to optimize\nwrite performance in the reftable backend. I've made the following\nchanges compared to v2:\n\n - Added a new patch to drop \"reftable/refname.{c,h}\" and related\n completely.\n\n - Reworded the commit message for the committer ident refactorings.\n\n - Added a new patch that unifies the code paths that release reftable\n writer resources.\n\n - Fixed a stale comment claiming that we retry deflating, which we\n don't.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (11):\n refs/reftable: fix D/F conflict error message on ref copy\n refs/reftable: perform explicit D/F check when writing symrefs\n refs/reftable: skip duplicate name checks\n reftable: remove name checks\n refs/reftable: don't recompute committer ident\n reftable/writer: refactorings for `writer_add_record()`\n reftable/writer: refactorings for `writer_flush_nonempty_block()`\n reftable/writer: unify releasing memory\n reftable/writer: reset `last_key` instead of releasing it\n reftable/block: reuse zstream when writing log blocks\n reftable/block: reuse compressed array\n\n Makefile | 2 -\n refs/reftable-backend.c | 75 +++++++++----\n reftable/block.c | 80 ++++++++------\n reftable/block.h | 4 +\n reftable/error.c | 2 -\n reftable/refname.c | 209 -------------------------------------\n reftable/refname.h | 29 -----\n reftable/refname_test.c | 101 ------------------\n reftable/reftable-error.h | 3 -\n reftable/reftable-tests.h | 1 -\n reftable/reftable-writer.h | 4 -\n reftable/stack.c | 67 +-----------\n reftable/stack_test.c | 39 -------\n reftable/writer.c | 137 +++++++++++++++---------\n t/helper/test-reftable.c | 1 -\n t/t0610-reftable-basics.sh | 35 ++++++-\n 16 files changed, 230 insertions(+), 559 deletions(-)\n delete mode 100644 reftable/refname.c\n delete mode 100644 reftable/refname.h\n delete mode 100644 reftable/refname_test.c\n\nRange-diff against v1:\n 1: 14b4dacd73 = 1: 926e802395 refs/reftable: fix D/F conflict error message on ref copy\n 2: 55db366e61 = 2: 6190171906 refs/reftable: perform explicit D/F check when writing symrefs\n 3: ad8210ec65 = 3: 80008cc5e7 refs/reftable: skip duplicate name checks\n -: ---------- > 4: 3497a570b4 reftable: remove name checks\n 4: a9a6795c02 ! 5: f892a3007b refs/reftable: don't recompute committer ident\n @@ Commit message\n refs/reftable: don't recompute committer ident\n \n In order to write reflog entries we need to compute the committer's\n - identity as it becomes encoded in the log record itself. In the reftable\n - backend, computing the identity is repeated for every single reflog\n - entry which we are about to write in a transaction. Needless to say,\n - this can be quite a waste of effort when writing many refs with reflog\n - entries in a single transaction.\n + identity as it gets encoded in the log record itself. The reftable\n + backend does this via `git_committer_info()` and `split_ident_line()` in\n + `fill_reftable_log_record()`, which use the Git config as well as\n + environment variables to figure out the identity.\n +\n + While most callers would only call `fill_reftable_log_record()` once or\n + twice, `write_transaction_table()` will call it as many times as there\n + are queued ref updates. This can be quite a waste of effort when writing\n + many refs with reflog entries in a single transaction.\n \n Refactor the code to pre-compute the committer information. This results\n in a small speedup when writing 100000 refs in a single transaction:\n 5: 8e9d69e9e6 = 6: 4877ab3921 reftable/writer: refactorings for `writer_add_record()`\n 6: 1f903afdda = 7: 8f1c5b4169 reftable/writer: refactorings for `writer_flush_nonempty_block()`\n -: ---------- > 8: 41db7414e1 reftable/writer: unify releasing memory\n 9: 6950ae4ea7 ! 9: e5c7dbe417 reftable/writer: reset `last_key` instead of releasing it\n @@ reftable/writer.c: static void writer_reinit_block_writer(struct reftable_writer\n \tblock_writer_init(&w->block_writer_data, typ, w->block,\n \t\t\t w->opts.block_size, block_start,\n \t\t\t hash_size(w->opts.hash_id));\n -@@ reftable/writer.c: void reftable_writer_free(struct reftable_writer *w)\n - \t\tblock_writer_release(w->block_writer);\n - \t\tw->block_writer = NULL;\n - \t}\n -+\tstrbuf_release(&w->last_key);\n - \treftable_free(w->block);\n - \treftable_free(w);\n - }\n @@ reftable/writer.c: static int writer_finish_section(struct reftable_writer *w)\n \tbstats->max_index_level = max_level;\n \n 7: 86dab54dfe ! 10: 26f422703f reftable/block: reuse zstream when writing log blocks\n @@ reftable/block.c: int block_writer_finish(struct block_writer *w)\n +\t\tw->zstream->avail_in = src_len;\n +\n +\t\t/*\n -+\t\t * We want to perform all decompression in a single\n -+\t\t * step, which is why we can pass Z_FINISH here. Note\n -+\t\t * that both `Z_OK` and `Z_BUF_ERROR` indicate that we\n -+\t\t * need to retry according to documentation.\n -+\t\t *\n -+\t\t * If the call fails we retry with a bigger output\n -+\t\t * buffer.\n ++\t\t * We want to perform all decompression in a single step, which\n ++\t\t * is why we can pass Z_FINISH here. As we have precomputed the\n ++\t\t * deflated buffer's size via `deflateBound()` this function is\n ++\t\t * guaranteed to succeed according to the zlib documentation.\n +\t\t */\n +\t\tret = deflate(w->zstream, Z_FINISH);\n +\t\tif (ret != Z_STREAM_END) {\n @@ reftable/block.h: license that can be found in the LICENSE file or at\n \tuint8_t *buf;\n \tuint32_t block_size;\n \n -\n - ## reftable/writer.c ##\n -@@ reftable/writer.c: void reftable_writer_free(struct reftable_writer *w)\n - {\n - \tif (!w)\n - \t\treturn;\n -+\tif (w->block_writer) {\n -+\t\tblock_writer_release(w->block_writer);\n -+\t\tw->block_writer = NULL;\n -+\t}\n - \treftable_free(w->block);\n - \treftable_free(w);\n - }\n 8: 9899b58dcf ! 11: 4f9df714da reftable/block: reuse compressed array\n @@ reftable/block.c: int block_writer_finish(struct block_writer *w)\n \t\tw->zstream->next_in = w->buf + block_header_skip;\n \t\tw->zstream->avail_in = src_len;\n @@ reftable/block.c: int block_writer_finish(struct block_writer *w)\n - \t\t * buffer.\n + \t\t * guaranteed to succeed according to the zlib documentation.\n \t\t */\n \t\tret = deflate(w->zstream, Z_FINISH);\n -\t\tif (ret != Z_STREAM_END) {\n-- \n2.44.GIT\n\n"},{"id":"492203","messageId":"926e80239580b601cff752fa5f2086b2ff9298f6.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 01/11] refs/reftable: fix D/F conflict error message on ref copy","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:09Z","receivedAt":"2024-04-04T05:48:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `write_copy_table()` function is shared between the reftable\nimplementations for renaming and copying refs. The only difference\nbetween those two cases is that the rename will also delete the old\nreference, whereas copying won't.\n\nThis has resulted in a bug though where we don't properly verify refname\navailability. When calling `refs_verify_refname_available()`, we always\nadd the old ref name to the list of refs to be skipped when computing\navailability, which indicates that the name would be available even if\nit already exists at the current point in time. This is only the right\nthing to do for renames though, not for copies.\n\nThe consequence of this bug is quite harmless because the reftable\nbackend has its own checks for D/F conflicts further down in the call\nstack, and thus we refuse the update regardless of the bug. But all the\nuser gets in this case is an uninformative message that copying the ref\nhas failed, without any further details.\n\nFix the bug and only add the old name to the skip-list in case we rename\nthe ref. Consequently, this error case will now be handled by\n`refs_verify_refname_available()`, which knows to provide a proper error\nmessage.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 3 ++-\n t/t0610-reftable-basics.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex e206d5a073..0358da14db 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1351,7 +1351,8 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \t/*\n \t * Verify that the new refname is available.\n \t */\n-\tstring_list_insert(&skip, arg->oldname);\n+\tif (arg->delete_old)\n+\t\tstring_list_insert(&skip, arg->oldname);\n \tret = refs_verify_refname_available(&arg->refs->base, arg->newname,\n \t\t\t\t\t NULL, &skip, &errbuf);\n \tif (ret < 0) {\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 686781192e..055231a707 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -730,6 +730,39 @@ test_expect_success 'reflog: updates via HEAD update HEAD reflog' '\n \t)\n '\n \n+test_expect_success 'branch: copying branch with D/F conflict' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit branch branch &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${SQ}refs/heads/branch${SQ} exists; cannot create ${SQ}refs/heads/branch/moved${SQ}\n+\t\tfatal: branch copy failed\n+\t\tEOF\n+\t\ttest_must_fail git branch -c branch branch/moved 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'branch: moving branch with D/F conflict' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit branch branch &&\n+\t\tgit branch conflict &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${SQ}refs/heads/conflict${SQ} exists; cannot create ${SQ}refs/heads/conflict/moved${SQ}\n+\t\tfatal: branch rename failed\n+\t\tEOF\n+\t\ttest_must_fail git branch -m branch conflict/moved 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'worktree: adding worktree creates separate stack' '\n \ttest_when_finished \"rm -rf repo worktree\" &&\n \tgit init repo &&\n-- \n2.44.GIT\n\n"},{"id":"492204","messageId":"61901719066a2d0b70b99b275552030326fd375a.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 02/11] refs/reftable: perform explicit D/F check when writing symrefs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:13Z","receivedAt":"2024-04-04T05:48:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We already perform explicit D/F checks in all reftable callbacks which\nwrite refs, except when writing symrefs. For one this leads to an error\nmessage which isn't perfectly actionable because we only tell the user\nthat there was a D/F conflict, but not which refs conflicted with each\nother. But second, once all ref updating callbacks explicitly check for\nD/F conflicts, we can disable the D/F checks in the reftable library\nitself and thus avoid some duplicated efforts.\n\nRefactor the code that writes symref tables to explicitly call into\n`refs_verify_refname_available()` when writing symrefs.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 20 +++++++++++++++++---\n t/t0610-reftable-basics.sh | 2 +-\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 0358da14db..8a54b0d8b2 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1217,6 +1217,7 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,\n struct write_create_symref_arg {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_stack *stack;\n+\tstruct strbuf *err;\n \tconst char *refname;\n \tconst char *target;\n \tconst char *logmsg;\n@@ -1239,6 +1240,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \n \treftable_writer_set_limits(writer, ts, ts);\n \n+\tret = refs_verify_refname_available(&create->refs->base, create->refname,\n+\t\t\t\t\t NULL, NULL, create->err);\n+\tif (ret < 0)\n+\t\treturn ret;\n+\n \tret = reftable_writer_add_ref(writer, &ref);\n \tif (ret)\n \t\treturn ret;\n@@ -1280,12 +1286,14 @@ static int reftable_be_create_symref(struct ref_store *ref_store,\n \tstruct reftable_ref_store *refs =\n \t\treftable_be_downcast(ref_store, REF_STORE_WRITE, \"create_symref\");\n \tstruct reftable_stack *stack = stack_for(refs, refname, &refname);\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct write_create_symref_arg arg = {\n \t\t.refs = refs,\n \t\t.stack = stack,\n \t\t.refname = refname,\n \t\t.target = target,\n \t\t.logmsg = logmsg,\n+\t\t.err = &err,\n \t};\n \tint ret;\n \n@@ -1301,9 +1309,15 @@ static int reftable_be_create_symref(struct ref_store *ref_store,\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n-\tif (ret)\n-\t\terror(\"unable to write symref for %s: %s\", refname,\n-\t\t reftable_error_str(ret));\n+\tif (ret) {\n+\t\tif (err.len)\n+\t\t\terror(\"%s\", err.buf);\n+\t\telse\n+\t\t\terror(\"unable to write symref for %s: %s\", refname,\n+\t\t\t reftable_error_str(ret));\n+\t}\n+\n+\tstrbuf_release(&err);\n \treturn ret;\n }\n \ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 055231a707..12b0004781 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -255,7 +255,7 @@ test_expect_success 'ref transaction: creating symbolic ref fails with F/D confl\n \tgit init repo &&\n \ttest_commit -C repo A &&\n \tcat >expect <<-EOF &&\n-\terror: unable to write symref for refs/heads: file/directory conflict\n+\terror: ${SQ}refs/heads/main${SQ} exists; cannot create ${SQ}refs/heads${SQ}\n \tEOF\n \ttest_must_fail git -C repo symbolic-ref refs/heads refs/heads/foo 2>err &&\n \ttest_cmp expect err\n-- \n2.44.GIT\n\n"},{"id":"492205","messageId":"80008cc5e7772f5b6cc93d3cc898b2d2951a3588.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 03/11] refs/reftable: skip duplicate name checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:17Z","receivedAt":"2024-04-04T05:48:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"All the callback functions which write refs in the reftable backend\nperform D/F conflict checks via `refs_verify_refname_available()`. But\nin reality we perform these D/F conflict checks a second time in the\nreftable library via `stack_check_addition()`.\n\nInterestingly, the code in the reftable library is inferior compared to\nthe generic function:\n\n - It is slower than `refs_verify_refname_available()`, even though\n this can probably be optimized.\n\n - It does not provide a proper error message to the caller, and thus\n all the user would see is a generic \"file/directory conflict\"\n message.\n\nDisable the D/F conflict checks in the reftable library by setting the\n`skip_name_check` write option. This results in a non-negligible speedup\nwhen writing many refs. The following benchmark writes 100k refs in a\nsingle transaction:\n\n Benchmark 1: update-ref: create many refs (HEAD~)\n Time (mean ± σ): 3.241 s ± 0.040 s [User: 1.854 s, System: 1.381 s]\n Range (min … max): 3.185 s … 3.454 s 100 runs\n\n Benchmark 2: update-ref: create many refs (HEAD)\n Time (mean ± σ): 2.878 s ± 0.024 s [User: 1.506 s, System: 1.367 s]\n Range (min … max): 2.838 s … 2.960 s 100 runs\n\n Summary\n update-ref: create many refs (HEAD~) ran\n 1.13 ± 0.02 times faster than update-ref: create many refs (HEAD)\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 8a54b0d8b2..7515dd3019 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -247,6 +247,11 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.block_size = 4096;\n \trefs->write_options.hash_id = repo->hash_algo->format_id;\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n+\t/*\n+\t * We verify names via `refs_verify_refname_available()`, so there is\n+\t * no need to do the same checks in the reftable library again.\n+\t */\n+\trefs->write_options.skip_name_check = 1;\n \n \t/*\n \t * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.\n-- \n2.44.GIT\n\n"},{"id":"492206","messageId":"3497a570b4988e99c275acf41267f6d729717657.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 04/11] reftable: remove name checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:21Z","receivedAt":"2024-04-04T05:48:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commit we have disabled name checks in the \"reftable\"\nbackend. These checks were responsible for verifying multiple things\nwhen writing records to the reftable stack:\n\n - Detecting file/directory conflicts. Starting with the preceding\n commits this is now handled by the reftable backend itself via\n `refs_verify_refname_available()`.\n\n - Validating refnames. This is handled by `check_refname_format()` in\n the generic ref transacton layer.\n\nThe code in the reftable library is thus not used anymore and likely to\nbitrot over time. Remove it.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 2 -\n refs/reftable-backend.c | 5 -\n reftable/error.c | 2 -\n reftable/refname.c | 209 -------------------------------------\n reftable/refname.h | 29 -----\n reftable/refname_test.c | 101 ------------------\n reftable/reftable-error.h | 3 -\n reftable/reftable-tests.h | 1 -\n reftable/reftable-writer.h | 4 -\n reftable/stack.c | 67 +-----------\n reftable/stack_test.c | 39 -------\n t/helper/test-reftable.c | 1 -\n 12 files changed, 1 insertion(+), 462 deletions(-)\n delete mode 100644 reftable/refname.c\n delete mode 100644 reftable/refname.h\n delete mode 100644 reftable/refname_test.c\n\ndiff --git a/Makefile b/Makefile\nindex c43c1bd1a0..05e3d37581 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2655,7 +2655,6 @@ REFTABLE_OBJS += reftable/merged.o\n REFTABLE_OBJS += reftable/pq.o\n REFTABLE_OBJS += reftable/reader.o\n REFTABLE_OBJS += reftable/record.o\n-REFTABLE_OBJS += reftable/refname.o\n REFTABLE_OBJS += reftable/generic.o\n REFTABLE_OBJS += reftable/stack.o\n REFTABLE_OBJS += reftable/tree.o\n@@ -2668,7 +2667,6 @@ REFTABLE_TEST_OBJS += reftable/merged_test.o\n REFTABLE_TEST_OBJS += reftable/pq_test.o\n REFTABLE_TEST_OBJS += reftable/record_test.o\n REFTABLE_TEST_OBJS += reftable/readwrite_test.o\n-REFTABLE_TEST_OBJS += reftable/refname_test.o\n REFTABLE_TEST_OBJS += reftable/stack_test.o\n REFTABLE_TEST_OBJS += reftable/test_framework.o\n REFTABLE_TEST_OBJS += reftable/tree_test.o\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 7515dd3019..8a54b0d8b2 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -247,11 +247,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.block_size = 4096;\n \trefs->write_options.hash_id = repo->hash_algo->format_id;\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n-\t/*\n-\t * We verify names via `refs_verify_refname_available()`, so there is\n-\t * no need to do the same checks in the reftable library again.\n-\t */\n-\trefs->write_options.skip_name_check = 1;\n \n \t/*\n \t * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.\ndiff --git a/reftable/error.c b/reftable/error.c\nindex 0d1766735e..169f89d2f1 100644\n--- a/reftable/error.c\n+++ b/reftable/error.c\n@@ -27,8 +27,6 @@ const char *reftable_error_str(int err)\n \t\treturn \"misuse of the reftable API\";\n \tcase REFTABLE_ZLIB_ERROR:\n \t\treturn \"zlib failure\";\n-\tcase REFTABLE_NAME_CONFLICT:\n-\t\treturn \"file/directory conflict\";\n \tcase REFTABLE_EMPTY_TABLE_ERROR:\n \t\treturn \"wrote empty table\";\n \tcase REFTABLE_REFNAME_ERROR:\ndiff --git a/reftable/refname.c b/reftable/refname.c\ndeleted file mode 100644\nindex 7570e4acf9..0000000000\n--- a/reftable/refname.c\n+++ /dev/null\n@@ -1,209 +0,0 @@\n-/*\n- Copyright 2020 Google LLC\n-\n- Use of this source code is governed by a BSD-style\n- license that can be found in the LICENSE file or at\n- https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#include \"system.h\"\n-#include \"reftable-error.h\"\n-#include \"basics.h\"\n-#include \"refname.h\"\n-#include \"reftable-iterator.h\"\n-\n-struct find_arg {\n-\tchar **names;\n-\tconst char *want;\n-};\n-\n-static int find_name(size_t k, void *arg)\n-{\n-\tstruct find_arg *f_arg = arg;\n-\treturn strcmp(f_arg->names[k], f_arg->want) >= 0;\n-}\n-\n-static int modification_has_ref(struct modification *mod, const char *name)\n-{\n-\tstruct reftable_ref_record ref = { NULL };\n-\tint err = 0;\n-\n-\tif (mod->add_len > 0) {\n-\t\tstruct find_arg arg = {\n-\t\t\t.names = mod->add,\n-\t\t\t.want = name,\n-\t\t};\n-\t\tint idx = binsearch(mod->add_len, find_name, &arg);\n-\t\tif (idx < mod->add_len && !strcmp(mod->add[idx], name)) {\n-\t\t\treturn 0;\n-\t\t}\n-\t}\n-\n-\tif (mod->del_len > 0) {\n-\t\tstruct find_arg arg = {\n-\t\t\t.names = mod->del,\n-\t\t\t.want = name,\n-\t\t};\n-\t\tint idx = binsearch(mod->del_len, find_name, &arg);\n-\t\tif (idx < mod->del_len && !strcmp(mod->del[idx], name)) {\n-\t\t\treturn 1;\n-\t\t}\n-\t}\n-\n-\terr = reftable_table_read_ref(&mod->tab, name, &ref);\n-\treftable_ref_record_release(&ref);\n-\treturn err;\n-}\n-\n-static void modification_release(struct modification *mod)\n-{\n-\t/* don't delete the strings themselves; they're owned by ref records.\n-\t */\n-\tFREE_AND_NULL(mod->add);\n-\tFREE_AND_NULL(mod->del);\n-\tmod->add_len = 0;\n-\tmod->del_len = 0;\n-}\n-\n-static int modification_has_ref_with_prefix(struct modification *mod,\n-\t\t\t\t\t const char *prefix)\n-{\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_ref_record ref = { NULL };\n-\tint err = 0;\n-\n-\tif (mod->add_len > 0) {\n-\t\tstruct find_arg arg = {\n-\t\t\t.names = mod->add,\n-\t\t\t.want = prefix,\n-\t\t};\n-\t\tint idx = binsearch(mod->add_len, find_name, &arg);\n-\t\tif (idx < mod->add_len &&\n-\t\t !strncmp(prefix, mod->add[idx], strlen(prefix)))\n-\t\t\tgoto done;\n-\t}\n-\terr = reftable_table_seek_ref(&mod->tab, &it, prefix);\n-\tif (err)\n-\t\tgoto done;\n-\n-\twhile (1) {\n-\t\terr = reftable_iterator_next_ref(&it, &ref);\n-\t\tif (err)\n-\t\t\tgoto done;\n-\n-\t\tif (mod->del_len > 0) {\n-\t\t\tstruct find_arg arg = {\n-\t\t\t\t.names = mod->del,\n-\t\t\t\t.want = ref.refname,\n-\t\t\t};\n-\t\t\tint idx = binsearch(mod->del_len, find_name, &arg);\n-\t\t\tif (idx < mod->del_len &&\n-\t\t\t !strcmp(ref.refname, mod->del[idx])) {\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t}\n-\n-\t\tif (strncmp(ref.refname, prefix, strlen(prefix))) {\n-\t\t\terr = 1;\n-\t\t\tgoto done;\n-\t\t}\n-\t\terr = 0;\n-\t\tgoto done;\n-\t}\n-\n-done:\n-\treftable_ref_record_release(&ref);\n-\treftable_iterator_destroy(&it);\n-\treturn err;\n-}\n-\n-static int validate_refname(const char *name)\n-{\n-\twhile (1) {\n-\t\tchar *next = strchr(name, '/');\n-\t\tif (!*name) {\n-\t\t\treturn REFTABLE_REFNAME_ERROR;\n-\t\t}\n-\t\tif (!next) {\n-\t\t\treturn 0;\n-\t\t}\n-\t\tif (next - name == 0 || (next - name == 1 && *name == '.') ||\n-\t\t (next - name == 2 && name[0] == '.' && name[1] == '.'))\n-\t\t\treturn REFTABLE_REFNAME_ERROR;\n-\t\tname = next + 1;\n-\t}\n-\treturn 0;\n-}\n-\n-int validate_ref_record_addition(struct reftable_table tab,\n-\t\t\t\t struct reftable_ref_record *recs, size_t sz)\n-{\n-\tstruct modification mod = {\n-\t\t.tab = tab,\n-\t\t.add = reftable_calloc(sz, sizeof(*mod.add)),\n-\t\t.del = reftable_calloc(sz, sizeof(*mod.del)),\n-\t};\n-\tint i = 0;\n-\tint err = 0;\n-\tfor (; i < sz; i++) {\n-\t\tif (reftable_ref_record_is_deletion(&recs[i])) {\n-\t\t\tmod.del[mod.del_len++] = recs[i].refname;\n-\t\t} else {\n-\t\t\tmod.add[mod.add_len++] = recs[i].refname;\n-\t\t}\n-\t}\n-\n-\terr = modification_validate(&mod);\n-\tmodification_release(&mod);\n-\treturn err;\n-}\n-\n-static void strbuf_trim_component(struct strbuf *sl)\n-{\n-\twhile (sl->len > 0) {\n-\t\tint is_slash = (sl->buf[sl->len - 1] == '/');\n-\t\tstrbuf_setlen(sl, sl->len - 1);\n-\t\tif (is_slash)\n-\t\t\tbreak;\n-\t}\n-}\n-\n-int modification_validate(struct modification *mod)\n-{\n-\tstruct strbuf slashed = STRBUF_INIT;\n-\tint err = 0;\n-\tint i = 0;\n-\tfor (; i < mod->add_len; i++) {\n-\t\terr = validate_refname(mod->add[i]);\n-\t\tif (err)\n-\t\t\tgoto done;\n-\t\tstrbuf_reset(&slashed);\n-\t\tstrbuf_addstr(&slashed, mod->add[i]);\n-\t\tstrbuf_addstr(&slashed, \"/\");\n-\n-\t\terr = modification_has_ref_with_prefix(mod, slashed.buf);\n-\t\tif (err == 0) {\n-\t\t\terr = REFTABLE_NAME_CONFLICT;\n-\t\t\tgoto done;\n-\t\t}\n-\t\tif (err < 0)\n-\t\t\tgoto done;\n-\n-\t\tstrbuf_reset(&slashed);\n-\t\tstrbuf_addstr(&slashed, mod->add[i]);\n-\t\twhile (slashed.len) {\n-\t\t\tstrbuf_trim_component(&slashed);\n-\t\t\terr = modification_has_ref(mod, slashed.buf);\n-\t\t\tif (err == 0) {\n-\t\t\t\terr = REFTABLE_NAME_CONFLICT;\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\t\t\tif (err < 0)\n-\t\t\t\tgoto done;\n-\t\t}\n-\t}\n-\terr = 0;\n-done:\n-\tstrbuf_release(&slashed);\n-\treturn err;\n-}\ndiff --git a/reftable/refname.h b/reftable/refname.h\ndeleted file mode 100644\nindex a24b40fcb4..0000000000\n--- a/reftable/refname.h\n+++ /dev/null\n@@ -1,29 +0,0 @@\n-/*\n- Copyright 2020 Google LLC\n-\n- Use of this source code is governed by a BSD-style\n- license that can be found in the LICENSE file or at\n- https://developers.google.com/open-source/licenses/bsd\n-*/\n-#ifndef REFNAME_H\n-#define REFNAME_H\n-\n-#include \"reftable-record.h\"\n-#include \"reftable-generic.h\"\n-\n-struct modification {\n-\tstruct reftable_table tab;\n-\n-\tchar **add;\n-\tsize_t add_len;\n-\n-\tchar **del;\n-\tsize_t del_len;\n-};\n-\n-int validate_ref_record_addition(struct reftable_table tab,\n-\t\t\t\t struct reftable_ref_record *recs, size_t sz);\n-\n-int modification_validate(struct modification *mod);\n-\n-#endif\ndiff --git a/reftable/refname_test.c b/reftable/refname_test.c\ndeleted file mode 100644\nindex b9cc62554e..0000000000\n--- a/reftable/refname_test.c\n+++ /dev/null\n@@ -1,101 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-license that can be found in the LICENSE file or at\n-https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#include \"basics.h\"\n-#include \"block.h\"\n-#include \"blocksource.h\"\n-#include \"reader.h\"\n-#include \"record.h\"\n-#include \"refname.h\"\n-#include \"reftable-error.h\"\n-#include \"reftable-writer.h\"\n-#include \"system.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-\n-struct testcase {\n-\tchar *add;\n-\tchar *del;\n-\tint error_code;\n-};\n-\n-static void test_conflict(void)\n-{\n-\tstruct reftable_write_options opts = { 0 };\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\tstruct reftable_ref_record rec = {\n-\t\t.refname = \"a/b\",\n-\t\t.value_type = REFTABLE_REF_SYMREF,\n-\t\t.value.symref = \"destination\", /* make sure it's not a symref.\n-\t\t\t\t\t\t*/\n-\t\t.update_index = 1,\n-\t};\n-\tint err;\n-\tint i;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader *rd = NULL;\n-\tstruct reftable_table tab = { NULL };\n-\tstruct testcase cases[] = {\n-\t\t{ \"a/b/c\", NULL, REFTABLE_NAME_CONFLICT },\n-\t\t{ \"b\", NULL, 0 },\n-\t\t{ \"a\", NULL, REFTABLE_NAME_CONFLICT },\n-\t\t{ \"a\", \"a/b\", 0 },\n-\n-\t\t{ \"p/\", NULL, REFTABLE_REFNAME_ERROR },\n-\t\t{ \"p//q\", NULL, REFTABLE_REFNAME_ERROR },\n-\t\t{ \"p/./q\", NULL, REFTABLE_REFNAME_ERROR },\n-\t\t{ \"p/../q\", NULL, REFTABLE_REFNAME_ERROR },\n-\n-\t\t{ \"a/b/c\", \"a/b\", 0 },\n-\t\t{ NULL, \"a//b\", 0 },\n-\t};\n-\treftable_writer_set_limits(w, 1, 1);\n-\n-\terr = reftable_writer_add_ref(w, &rec);\n-\tEXPECT_ERR(err);\n-\n-\terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n-\treftable_writer_free(w);\n-\n-\tblock_source_from_strbuf(&source, &buf);\n-\terr = reftable_new_reader(&rd, &source, \"filename\");\n-\tEXPECT_ERR(err);\n-\n-\treftable_table_from_reader(&tab, rd);\n-\n-\tfor (i = 0; i < ARRAY_SIZE(cases); i++) {\n-\t\tstruct modification mod = {\n-\t\t\t.tab = tab,\n-\t\t};\n-\n-\t\tif (cases[i].add) {\n-\t\t\tmod.add = &cases[i].add;\n-\t\t\tmod.add_len = 1;\n-\t\t}\n-\t\tif (cases[i].del) {\n-\t\t\tmod.del = &cases[i].del;\n-\t\t\tmod.del_len = 1;\n-\t\t}\n-\n-\t\terr = modification_validate(&mod);\n-\t\tEXPECT(err == cases[i].error_code);\n-\t}\n-\n-\treftable_reader_free(rd);\n-\tstrbuf_release(&buf);\n-}\n-\n-int refname_test_main(int argc, const char *argv[])\n-{\n-\tRUN_TEST(test_conflict);\n-\treturn 0;\n-}\ndiff --git a/reftable/reftable-error.h b/reftable/reftable-error.h\nindex 4c457aaaf8..3a5f5b92c6 100644\n--- a/reftable/reftable-error.h\n+++ b/reftable/reftable-error.h\n@@ -48,9 +48,6 @@ enum reftable_error {\n \t/* Wrote a table without blocks. */\n \tREFTABLE_EMPTY_TABLE_ERROR = -8,\n \n-\t/* Dir/file conflict. */\n-\tREFTABLE_NAME_CONFLICT = -9,\n-\n \t/* Invalid ref name. */\n \tREFTABLE_REFNAME_ERROR = -10,\n \ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex 0019cbcfa4..114cc3d053 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -14,7 +14,6 @@ int block_test_main(int argc, const char **argv);\n int merged_test_main(int argc, const char **argv);\n int pq_test_main(int argc, const char **argv);\n int record_test_main(int argc, const char **argv);\n-int refname_test_main(int argc, const char **argv);\n int readwrite_test_main(int argc, const char **argv);\n int stack_test_main(int argc, const char **argv);\n int tree_test_main(int argc, const char **argv);\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 7c7cae5f99..3c119e2bbb 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -38,10 +38,6 @@ struct reftable_write_options {\n \t/* Default mode for creating files. If unset, use 0666 (+umask) */\n \tunsigned int default_permissions;\n \n-\t/* boolean: do not check ref names for validity or dir/file conflicts.\n-\t */\n-\tunsigned skip_name_check : 1;\n-\n \t/* boolean: copy log messages exactly. If unset, check that the message\n \t * is a single line, and add '\\n' if missing.\n \t */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 1ecf1b9751..e264df5ced 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \"merged.h\"\n #include \"reader.h\"\n-#include \"refname.h\"\n #include \"reftable-error.h\"\n+#include \"reftable-generic.h\"\n #include \"reftable-record.h\"\n #include \"reftable-merged.h\"\n #include \"writer.h\"\n@@ -27,8 +27,6 @@ static int stack_write_compact(struct reftable_stack *st,\n \t\t\t struct reftable_writer *wr,\n \t\t\t size_t first, size_t last,\n \t\t\t struct reftable_log_expiry_config *config);\n-static int stack_check_addition(struct reftable_stack *st,\n-\t\t\t\tconst char *new_tab_name);\n static void reftable_addition_close(struct reftable_addition *add);\n static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\t\t\t\t int reuse_open);\n@@ -781,10 +779,6 @@ int reftable_addition_add(struct reftable_addition *add,\n \t\tgoto done;\n \t}\n \n-\terr = stack_check_addition(add->stack, get_tempfile_path(tab_file));\n-\tif (err < 0)\n-\t\tgoto done;\n-\n \tif (wr->min_update_index < add->next_update_index) {\n \t\terr = REFTABLE_API_ERROR;\n \t\tgoto done;\n@@ -1340,65 +1334,6 @@ int reftable_stack_read_log(struct reftable_stack *st, const char *refname,\n \treturn err;\n }\n \n-static int stack_check_addition(struct reftable_stack *st,\n-\t\t\t\tconst char *new_tab_name)\n-{\n-\tint err = 0;\n-\tstruct reftable_block_source src = { NULL };\n-\tstruct reftable_reader *rd = NULL;\n-\tstruct reftable_table tab = { NULL };\n-\tstruct reftable_ref_record *refs = NULL;\n-\tstruct reftable_iterator it = { NULL };\n-\tint cap = 0;\n-\tint len = 0;\n-\tint i = 0;\n-\n-\tif (st->config.skip_name_check)\n-\t\treturn 0;\n-\n-\terr = reftable_block_source_from_file(&src, new_tab_name);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_new_reader(&rd, &src, new_tab_name);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_reader_seek_ref(rd, &it, \"\");\n-\tif (err > 0) {\n-\t\terr = 0;\n-\t\tgoto done;\n-\t}\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\twhile (1) {\n-\t\tstruct reftable_ref_record ref = { NULL };\n-\t\terr = reftable_iterator_next_ref(&it, &ref);\n-\t\tif (err > 0)\n-\t\t\tbreak;\n-\t\tif (err < 0)\n-\t\t\tgoto done;\n-\n-\t\tREFTABLE_ALLOC_GROW(refs, len + 1, cap);\n-\t\trefs[len++] = ref;\n-\t}\n-\n-\treftable_table_from_merged_table(&tab, reftable_stack_merged_table(st));\n-\n-\terr = validate_ref_record_addition(tab, refs, len);\n-\n-done:\n-\tfor (i = 0; i < len; i++) {\n-\t\treftable_ref_record_release(&refs[i]);\n-\t}\n-\n-\tfree(refs);\n-\treftable_iterator_destroy(&it);\n-\treftable_reader_free(rd);\n-\treturn err;\n-}\n-\n static int is_table_name(const char *s)\n {\n \tconst char *dot = strrchr(s, '.');\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 0dc9a44648..b88097c3b6 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -353,44 +353,6 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n \tclear_dir(dir);\n }\n \n-static void test_reftable_stack_validate_refname(void)\n-{\n-\tstruct reftable_write_options cfg = { 0 };\n-\tstruct reftable_stack *st = NULL;\n-\tint err;\n-\tchar *dir = get_tmp_dir(__LINE__);\n-\n-\tint i;\n-\tstruct reftable_ref_record ref = {\n-\t\t.refname = \"a/b\",\n-\t\t.update_index = 1,\n-\t\t.value_type = REFTABLE_REF_SYMREF,\n-\t\t.value.symref = \"master\",\n-\t};\n-\tchar *additions[] = { \"a\", \"a/b/c\" };\n-\n-\terr = reftable_new_stack(&st, dir, cfg);\n-\tEXPECT_ERR(err);\n-\n-\terr = reftable_stack_add(st, &write_test_ref, &ref);\n-\tEXPECT_ERR(err);\n-\n-\tfor (i = 0; i < ARRAY_SIZE(additions); i++) {\n-\t\tstruct reftable_ref_record ref = {\n-\t\t\t.refname = additions[i],\n-\t\t\t.update_index = 1,\n-\t\t\t.value_type = REFTABLE_REF_SYMREF,\n-\t\t\t.value.symref = \"master\",\n-\t\t};\n-\n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n-\t\tEXPECT(err == REFTABLE_NAME_CONFLICT);\n-\t}\n-\n-\treftable_stack_destroy(st);\n-\tclear_dir(dir);\n-}\n-\n static int write_error(struct reftable_writer *wr, void *arg)\n {\n \treturn *((int *)arg);\n@@ -1097,7 +1059,6 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n-\tRUN_TEST(test_reftable_stack_validate_refname);\n \tRUN_TEST(test_sizes_to_segments);\n \tRUN_TEST(test_sizes_to_segments_all_equal);\n \tRUN_TEST(test_sizes_to_segments_empty);\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 00237ef0d9..bae731669c 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -13,7 +13,6 @@ int cmd__reftable(int argc, const char **argv)\n \treadwrite_test_main(argc, argv);\n \tmerged_test_main(argc, argv);\n \tstack_test_main(argc, argv);\n-\trefname_test_main(argc, argv);\n \treturn 0;\n }\n \n-- \n2.44.GIT\n\n"},{"id":"492207","messageId":"f892a3007bbbd7ee5060a5205005db6339ce7206.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 05/11] refs/reftable: don't recompute committer ident","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:25Z","receivedAt":"2024-04-04T05:48:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to write reflog entries we need to compute the committer's\nidentity as it gets encoded in the log record itself. The reftable\nbackend does this via `git_committer_info()` and `split_ident_line()` in\n`fill_reftable_log_record()`, which use the Git config as well as\nenvironment variables to figure out the identity.\n\nWhile most callers would only call `fill_reftable_log_record()` once or\ntwice, `write_transaction_table()` will call it as many times as there\nare queued ref updates. This can be quite a waste of effort when writing\nmany refs with reflog entries in a single transaction.\n\nRefactor the code to pre-compute the committer information. This results\nin a small speedup when writing 100000 refs in a single transaction:\n\n Benchmark 1: update-ref: create many refs (HEAD~)\n Time (mean ± σ): 2.895 s ± 0.020 s [User: 1.516 s, System: 1.374 s]\n Range (min … max): 2.868 s … 2.983 s 100 runs\n\n Benchmark 2: update-ref: create many refs (HEAD)\n Time (mean ± σ): 2.845 s ± 0.017 s [User: 1.461 s, System: 1.379 s]\n Range (min … max): 2.803 s … 2.913 s 100 runs\n\n Summary\n update-ref: create many refs (HEAD) ran\n 1.02 ± 0.01 times faster than update-ref: create many refs (HEAD~)\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 52 +++++++++++++++++++++++++++--------------\n 1 file changed, 34 insertions(+), 18 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 8a54b0d8b2..a5ef36ffa9 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -171,32 +171,30 @@ static int should_write_log(struct ref_store *refs, const char *refname)\n \t}\n }\n \n-static void fill_reftable_log_record(struct reftable_log_record *log)\n+static void fill_reftable_log_record(struct reftable_log_record *log, const struct ident_split *split)\n {\n-\tconst char *info = git_committer_info(0);\n-\tstruct ident_split split = {0};\n+\tconst char *tz_begin;\n \tint sign = 1;\n \n-\tif (split_ident_line(&split, info, strlen(info)))\n-\t\tBUG(\"failed splitting committer info\");\n-\n \treftable_log_record_release(log);\n \tlog->value_type = REFTABLE_LOG_UPDATE;\n \tlog->value.update.name =\n-\t\txstrndup(split.name_begin, split.name_end - split.name_begin);\n+\t\txstrndup(split->name_begin, split->name_end - split->name_begin);\n \tlog->value.update.email =\n-\t\txstrndup(split.mail_begin, split.mail_end - split.mail_begin);\n-\tlog->value.update.time = atol(split.date_begin);\n-\tif (*split.tz_begin == '-') {\n+\t\txstrndup(split->mail_begin, split->mail_end - split->mail_begin);\n+\tlog->value.update.time = atol(split->date_begin);\n+\n+\ttz_begin = split->tz_begin;\n+\tif (*tz_begin == '-') {\n \t\tsign = -1;\n-\t\tsplit.tz_begin++;\n+\t\ttz_begin++;\n \t}\n-\tif (*split.tz_begin == '+') {\n+\tif (*tz_begin == '+') {\n \t\tsign = 1;\n-\t\tsplit.tz_begin++;\n+\t\ttz_begin++;\n \t}\n \n-\tlog->value.update.tz_offset = sign * atoi(split.tz_begin);\n+\tlog->value.update.tz_offset = sign * atoi(tz_begin);\n }\n \n static int read_ref_without_reload(struct reftable_stack *stack,\n@@ -1018,9 +1016,15 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\treftable_stack_merged_table(arg->stack);\n \tuint64_t ts = reftable_stack_next_update_index(arg->stack);\n \tstruct reftable_log_record *logs = NULL;\n+\tstruct ident_split committer_ident = {0};\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n+\tconst char *committer_info;\n \tint ret = 0;\n \n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n \tQSORT(arg->updates, arg->updates_nr, transaction_update_cmp);\n \n \treftable_writer_set_limits(writer, ts, ts);\n@@ -1086,7 +1090,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\t\tlog = &logs[logs_nr++];\n \t\t\tmemset(log, 0, sizeof(*log));\n \n-\t\t\tfill_reftable_log_record(log);\n+\t\t\tfill_reftable_log_record(log, &committer_ident);\n \t\t\tlog->update_index = ts;\n \t\t\tlog->refname = xstrdup(u->refname);\n \t\t\tmemcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ);\n@@ -1233,9 +1237,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \t\t.value.symref = (char *)create->target,\n \t\t.update_index = ts,\n \t};\n+\tstruct ident_split committer_ident = {0};\n \tstruct reftable_log_record log = {0};\n \tstruct object_id new_oid;\n \tstruct object_id old_oid;\n+\tconst char *committer_info;\n \tint ret;\n \n \treftable_writer_set_limits(writer, ts, ts);\n@@ -1263,7 +1269,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \t !should_write_log(&create->refs->base, create->refname))\n \t\treturn 0;\n \n-\tfill_reftable_log_record(&log);\n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n+\tfill_reftable_log_record(&log, &committer_ident);\n \tlog.refname = xstrdup(create->refname);\n \tlog.update_index = ts;\n \tlog.value.update.message = xstrndup(create->logmsg,\n@@ -1339,10 +1349,16 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \tstruct reftable_log_record old_log = {0}, *logs = NULL;\n \tstruct reftable_iterator it = {0};\n \tstruct string_list skip = STRING_LIST_INIT_NODUP;\n+\tstruct ident_split committer_ident = {0};\n \tstruct strbuf errbuf = STRBUF_INIT;\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n+\tconst char *committer_info;\n \tint ret;\n \n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n \tif (reftable_stack_read_ref(arg->stack, arg->oldname, &old_ref)) {\n \t\tret = error(_(\"refname %s not found\"), arg->oldname);\n \t\tgoto done;\n@@ -1417,7 +1433,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \n \t\tALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n \t\tmemset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));\n-\t\tfill_reftable_log_record(&logs[logs_nr]);\n+\t\tfill_reftable_log_record(&logs[logs_nr], &committer_ident);\n \t\tlogs[logs_nr].refname = (char *)arg->newname;\n \t\tlogs[logs_nr].update_index = deletion_ts;\n \t\tlogs[logs_nr].value.update.message =\n@@ -1449,7 +1465,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \t */\n \tALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n \tmemset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));\n-\tfill_reftable_log_record(&logs[logs_nr]);\n+\tfill_reftable_log_record(&logs[logs_nr], &committer_ident);\n \tlogs[logs_nr].refname = (char *)arg->newname;\n \tlogs[logs_nr].update_index = creation_ts;\n \tlogs[logs_nr].value.update.message =\n-- \n2.44.GIT\n\n"},{"id":"492208","messageId":"4877ab39212867e91058c60f99fe0dc2a592d583.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 06/11] reftable/writer: refactorings for `writer_add_record()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:29Z","receivedAt":"2024-04-04T05:48:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Large parts of the reftable library do not conform to Git's typical code\nstyle. Refactor `writer_add_record()` such that it conforms better to it\nand add some documentation that explains some of its more intricate\nbehaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 38 +++++++++++++++++++++++++++-----------\n 1 file changed, 27 insertions(+), 11 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 1d9ff0fbfa..0ad5eb8887 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -209,7 +209,8 @@ static int writer_add_record(struct reftable_writer *w,\n \t\t\t struct reftable_record *rec)\n {\n \tstruct strbuf key = STRBUF_INIT;\n-\tint err = -1;\n+\tint err;\n+\n \treftable_record_key(rec, &key);\n \tif (strbuf_cmp(&w->last_key, &key) >= 0) {\n \t\terr = REFTABLE_API_ERROR;\n@@ -218,27 +219,42 @@ static int writer_add_record(struct reftable_writer *w,\n \n \tstrbuf_reset(&w->last_key);\n \tstrbuf_addbuf(&w->last_key, &key);\n-\tif (!w->block_writer) {\n+\tif (!w->block_writer)\n \t\twriter_reinit_block_writer(w, reftable_record_type(rec));\n-\t}\n \n-\tassert(block_writer_type(w->block_writer) == reftable_record_type(rec));\n+\tif (block_writer_type(w->block_writer) != reftable_record_type(rec))\n+\t\tBUG(\"record of type %d added to writer of type %d\",\n+\t\t reftable_record_type(rec), block_writer_type(w->block_writer));\n \n-\tif (block_writer_add(w->block_writer, rec) == 0) {\n+\t/*\n+\t * Try to add the record to the writer. If this succeeds then we're\n+\t * done. Otherwise the block writer may have hit the block size limit\n+\t * and needs to be flushed.\n+\t */\n+\tif (!block_writer_add(w->block_writer, rec)) {\n \t\terr = 0;\n \t\tgoto done;\n \t}\n \n+\t/*\n+\t * The current block is full, so we need to flush and reinitialize the\n+\t * writer to start writing the next block.\n+\t */\n \terr = writer_flush_block(w);\n-\tif (err < 0) {\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n-\n \twriter_reinit_block_writer(w, reftable_record_type(rec));\n+\n+\t/*\n+\t * Try to add the record to the writer again. If this still fails then\n+\t * the record does not fit into the block size.\n+\t *\n+\t * TODO: it would be great to have `block_writer_add()` return proper\n+\t * error codes so that we don't have to second-guess the failure\n+\t * mode here.\n+\t */\n \terr = block_writer_add(w->block_writer, rec);\n-\tif (err == -1) {\n-\t\t/* we are writing into memory, so an error can only mean it\n-\t\t * doesn't fit. */\n+\tif (err) {\n \t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tgoto done;\n \t}\n-- \n2.44.GIT\n\n"},{"id":"492209","messageId":"8f1c5b416986f6d4934bbfefe18ad7ed55231671.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 07/11] reftable/writer: refactorings for `writer_flush_nonempty_block()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:33Z","receivedAt":"2024-04-04T05:48:37Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Large parts of the reftable library do not conform to Git's typical code\nstyle. Refactor `writer_flush_nonempty_block()` such that it conforms\nbetter to it and add some documentation that explains some of its more\nintricate behaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 72 +++++++++++++++++++++++++++++------------------\n 1 file changed, 44 insertions(+), 28 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 0ad5eb8887..d347ec4cc6 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -659,58 +659,74 @@ static void writer_clear_index(struct reftable_writer *w)\n \tw->index_cap = 0;\n }\n \n-static const int debug = 0;\n-\n static int writer_flush_nonempty_block(struct reftable_writer *w)\n {\n+\tstruct reftable_index_record index_record = {\n+\t\t.last_key = STRBUF_INIT,\n+\t};\n \tuint8_t typ = block_writer_type(w->block_writer);\n-\tstruct reftable_block_stats *bstats =\n-\t\twriter_reftable_block_stats(w, typ);\n-\tuint64_t block_typ_off = (bstats->blocks == 0) ? w->next : 0;\n-\tint raw_bytes = block_writer_finish(w->block_writer);\n-\tint padding = 0;\n-\tint err = 0;\n-\tstruct reftable_index_record ir = { .last_key = STRBUF_INIT };\n+\tstruct reftable_block_stats *bstats;\n+\tint raw_bytes, padding = 0, err;\n+\tuint64_t block_typ_off;\n+\n+\t/*\n+\t * Finish the current block. This will cause the block writer to emit\n+\t * restart points and potentially compress records in case we are\n+\t * writing a log block.\n+\t *\n+\t * Note that this is still happening in memory.\n+\t */\n+\traw_bytes = block_writer_finish(w->block_writer);\n \tif (raw_bytes < 0)\n \t\treturn raw_bytes;\n \n-\tif (!w->opts.unpadded && typ != BLOCK_TYPE_LOG) {\n+\t/*\n+\t * By default, all records except for log records are padded to the\n+\t * block size.\n+\t */\n+\tif (!w->opts.unpadded && typ != BLOCK_TYPE_LOG)\n \t\tpadding = w->opts.block_size - raw_bytes;\n-\t}\n \n-\tif (block_typ_off > 0) {\n+\tbstats = writer_reftable_block_stats(w, typ);\n+\tblock_typ_off = (bstats->blocks == 0) ? w->next : 0;\n+\tif (block_typ_off > 0)\n \t\tbstats->offset = block_typ_off;\n-\t}\n-\n \tbstats->entries += w->block_writer->entries;\n \tbstats->restarts += w->block_writer->restart_len;\n \tbstats->blocks++;\n \tw->stats.blocks++;\n \n-\tif (debug) {\n-\t\tfprintf(stderr, \"block %c off %\" PRIu64 \" sz %d (%d)\\n\", typ,\n-\t\t\tw->next, raw_bytes,\n-\t\t\tget_be24(w->block + w->block_writer->header_off + 1));\n-\t}\n-\n-\tif (w->next == 0) {\n+\t/*\n+\t * If this is the first block we're writing to the table then we need\n+\t * to also write the reftable header.\n+\t */\n+\tif (!w->next)\n \t\twriter_write_header(w, w->block);\n-\t}\n \n \terr = padded_write(w, w->block, raw_bytes, padding);\n \tif (err < 0)\n \t\treturn err;\n \n+\t/*\n+\t * Add an index record for every block that we're writing. If we end up\n+\t * having more than a threshold of index records we will end up writing\n+\t * an index section in `writer_finish_section()`. Each index record\n+\t * contains the last record key of the block it is indexing as well as\n+\t * the offset of that block.\n+\t *\n+\t * Note that this also applies when flushing index blocks, in which\n+\t * case we will end up with a multi-level index.\n+\t */\n \tREFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);\n-\n-\tir.offset = w->next;\n-\tstrbuf_reset(&ir.last_key);\n-\tstrbuf_addbuf(&ir.last_key, &w->block_writer->last_key);\n-\tw->index[w->index_len] = ir;\n-\n+\tindex_record.offset = w->next;\n+\tstrbuf_reset(&index_record.last_key);\n+\tstrbuf_addbuf(&index_record.last_key, &w->block_writer->last_key);\n+\tw->index[w->index_len] = index_record;\n \tw->index_len++;\n+\n \tw->next += padding + raw_bytes;\n \tw->block_writer = NULL;\n+\n \treturn 0;\n }\n \n-- \n2.44.GIT\n\n"},{"id":"492210","messageId":"41db7414e17201f85b476af5e0183e72de450310.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 08/11] reftable/writer: unify releasing memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:37Z","receivedAt":"2024-04-04T05:48:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are two code paths which release memory of the reftable writer:\n\n - `reftable_writer_close()` releases internal state after it has\n written data.\n\n - `reftable_writer_free()` releases the block that was written to and\n the writer itself.\n\nBoth code paths free different parts of the writer, and consequently the\ncaller must make sure to call both. And while callers mostly do this\nalready, this falls apart when a write failure causes the caller to skip\ncalling `reftable_write_close()`.\n\nIntroduce a new function `reftable_writer_release()` that releases all\ninternal state and call it from both paths. Like this it is fine for the\ncaller to not call `reftable_writer_close()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex d347ec4cc6..7b70c9b666 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -149,11 +149,21 @@ void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n \tw->max_update_index = max;\n }\n \n+static void reftable_writer_release(struct reftable_writer *w)\n+{\n+\tif (w) {\n+\t\treftable_free(w->block);\n+\t\tw->block = NULL;\n+\t\tblock_writer_release(&w->block_writer_data);\n+\t\tw->block_writer = NULL;\n+\t\twriter_clear_index(w);\n+\t\tstrbuf_release(&w->last_key);\n+\t}\n+}\n+\n void reftable_writer_free(struct reftable_writer *w)\n {\n-\tif (!w)\n-\t\treturn;\n-\treftable_free(w->block);\n+\treftable_writer_release(w);\n \treftable_free(w);\n }\n \n@@ -643,16 +653,13 @@ int reftable_writer_close(struct reftable_writer *w)\n \t}\n \n done:\n-\t/* free up memory. */\n-\tblock_writer_release(&w->block_writer_data);\n-\twriter_clear_index(w);\n-\tstrbuf_release(&w->last_key);\n+\treftable_writer_release(w);\n \treturn err;\n }\n \n static void writer_clear_index(struct reftable_writer *w)\n {\n-\tfor (size_t i = 0; i < w->index_len; i++)\n+\tfor (size_t i = 0; w->index && i < w->index_len; i++)\n \t\tstrbuf_release(&w->index[i].last_key);\n \tFREE_AND_NULL(w->index);\n \tw->index_len = 0;\n-- \n2.44.GIT\n\n"},{"id":"492211","messageId":"e5c7dbe4179c38aab5fc218e4d5fa855fc8f92fa.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 09/11] reftable/writer: reset `last_key` instead of releasing it","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:42Z","receivedAt":"2024-04-04T05:48:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable writer tracks the last key that it has written so that it\ncan properly compute the compressed prefix for the next record it is\nabout to write. This last key must be reset whenever we move on to write\nthe next block, which is done in `writer_reinit_block_writer()`. We do\nthis by calling `strbuf_release()` though, which needlessly deallocates\nthe underlying buffer.\n\nConvert the code to use `strbuf_reset()` instead, which saves one\nallocation per block we're about to write. This requires us to also\namend `reftable_writer_free()` to release the buffer's memory now as we\npreviously seemingly relied on `writer_reinit_block_writer()` to release\nthe memory for us. Releasing memory here is the right thing to do\nanyway.\n\nWhile at it, convert a callsite where we truncate the buffer by setting\nits length to zero to instead use `strbuf_reset()`, too.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 7b70c9b666..32438e49b4 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -109,7 +109,7 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)\n \t\tblock_start = header_size(writer_version(w));\n \t}\n \n-\tstrbuf_release(&w->last_key);\n+\tstrbuf_reset(&w->last_key);\n \tblock_writer_init(&w->block_writer_data, typ, w->block,\n \t\t\t w->opts.block_size, block_start,\n \t\t\t hash_size(w->opts.hash_id));\n@@ -478,7 +478,7 @@ static int writer_finish_section(struct reftable_writer *w)\n \tbstats->max_index_level = max_level;\n \n \t/* Reinit lastKey, as the next section can start with any key. */\n-\tw->last_key.len = 0;\n+\tstrbuf_reset(&w->last_key);\n \n \treturn 0;\n }\n-- \n2.44.GIT\n\n"},{"id":"492212","messageId":"26f422703ff6dfeb8de9d5a41af3d322b64e7706.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 10/11] reftable/block: reuse zstream when writing log blocks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:46Z","receivedAt":"2024-04-04T05:48:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While most reftable blocks are written to disk as-is, blocks for log\nrecords are compressed with zlib. To compress them we use `compress2()`,\nwhich is a simple wrapper around the more complex `zstream` interface\nthat would require multiple function invocations.\n\nOne downside of this interface is that `compress2()` will reallocate\ninternal state of the `zstream` interface on every single invocation.\nConsequently, as we call `compress2()` for every single log block which\nwe are about to write, this can lead to quite some memory allocation\nchurn.\n\nRefactor the code so that the block writer reuses a `zstream`. This\nsignificantly reduces the number of bytes allocated when writing many\nrefs in a single transaction, as demonstrated by the following benchmark\nthat writes 100k refs in a single transaction.\n\nBefore:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,631,887 allocs, 22,631,736 frees, 1,854,670,793 bytes allocated\n\nAfter:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,620,528 allocs, 22,620,377 frees, 1,245,549,984 bytes allocated\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 80 +++++++++++++++++++++++++++++++-----------------\n reftable/block.h | 1 +\n 2 files changed, 53 insertions(+), 28 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex e2a2cee58d..9129305515 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -76,6 +76,10 @@ void block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *buf,\n \tbw->entries = 0;\n \tbw->restart_len = 0;\n \tbw->last_key.len = 0;\n+\tif (!bw->zstream) {\n+\t\tREFTABLE_CALLOC_ARRAY(bw->zstream, 1);\n+\t\tdeflateInit(bw->zstream, 9);\n+\t}\n }\n \n uint8_t block_writer_type(struct block_writer *bw)\n@@ -139,39 +143,57 @@ int block_writer_finish(struct block_writer *w)\n \tw->next += 2;\n \tput_be24(w->buf + 1 + w->header_off, w->next);\n \n+\t/*\n+\t * Log records are stored zlib-compressed. Note that the compression\n+\t * also spans over the restart points we have just written.\n+\t */\n \tif (block_writer_type(w) == BLOCK_TYPE_LOG) {\n \t\tint block_header_skip = 4 + w->header_off;\n-\t\tuLongf src_len = w->next - block_header_skip;\n-\t\tuLongf dest_cap = src_len * 1.001 + 12;\n-\t\tuint8_t *compressed;\n-\n-\t\tREFTABLE_ALLOC_ARRAY(compressed, dest_cap);\n-\n-\t\twhile (1) {\n-\t\t\tuLongf out_dest_len = dest_cap;\n-\t\t\tint zresult = compress2(compressed, &out_dest_len,\n-\t\t\t\t\t\tw->buf + block_header_skip,\n-\t\t\t\t\t\tsrc_len, 9);\n-\t\t\tif (zresult == Z_BUF_ERROR && dest_cap < LONG_MAX) {\n-\t\t\t\tdest_cap *= 2;\n-\t\t\t\tcompressed =\n-\t\t\t\t\treftable_realloc(compressed, dest_cap);\n-\t\t\t\tif (compressed)\n-\t\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\tif (Z_OK != zresult) {\n-\t\t\t\treftable_free(compressed);\n-\t\t\t\treturn REFTABLE_ZLIB_ERROR;\n-\t\t\t}\n-\n-\t\t\tmemcpy(w->buf + block_header_skip, compressed,\n-\t\t\t out_dest_len);\n-\t\t\tw->next = out_dest_len + block_header_skip;\n+\t\tuLongf src_len = w->next - block_header_skip, compressed_len;\n+\t\tunsigned char *compressed;\n+\t\tint ret;\n+\n+\t\tret = deflateReset(w->zstream);\n+\t\tif (ret != Z_OK)\n+\t\t\treturn REFTABLE_ZLIB_ERROR;\n+\n+\t\t/*\n+\t\t * Precompute the upper bound of how many bytes the compressed\n+\t\t * data may end up with. Combined with `Z_FINISH`, `deflate()`\n+\t\t * is guaranteed to return `Z_STREAM_END`.\n+\t\t */\n+\t\tcompressed_len = deflateBound(w->zstream, src_len);\n+\t\tREFTABLE_ALLOC_ARRAY(compressed, compressed_len);\n+\n+\t\tw->zstream->next_out = compressed;\n+\t\tw->zstream->avail_out = compressed_len;\n+\t\tw->zstream->next_in = w->buf + block_header_skip;\n+\t\tw->zstream->avail_in = src_len;\n+\n+\t\t/*\n+\t\t * We want to perform all decompression in a single step, which\n+\t\t * is why we can pass Z_FINISH here. As we have precomputed the\n+\t\t * deflated buffer's size via `deflateBound()` this function is\n+\t\t * guaranteed to succeed according to the zlib documentation.\n+\t\t */\n+\t\tret = deflate(w->zstream, Z_FINISH);\n+\t\tif (ret != Z_STREAM_END) {\n \t\t\treftable_free(compressed);\n-\t\t\tbreak;\n+\t\t\treturn REFTABLE_ZLIB_ERROR;\n \t\t}\n+\n+\t\t/*\n+\t\t * Overwrite the uncompressed data we have already written and\n+\t\t * adjust the `next` pointer to point right after the\n+\t\t * compressed data.\n+\t\t */\n+\t\tmemcpy(w->buf + block_header_skip, compressed,\n+\t\t w->zstream->total_out);\n+\t\tw->next = w->zstream->total_out + block_header_skip;\n+\n+\t\treftable_free(compressed);\n \t}\n+\n \treturn w->next;\n }\n \n@@ -425,6 +447,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \n void block_writer_release(struct block_writer *bw)\n {\n+\tdeflateEnd(bw->zstream);\n+\tFREE_AND_NULL(bw->zstream);\n \tFREE_AND_NULL(bw->restarts);\n \tstrbuf_release(&bw->last_key);\n \t/* the block is not owned. */\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 47acc62c0a..1375957fc8 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -18,6 +18,7 @@ license that can be found in the LICENSE file or at\n * allocation overhead.\n */\n struct block_writer {\n+\tz_stream *zstream;\n \tuint8_t *buf;\n \tuint32_t block_size;\n \n-- \n2.44.GIT\n\n"},{"id":"492213","messageId":"4f9df714da95a10a5d89225b3ceb8ca42e61ac93.1712209149.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"[PATCH v2 11/11] reftable/block: reuse compressed array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T05:48:50Z","receivedAt":"2024-04-04T05:48:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Similar to the preceding commit, let's reuse the `compressed` array that\nwe use to store compressed data in. This results in a small reduction in\nmemory allocations when writing many refs.\n\nBefore:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,620,528 allocs, 22,620,377 frees, 1,245,549,984 bytes allocated\n\nAfter:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,618,257 allocs, 22,618,106 frees, 1,236,351,528 bytes allocated\n\nSo while the reduction in allocations isn't really all that big, it's a\nlow hanging fruit and thus there isn't much of a reason not to pick it.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 14 +++++---------\n reftable/block.h | 3 +++\n 2 files changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 9129305515..f190c05520 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -150,7 +150,6 @@ int block_writer_finish(struct block_writer *w)\n \tif (block_writer_type(w) == BLOCK_TYPE_LOG) {\n \t\tint block_header_skip = 4 + w->header_off;\n \t\tuLongf src_len = w->next - block_header_skip, compressed_len;\n-\t\tunsigned char *compressed;\n \t\tint ret;\n \n \t\tret = deflateReset(w->zstream);\n@@ -163,9 +162,9 @@ int block_writer_finish(struct block_writer *w)\n \t\t * is guaranteed to return `Z_STREAM_END`.\n \t\t */\n \t\tcompressed_len = deflateBound(w->zstream, src_len);\n-\t\tREFTABLE_ALLOC_ARRAY(compressed, compressed_len);\n+\t\tREFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);\n \n-\t\tw->zstream->next_out = compressed;\n+\t\tw->zstream->next_out = w->compressed;\n \t\tw->zstream->avail_out = compressed_len;\n \t\tw->zstream->next_in = w->buf + block_header_skip;\n \t\tw->zstream->avail_in = src_len;\n@@ -177,21 +176,17 @@ int block_writer_finish(struct block_writer *w)\n \t\t * guaranteed to succeed according to the zlib documentation.\n \t\t */\n \t\tret = deflate(w->zstream, Z_FINISH);\n-\t\tif (ret != Z_STREAM_END) {\n-\t\t\treftable_free(compressed);\n+\t\tif (ret != Z_STREAM_END)\n \t\t\treturn REFTABLE_ZLIB_ERROR;\n-\t\t}\n \n \t\t/*\n \t\t * Overwrite the uncompressed data we have already written and\n \t\t * adjust the `next` pointer to point right after the\n \t\t * compressed data.\n \t\t */\n-\t\tmemcpy(w->buf + block_header_skip, compressed,\n+\t\tmemcpy(w->buf + block_header_skip, w->compressed,\n \t\t w->zstream->total_out);\n \t\tw->next = w->zstream->total_out + block_header_skip;\n-\n-\t\treftable_free(compressed);\n \t}\n \n \treturn w->next;\n@@ -450,6 +445,7 @@ void block_writer_release(struct block_writer *bw)\n \tdeflateEnd(bw->zstream);\n \tFREE_AND_NULL(bw->zstream);\n \tFREE_AND_NULL(bw->restarts);\n+\tFREE_AND_NULL(bw->compressed);\n \tstrbuf_release(&bw->last_key);\n \t/* the block is not owned. */\n }\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 1375957fc8..657498014c 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -19,6 +19,9 @@ license that can be found in the LICENSE file or at\n */\n struct block_writer {\n \tz_stream *zstream;\n+\tunsigned char *compressed;\n+\tsize_t compressed_cap;\n+\n \tuint8_t *buf;\n \tuint32_t block_size;\n \n-- \n2.44.GIT\n\n"},{"id":"492219","messageId":"CAOw_e7YeqEK4O=KWowMYGtRVMLwL3y6bWw2LRfC9TqJz06Esyg@mail.gmail.com","threadId":"61255","inReplyTo":"4877ab39212867e91058c60f99fe0dc2a592d583.1712209149.git.ps@pks.im","subject":"Re: [PATCH v2 06/11] reftable/writer: refactorings for `writer_add_record()`","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2024-04-04T06:58:08Z","receivedAt":"2024-04-04T06:58:20Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt wrote:\n> + /*\n> + * Try to add the record to the writer again. If this still fails then\n> + * the record does not fit into the block size.\n> + *\n> + * TODO: it would be great to have `block_writer_add()` return proper\n> + * error codes so that we don't have to second-guess the failure\n> + * mode here.\n> + */\n\nThe Go code returns a (size, boolean) tuple for the write routines\nhere, but that does not really work in the Git C style.\n\nIf you make the routines return error codes it suggests that the\nin-memory write can fail for other reasons beyond \"does not fit\". Not\nsure if that is really an improvement.\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"492220","messageId":"CAOw_e7aBPF1vPvF7iYXCM5VBQu-Nw00dO2pRC_6DU3PtdDUsbg@mail.gmail.com","threadId":"61255","inReplyTo":"41db7414e17201f85b476af5e0183e72de450310.1712209149.git.ps@pks.im","subject":"Re: [PATCH v2 08/11] reftable/writer: unify releasing memory","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2024-04-04T07:08:46Z","receivedAt":"2024-04-04T07:08:58Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt wrote:\n> There are two code paths which release memory of the reftable writer:\n>\n> - `reftable_writer_close()` releases internal state after it has\n> written data.\n>\n> - `reftable_writer_free()` releases the block that was written to and\n> the writer itself.\n\nThe bifurcation is there so you can read the stats after closing the\nwriter. The new method makes it harder to misuse, but now you have two\nways to end a writer. Suggestion: drop reftable_writer_{free,close}\nfrom reftable-writer.h (rename to remove the reftable_ prefix because\nthey are no longer considered public) and find another way to read out\nthe stats. Either pass an optional reftable_writer_stats into the\nconstruction of the writer, return the stats from the close function,\nor drop stats altogether. IIRC They are only used in the unit tests.\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"492221","messageId":"CAOw_e7ZFJVwV-vCP65kaT5jrvHeigWRyfsC0vfnk3B-S_dXz2A@mail.gmail.com","threadId":"61255","inReplyTo":"cover.1712209149.git.ps@pks.im","subject":"Re: [PATCH v2 00/11] reftable: optimize write performance","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2024-04-04T07:09:53Z","receivedAt":"2024-04-04T07:10:05Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt wrote:\n>\n> Hi,\n>\n> this is the second version of my patch series that aims to optimize\n> write performance in the reftable backend. I've made the following\n> changes compared to v2:\n\nLooks OK overall; I had a cursory glance.\n\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"492223","messageId":"Zg5XhYq802lG4AgU@tanuki","threadId":"61255","inReplyTo":"CAOw_e7YeqEK4O=KWowMYGtRVMLwL3y6bWw2LRfC9TqJz06Esyg@mail.gmail.com","subject":"Re: [PATCH v2 06/11] reftable/writer: refactorings for `writer_add_record()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T07:32:21Z","receivedAt":"2024-04-04T07:32:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Apr 04, 2024 at 08:58:08AM +0200, Han-Wen Nienhuys wrote:\n> On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt wrote:\n> > + /*\n> > + * Try to add the record to the writer again. If this still fails then\n> > + * the record does not fit into the block size.\n> > + *\n> > + * TODO: it would be great to have `block_writer_add()` return proper\n> > + * error codes so that we don't have to second-guess the failure\n> > + * mode here.\n> > + */\n> \n> The Go code returns a (size, boolean) tuple for the write routines\n> here, but that does not really work in the Git C style.\n> \n> If you make the routines return error codes it suggests that the\n> in-memory write can fail for other reasons beyond \"does not fit\". Not\n> sure if that is really an improvement.\n\nIn reality, `block_writer_add()` already can fail because of different\nreasons: it returns `REFTABLE_API_ERROR` if the passed-in record has an\nempty key. This shouldn't ever happen, but it demonstrates that this is\ncertainly an area which needs some further cleanups.\n\nPatrick\n"},{"id":"492224","messageId":"Zg5Xorz75NMRqONI@tanuki","threadId":"61255","inReplyTo":"CAOw_e7aBPF1vPvF7iYXCM5VBQu-Nw00dO2pRC_6DU3PtdDUsbg@mail.gmail.com","subject":"Re: [PATCH v2 08/11] reftable/writer: unify releasing memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T07:32:50Z","receivedAt":"2024-04-04T07:32:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Apr 04, 2024 at 09:08:46AM +0200, Han-Wen Nienhuys wrote:\n> On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt wrote:\n> > There are two code paths which release memory of the reftable writer:\n> >\n> > - `reftable_writer_close()` releases internal state after it has\n> > written data.\n> >\n> > - `reftable_writer_free()` releases the block that was written to and\n> > the writer itself.\n> \n> The bifurcation is there so you can read the stats after closing the\n> writer. The new method makes it harder to misuse, but now you have two\n> ways to end a writer. Suggestion: drop reftable_writer_{free,close}\n> from reftable-writer.h (rename to remove the reftable_ prefix because\n> they are no longer considered public) and find another way to read out\n> the stats. Either pass an optional reftable_writer_stats into the\n> construction of the writer, return the stats from the close function,\n> or drop stats altogether. IIRC They are only used in the unit tests.\n\nBut even with these refactorings the stats remain intact after calling\n`reftable_writer_close()` or `reftable_writer_release()`, right? So it\nbasically continues to work as expected.\n\nIt might not be the cleanest way to handle this, but I think this patch\nis an improvement over the previous state because we plug a memory leak\nand deduplicate the cleanup logic. So I would suggest to defer your\nproposed refactorings to a later point, if you're okay with that.\n\nThanks!\n\nPatrick\n"},{"id":"492225","messageId":"Zg5XqI-MCiGsux8o@tanuki","threadId":"61255","inReplyTo":"CAOw_e7ZFJVwV-vCP65kaT5jrvHeigWRyfsC0vfnk3B-S_dXz2A@mail.gmail.com","subject":"Re: [PATCH v2 00/11] reftable: optimize write performance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T07:32:56Z","receivedAt":"2024-04-04T07:33:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Apr 04, 2024 at 09:09:53AM +0200, Han-Wen Nienhuys wrote:\n> On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt wrote:\n> >\n> > Hi,\n> >\n> > this is the second version of my patch series that aims to optimize\n> > write performance in the reftable backend. I've made the following\n> > changes compared to v2:\n> \n> Looks OK overall; I had a cursory glance.\n\nThanks!\n\nPatrick\n"},{"id":"492227","messageId":"CAOw_e7a9BnEh2OatwaGoSyVK46Wv2-sVkArbtXHLPt79b6g2qQ@mail.gmail.com","threadId":"61255","inReplyTo":"Zg5Xorz75NMRqONI@tanuki","subject":"Re: [PATCH v2 08/11] reftable/writer: unify releasing memory","fromName":"Han-Wen Nienhuys","fromEmail":"hanwenn@gmail.com","sentAt":"2024-04-04T09:00:46Z","receivedAt":"2024-04-04T09:00:58Z","isPatch":true,"sender":{"key":"hanwenn@gmail.com","avatar":"https://gravatar.com/avatar/058832acb8d613baeb6ce9d21b009d9772424a309b9a521330423daef909a27a?d=mp&s=160"},"body":"On Thu, Apr 4, 2024 at 9:32 AM Patrick Steinhardt wrote:\n>\n> On Thu, Apr 04, 2024 at 09:08:46AM +0200, Han-Wen Nienhuys wrote:\n> > On Thu, Apr 4, 2024 at 7:48 AM Patrick Steinhardt wrote:\n> > > There are two code paths which release memory of the reftable writer:\n> > >\n> > > - `reftable_writer_close()` releases internal state after it has\n> > > written data.\n> > >\n> > > - `reftable_writer_free()` releases the block that was written to and\n> > > the writer itself.\n> >\n> > The bifurcation is there so you can read the stats after closing the\n> > writer. The new method makes it harder to misuse, but now you have two\n> > ways to end a writer. Suggestion: drop reftable_writer_{free,close}\n> > from reftable-writer.h (rename to remove the reftable_ prefix because\n> > they are no longer considered public) and find another way to read out\n> > the stats. Either pass an optional reftable_writer_stats into the\n> > construction of the writer, return the stats from the close function,\n> > or drop stats altogether. IIRC They are only used in the unit tests.\n>\n> But even with these refactorings the stats remain intact after calling\n> `reftable_writer_close()` or `reftable_writer_release()`, right? So it\n> basically continues to work as expected.\n\nRight - I misinterpreted your change.\n\n> It might not be the cleanest way to handle this, but I think this patch\n> is an improvement over the previous state because we plug a memory leak\n> and deduplicate the cleanup logic. So I would suggest to defer your\n> proposed refactorings to a later point, if you're okay with that.\n\nyes. Please add reftable_writer_release to reftable-writer.h for\nconsistency, though. Or remove the reftable_ prefix.\n\n-- \nHan-Wen Nienhuys - hanwenn@gmail.com - http://www.xs4all.nl/~hanwen\n"},{"id":"492233","messageId":"Zg6SVcGC8kSGSYh-@tanuki","threadId":"61255","inReplyTo":"CAOw_e7a9BnEh2OatwaGoSyVK46Wv2-sVkArbtXHLPt79b6g2qQ@mail.gmail.com","subject":"Re: [PATCH v2 08/11] reftable/writer: unify releasing memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-04T11:43:17Z","receivedAt":"2024-04-04T11:43:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Apr 04, 2024 at 11:00:46AM +0200, Han-Wen Nienhuys wrote:\n> On Thu, Apr 4, 2024 at 9:32 AM Patrick Steinhardt wrote:\n> > On Thu, Apr 04, 2024 at 09:08:46AM +0200, Han-Wen Nienhuys wrote:\n[snip]\n> > It might not be the cleanest way to handle this, but I think this patch\n> > is an improvement over the previous state because we plug a memory leak\n> > and deduplicate the cleanup logic. So I would suggest to defer your\n> > proposed refactorings to a later point, if you're okay with that.\n> \n> yes. Please add reftable_writer_release to reftable-writer.h for\n> consistency, though. Or remove the reftable_ prefix.\n\nI've dropped the `reftable_` prefix locally. Will wait a bit for\nadditional reviews though before sending out v3.\n\nPatrick\n"},{"id":"492524","messageId":"cover.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712078736.git.ps@pks.im","subject":"[PATCH v3 00/11] reftable: optimize write performance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:23:47Z","receivedAt":"2024-04-08T12:23:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the first version of my patch series that aims to optimize write\nperformance with the reftable backend.\n\nChanges compared to v2:\n\n - The series now deepends on ps/reftable-binsearch-update at\n d51d8cc368 (reftable/block: avoid decoding keys when searching\n restart points, 2024-04-03). This is to resolve a merge conflict\n with that other series which has landed in \"next\" already.\n\n - Dropped the \"reftable_\" prefix from newly introduced internal\n reftable functions.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (11):\n refs/reftable: fix D/F conflict error message on ref copy\n refs/reftable: perform explicit D/F check when writing symrefs\n refs/reftable: skip duplicate name checks\n reftable: remove name checks\n refs/reftable: don't recompute committer ident\n reftable/writer: refactorings for `writer_add_record()`\n reftable/writer: refactorings for `writer_flush_nonempty_block()`\n reftable/writer: unify releasing memory\n reftable/writer: reset `last_key` instead of releasing it\n reftable/block: reuse zstream when writing log blocks\n reftable/block: reuse compressed array\n\n Makefile | 2 -\n refs/reftable-backend.c | 75 ++++++++++----\n reftable/block.c | 80 ++++++++------\n reftable/block.h | 4 +\n reftable/error.c | 2 -\n reftable/refname.c | 206 -------------------------------------\n reftable/refname.h | 29 ------\n reftable/refname_test.c | 101 ------------------\n reftable/reftable-error.h | 3 -\n reftable/reftable-tests.h | 1 -\n reftable/reftable-writer.h | 4 -\n reftable/stack.c | 67 +-----------\n reftable/stack_test.c | 39 -------\n reftable/writer.c | 137 +++++++++++++++---------\n t/helper/test-reftable.c | 1 -\n t/t0610-reftable-basics.sh | 35 ++++++-\n 16 files changed, 230 insertions(+), 556 deletions(-)\n delete mode 100644 reftable/refname.c\n delete mode 100644 reftable/refname.h\n delete mode 100644 reftable/refname_test.c\n\nRange-diff against v2:\n 1: 926e802395 = 1: bb735c389a refs/reftable: fix D/F conflict error message on ref copy\n 2: 6190171906 = 2: fe3f00d85a refs/reftable: perform explicit D/F check when writing symrefs\n 3: 80008cc5e7 = 3: 763c6fdfcd refs/reftable: skip duplicate name checks\n 4: 3497a570b4 ! 4: 2a5f07627a reftable: remove name checks\n @@ reftable/refname.c (deleted)\n -#include \"refname.h\"\n -#include \"reftable-iterator.h\"\n -\n --struct find_arg {\n --\tchar **names;\n --\tconst char *want;\n +-struct refname_needle_lesseq_args {\n +-\tchar **haystack;\n +-\tconst char *needle;\n -};\n -\n --static int find_name(size_t k, void *arg)\n +-static int refname_needle_lesseq(size_t k, void *_args)\n -{\n --\tstruct find_arg *f_arg = arg;\n --\treturn strcmp(f_arg->names[k], f_arg->want) >= 0;\n +-\tstruct refname_needle_lesseq_args *args = _args;\n +-\treturn strcmp(args->needle, args->haystack[k]) <= 0;\n -}\n -\n -static int modification_has_ref(struct modification *mod, const char *name)\n @@ reftable/refname.c (deleted)\n -\tint err = 0;\n -\n -\tif (mod->add_len > 0) {\n --\t\tstruct find_arg arg = {\n --\t\t\t.names = mod->add,\n --\t\t\t.want = name,\n +-\t\tstruct refname_needle_lesseq_args args = {\n +-\t\t\t.haystack = mod->add,\n +-\t\t\t.needle = name,\n -\t\t};\n --\t\tint idx = binsearch(mod->add_len, find_name, &arg);\n --\t\tif (idx < mod->add_len && !strcmp(mod->add[idx], name)) {\n +-\t\tsize_t idx = binsearch(mod->add_len, refname_needle_lesseq, &args);\n +-\t\tif (idx < mod->add_len && !strcmp(mod->add[idx], name))\n -\t\t\treturn 0;\n --\t\t}\n -\t}\n -\n -\tif (mod->del_len > 0) {\n --\t\tstruct find_arg arg = {\n --\t\t\t.names = mod->del,\n --\t\t\t.want = name,\n +-\t\tstruct refname_needle_lesseq_args args = {\n +-\t\t\t.haystack = mod->del,\n +-\t\t\t.needle = name,\n -\t\t};\n --\t\tint idx = binsearch(mod->del_len, find_name, &arg);\n --\t\tif (idx < mod->del_len && !strcmp(mod->del[idx], name)) {\n +-\t\tsize_t idx = binsearch(mod->del_len, refname_needle_lesseq, &args);\n +-\t\tif (idx < mod->del_len && !strcmp(mod->del[idx], name))\n -\t\t\treturn 1;\n --\t\t}\n -\t}\n -\n -\terr = reftable_table_read_ref(&mod->tab, name, &ref);\n @@ reftable/refname.c (deleted)\n -\tint err = 0;\n -\n -\tif (mod->add_len > 0) {\n --\t\tstruct find_arg arg = {\n --\t\t\t.names = mod->add,\n --\t\t\t.want = prefix,\n +-\t\tstruct refname_needle_lesseq_args args = {\n +-\t\t\t.haystack = mod->add,\n +-\t\t\t.needle = prefix,\n -\t\t};\n --\t\tint idx = binsearch(mod->add_len, find_name, &arg);\n +-\t\tsize_t idx = binsearch(mod->add_len, refname_needle_lesseq, &args);\n -\t\tif (idx < mod->add_len &&\n -\t\t !strncmp(prefix, mod->add[idx], strlen(prefix)))\n -\t\t\tgoto done;\n @@ reftable/refname.c (deleted)\n -\t\t\tgoto done;\n -\n -\t\tif (mod->del_len > 0) {\n --\t\t\tstruct find_arg arg = {\n --\t\t\t\t.names = mod->del,\n --\t\t\t\t.want = ref.refname,\n +-\t\t\tstruct refname_needle_lesseq_args args = {\n +-\t\t\t\t.haystack = mod->del,\n +-\t\t\t\t.needle = ref.refname,\n -\t\t\t};\n --\t\t\tint idx = binsearch(mod->del_len, find_name, &arg);\n +-\t\t\tsize_t idx = binsearch(mod->del_len, refname_needle_lesseq, &args);\n -\t\t\tif (idx < mod->del_len &&\n --\t\t\t !strcmp(ref.refname, mod->del[idx])) {\n +-\t\t\t !strcmp(ref.refname, mod->del[idx]))\n -\t\t\t\tcontinue;\n --\t\t\t}\n -\t\t}\n -\n -\t\tif (strncmp(ref.refname, prefix, strlen(prefix))) {\n 5: f892a3007b = 5: 1ca7d9b6cf refs/reftable: don't recompute committer ident\n 6: 4877ab3921 = 6: deabf82186 reftable/writer: refactorings for `writer_add_record()`\n 7: 8f1c5b4169 = 7: d47ad49d49 reftable/writer: refactorings for `writer_flush_nonempty_block()`\n 8: 41db7414e1 ! 8: 76d4a1f73b reftable/writer: unify releasing memory\n @@ reftable/writer.c: void reftable_writer_set_limits(struct reftable_writer *w, ui\n \tw->max_update_index = max;\n }\n \n -+static void reftable_writer_release(struct reftable_writer *w)\n ++static void writer_release(struct reftable_writer *w)\n +{\n +\tif (w) {\n +\t\treftable_free(w->block);\n @@ reftable/writer.c: void reftable_writer_set_limits(struct reftable_writer *w, ui\n -\tif (!w)\n -\t\treturn;\n -\treftable_free(w->block);\n -+\treftable_writer_release(w);\n ++\twriter_release(w);\n \treftable_free(w);\n }\n \n @@ reftable/writer.c: int reftable_writer_close(struct reftable_writer *w)\n -\tblock_writer_release(&w->block_writer_data);\n -\twriter_clear_index(w);\n -\tstrbuf_release(&w->last_key);\n -+\treftable_writer_release(w);\n ++\twriter_release(w);\n \treturn err;\n }\n \n 9: e5c7dbe417 = 9: 722ab0ee28 reftable/writer: reset `last_key` instead of releasing it\n10: 26f422703f = 10: 962a96003b reftable/block: reuse zstream when writing log blocks\n11: 4f9df714da = 11: 323892841a reftable/block: reuse compressed array\n\nbase-commit: 7774cfed6261ce2900c84e55906da708c711d601\n-- \n2.44.GIT\n\n"},{"id":"492525","messageId":"bb735c389a234b5b90524212f0123d7404fe3d29.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 01/11] refs/reftable: fix D/F conflict error message on ref copy","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:23:52Z","receivedAt":"2024-04-08T12:23:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `write_copy_table()` function is shared between the reftable\nimplementations for renaming and copying refs. The only difference\nbetween those two cases is that the rename will also delete the old\nreference, whereas copying won't.\n\nThis has resulted in a bug though where we don't properly verify refname\navailability. When calling `refs_verify_refname_available()`, we always\nadd the old ref name to the list of refs to be skipped when computing\navailability, which indicates that the name would be available even if\nit already exists at the current point in time. This is only the right\nthing to do for renames though, not for copies.\n\nThe consequence of this bug is quite harmless because the reftable\nbackend has its own checks for D/F conflicts further down in the call\nstack, and thus we refuse the update regardless of the bug. But all the\nuser gets in this case is an uninformative message that copying the ref\nhas failed, without any further details.\n\nFix the bug and only add the old name to the skip-list in case we rename\nthe ref. Consequently, this error case will now be handled by\n`refs_verify_refname_available()`, which knows to provide a proper error\nmessage.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 3 ++-\n t/t0610-reftable-basics.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex e206d5a073..0358da14db 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1351,7 +1351,8 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \t/*\n \t * Verify that the new refname is available.\n \t */\n-\tstring_list_insert(&skip, arg->oldname);\n+\tif (arg->delete_old)\n+\t\tstring_list_insert(&skip, arg->oldname);\n \tret = refs_verify_refname_available(&arg->refs->base, arg->newname,\n \t\t\t\t\t NULL, &skip, &errbuf);\n \tif (ret < 0) {\ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 686781192e..055231a707 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -730,6 +730,39 @@ test_expect_success 'reflog: updates via HEAD update HEAD reflog' '\n \t)\n '\n \n+test_expect_success 'branch: copying branch with D/F conflict' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit branch branch &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${SQ}refs/heads/branch${SQ} exists; cannot create ${SQ}refs/heads/branch/moved${SQ}\n+\t\tfatal: branch copy failed\n+\t\tEOF\n+\t\ttest_must_fail git branch -c branch branch/moved 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n+test_expect_success 'branch: moving branch with D/F conflict' '\n+\ttest_when_finished \"rm -rf repo\" &&\n+\tgit init repo &&\n+\t(\n+\t\tcd repo &&\n+\t\ttest_commit A &&\n+\t\tgit branch branch &&\n+\t\tgit branch conflict &&\n+\t\tcat >expect <<-EOF &&\n+\t\terror: ${SQ}refs/heads/conflict${SQ} exists; cannot create ${SQ}refs/heads/conflict/moved${SQ}\n+\t\tfatal: branch rename failed\n+\t\tEOF\n+\t\ttest_must_fail git branch -m branch conflict/moved 2>err &&\n+\t\ttest_cmp expect err\n+\t)\n+'\n+\n test_expect_success 'worktree: adding worktree creates separate stack' '\n \ttest_when_finished \"rm -rf repo worktree\" &&\n \tgit init repo &&\n-- \n2.44.GIT\n\n"},{"id":"492526","messageId":"fe3f00d85a15da3b2b81dde27a016d1edcb72c2b.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 02/11] refs/reftable: perform explicit D/F check when writing symrefs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:23:56Z","receivedAt":"2024-04-08T12:24:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We already perform explicit D/F checks in all reftable callbacks which\nwrite refs, except when writing symrefs. For one this leads to an error\nmessage which isn't perfectly actionable because we only tell the user\nthat there was a D/F conflict, but not which refs conflicted with each\nother. But second, once all ref updating callbacks explicitly check for\nD/F conflicts, we can disable the D/F checks in the reftable library\nitself and thus avoid some duplicated efforts.\n\nRefactor the code that writes symref tables to explicitly call into\n`refs_verify_refname_available()` when writing symrefs.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 20 +++++++++++++++++---\n t/t0610-reftable-basics.sh | 2 +-\n 2 files changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 0358da14db..8a54b0d8b2 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1217,6 +1217,7 @@ static int reftable_be_pack_refs(struct ref_store *ref_store,\n struct write_create_symref_arg {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_stack *stack;\n+\tstruct strbuf *err;\n \tconst char *refname;\n \tconst char *target;\n \tconst char *logmsg;\n@@ -1239,6 +1240,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \n \treftable_writer_set_limits(writer, ts, ts);\n \n+\tret = refs_verify_refname_available(&create->refs->base, create->refname,\n+\t\t\t\t\t NULL, NULL, create->err);\n+\tif (ret < 0)\n+\t\treturn ret;\n+\n \tret = reftable_writer_add_ref(writer, &ref);\n \tif (ret)\n \t\treturn ret;\n@@ -1280,12 +1286,14 @@ static int reftable_be_create_symref(struct ref_store *ref_store,\n \tstruct reftable_ref_store *refs =\n \t\treftable_be_downcast(ref_store, REF_STORE_WRITE, \"create_symref\");\n \tstruct reftable_stack *stack = stack_for(refs, refname, &refname);\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct write_create_symref_arg arg = {\n \t\t.refs = refs,\n \t\t.stack = stack,\n \t\t.refname = refname,\n \t\t.target = target,\n \t\t.logmsg = logmsg,\n+\t\t.err = &err,\n \t};\n \tint ret;\n \n@@ -1301,9 +1309,15 @@ static int reftable_be_create_symref(struct ref_store *ref_store,\n \n done:\n \tassert(ret != REFTABLE_API_ERROR);\n-\tif (ret)\n-\t\terror(\"unable to write symref for %s: %s\", refname,\n-\t\t reftable_error_str(ret));\n+\tif (ret) {\n+\t\tif (err.len)\n+\t\t\terror(\"%s\", err.buf);\n+\t\telse\n+\t\t\terror(\"unable to write symref for %s: %s\", refname,\n+\t\t\t reftable_error_str(ret));\n+\t}\n+\n+\tstrbuf_release(&err);\n \treturn ret;\n }\n \ndiff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh\nindex 055231a707..12b0004781 100755\n--- a/t/t0610-reftable-basics.sh\n+++ b/t/t0610-reftable-basics.sh\n@@ -255,7 +255,7 @@ test_expect_success 'ref transaction: creating symbolic ref fails with F/D confl\n \tgit init repo &&\n \ttest_commit -C repo A &&\n \tcat >expect <<-EOF &&\n-\terror: unable to write symref for refs/heads: file/directory conflict\n+\terror: ${SQ}refs/heads/main${SQ} exists; cannot create ${SQ}refs/heads${SQ}\n \tEOF\n \ttest_must_fail git -C repo symbolic-ref refs/heads refs/heads/foo 2>err &&\n \ttest_cmp expect err\n-- \n2.44.GIT\n\n"},{"id":"492527","messageId":"763c6fdfcd93651dac46de9c308c66f10d73d3d2.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 03/11] refs/reftable: skip duplicate name checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:01Z","receivedAt":"2024-04-08T12:24:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"All the callback functions which write refs in the reftable backend\nperform D/F conflict checks via `refs_verify_refname_available()`. But\nin reality we perform these D/F conflict checks a second time in the\nreftable library via `stack_check_addition()`.\n\nInterestingly, the code in the reftable library is inferior compared to\nthe generic function:\n\n - It is slower than `refs_verify_refname_available()`, even though\n this can probably be optimized.\n\n - It does not provide a proper error message to the caller, and thus\n all the user would see is a generic \"file/directory conflict\"\n message.\n\nDisable the D/F conflict checks in the reftable library by setting the\n`skip_name_check` write option. This results in a non-negligible speedup\nwhen writing many refs. The following benchmark writes 100k refs in a\nsingle transaction:\n\n Benchmark 1: update-ref: create many refs (HEAD~)\n Time (mean ± σ): 3.241 s ± 0.040 s [User: 1.854 s, System: 1.381 s]\n Range (min … max): 3.185 s … 3.454 s 100 runs\n\n Benchmark 2: update-ref: create many refs (HEAD)\n Time (mean ± σ): 2.878 s ± 0.024 s [User: 1.506 s, System: 1.367 s]\n Range (min … max): 2.838 s … 2.960 s 100 runs\n\n Summary\n update-ref: create many refs (HEAD~) ran\n 1.13 ± 0.02 times faster than update-ref: create many refs (HEAD)\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 8a54b0d8b2..7515dd3019 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -247,6 +247,11 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.block_size = 4096;\n \trefs->write_options.hash_id = repo->hash_algo->format_id;\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n+\t/*\n+\t * We verify names via `refs_verify_refname_available()`, so there is\n+\t * no need to do the same checks in the reftable library again.\n+\t */\n+\trefs->write_options.skip_name_check = 1;\n \n \t/*\n \t * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.\n-- \n2.44.GIT\n\n"},{"id":"492528","messageId":"2a5f07627a445f214940bf093487af9492e27684.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 04/11] reftable: remove name checks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:06Z","receivedAt":"2024-04-08T12:24:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In the preceding commit we have disabled name checks in the \"reftable\"\nbackend. These checks were responsible for verifying multiple things\nwhen writing records to the reftable stack:\n\n - Detecting file/directory conflicts. Starting with the preceding\n commits this is now handled by the reftable backend itself via\n `refs_verify_refname_available()`.\n\n - Validating refnames. This is handled by `check_refname_format()` in\n the generic ref transacton layer.\n\nThe code in the reftable library is thus not used anymore and likely to\nbitrot over time. Remove it.\n\nSigned-off-by: Patrick Steinhardt \n---\n Makefile | 2 -\n refs/reftable-backend.c | 5 -\n reftable/error.c | 2 -\n reftable/refname.c | 206 -------------------------------------\n reftable/refname.h | 29 ------\n reftable/refname_test.c | 101 ------------------\n reftable/reftable-error.h | 3 -\n reftable/reftable-tests.h | 1 -\n reftable/reftable-writer.h | 4 -\n reftable/stack.c | 67 +-----------\n reftable/stack_test.c | 39 -------\n t/helper/test-reftable.c | 1 -\n 12 files changed, 1 insertion(+), 459 deletions(-)\n delete mode 100644 reftable/refname.c\n delete mode 100644 reftable/refname.h\n delete mode 100644 reftable/refname_test.c\n\ndiff --git a/Makefile b/Makefile\nindex c43c1bd1a0..05e3d37581 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2655,7 +2655,6 @@ REFTABLE_OBJS += reftable/merged.o\n REFTABLE_OBJS += reftable/pq.o\n REFTABLE_OBJS += reftable/reader.o\n REFTABLE_OBJS += reftable/record.o\n-REFTABLE_OBJS += reftable/refname.o\n REFTABLE_OBJS += reftable/generic.o\n REFTABLE_OBJS += reftable/stack.o\n REFTABLE_OBJS += reftable/tree.o\n@@ -2668,7 +2667,6 @@ REFTABLE_TEST_OBJS += reftable/merged_test.o\n REFTABLE_TEST_OBJS += reftable/pq_test.o\n REFTABLE_TEST_OBJS += reftable/record_test.o\n REFTABLE_TEST_OBJS += reftable/readwrite_test.o\n-REFTABLE_TEST_OBJS += reftable/refname_test.o\n REFTABLE_TEST_OBJS += reftable/stack_test.o\n REFTABLE_TEST_OBJS += reftable/test_framework.o\n REFTABLE_TEST_OBJS += reftable/tree_test.o\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 7515dd3019..8a54b0d8b2 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -247,11 +247,6 @@ static struct ref_store *reftable_be_init(struct repository *repo,\n \trefs->write_options.block_size = 4096;\n \trefs->write_options.hash_id = repo->hash_algo->format_id;\n \trefs->write_options.default_permissions = calc_shared_perm(0666 & ~mask);\n-\t/*\n-\t * We verify names via `refs_verify_refname_available()`, so there is\n-\t * no need to do the same checks in the reftable library again.\n-\t */\n-\trefs->write_options.skip_name_check = 1;\n \n \t/*\n \t * Set up the main reftable stack that is hosted in GIT_COMMON_DIR.\ndiff --git a/reftable/error.c b/reftable/error.c\nindex 0d1766735e..169f89d2f1 100644\n--- a/reftable/error.c\n+++ b/reftable/error.c\n@@ -27,8 +27,6 @@ const char *reftable_error_str(int err)\n \t\treturn \"misuse of the reftable API\";\n \tcase REFTABLE_ZLIB_ERROR:\n \t\treturn \"zlib failure\";\n-\tcase REFTABLE_NAME_CONFLICT:\n-\t\treturn \"file/directory conflict\";\n \tcase REFTABLE_EMPTY_TABLE_ERROR:\n \t\treturn \"wrote empty table\";\n \tcase REFTABLE_REFNAME_ERROR:\ndiff --git a/reftable/refname.c b/reftable/refname.c\ndeleted file mode 100644\nindex bbfde15754..0000000000\n--- a/reftable/refname.c\n+++ /dev/null\n@@ -1,206 +0,0 @@\n-/*\n- Copyright 2020 Google LLC\n-\n- Use of this source code is governed by a BSD-style\n- license that can be found in the LICENSE file or at\n- https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#include \"system.h\"\n-#include \"reftable-error.h\"\n-#include \"basics.h\"\n-#include \"refname.h\"\n-#include \"reftable-iterator.h\"\n-\n-struct refname_needle_lesseq_args {\n-\tchar **haystack;\n-\tconst char *needle;\n-};\n-\n-static int refname_needle_lesseq(size_t k, void *_args)\n-{\n-\tstruct refname_needle_lesseq_args *args = _args;\n-\treturn strcmp(args->needle, args->haystack[k]) <= 0;\n-}\n-\n-static int modification_has_ref(struct modification *mod, const char *name)\n-{\n-\tstruct reftable_ref_record ref = { NULL };\n-\tint err = 0;\n-\n-\tif (mod->add_len > 0) {\n-\t\tstruct refname_needle_lesseq_args args = {\n-\t\t\t.haystack = mod->add,\n-\t\t\t.needle = name,\n-\t\t};\n-\t\tsize_t idx = binsearch(mod->add_len, refname_needle_lesseq, &args);\n-\t\tif (idx < mod->add_len && !strcmp(mod->add[idx], name))\n-\t\t\treturn 0;\n-\t}\n-\n-\tif (mod->del_len > 0) {\n-\t\tstruct refname_needle_lesseq_args args = {\n-\t\t\t.haystack = mod->del,\n-\t\t\t.needle = name,\n-\t\t};\n-\t\tsize_t idx = binsearch(mod->del_len, refname_needle_lesseq, &args);\n-\t\tif (idx < mod->del_len && !strcmp(mod->del[idx], name))\n-\t\t\treturn 1;\n-\t}\n-\n-\terr = reftable_table_read_ref(&mod->tab, name, &ref);\n-\treftable_ref_record_release(&ref);\n-\treturn err;\n-}\n-\n-static void modification_release(struct modification *mod)\n-{\n-\t/* don't delete the strings themselves; they're owned by ref records.\n-\t */\n-\tFREE_AND_NULL(mod->add);\n-\tFREE_AND_NULL(mod->del);\n-\tmod->add_len = 0;\n-\tmod->del_len = 0;\n-}\n-\n-static int modification_has_ref_with_prefix(struct modification *mod,\n-\t\t\t\t\t const char *prefix)\n-{\n-\tstruct reftable_iterator it = { NULL };\n-\tstruct reftable_ref_record ref = { NULL };\n-\tint err = 0;\n-\n-\tif (mod->add_len > 0) {\n-\t\tstruct refname_needle_lesseq_args args = {\n-\t\t\t.haystack = mod->add,\n-\t\t\t.needle = prefix,\n-\t\t};\n-\t\tsize_t idx = binsearch(mod->add_len, refname_needle_lesseq, &args);\n-\t\tif (idx < mod->add_len &&\n-\t\t !strncmp(prefix, mod->add[idx], strlen(prefix)))\n-\t\t\tgoto done;\n-\t}\n-\terr = reftable_table_seek_ref(&mod->tab, &it, prefix);\n-\tif (err)\n-\t\tgoto done;\n-\n-\twhile (1) {\n-\t\terr = reftable_iterator_next_ref(&it, &ref);\n-\t\tif (err)\n-\t\t\tgoto done;\n-\n-\t\tif (mod->del_len > 0) {\n-\t\t\tstruct refname_needle_lesseq_args args = {\n-\t\t\t\t.haystack = mod->del,\n-\t\t\t\t.needle = ref.refname,\n-\t\t\t};\n-\t\t\tsize_t idx = binsearch(mod->del_len, refname_needle_lesseq, &args);\n-\t\t\tif (idx < mod->del_len &&\n-\t\t\t !strcmp(ref.refname, mod->del[idx]))\n-\t\t\t\tcontinue;\n-\t\t}\n-\n-\t\tif (strncmp(ref.refname, prefix, strlen(prefix))) {\n-\t\t\terr = 1;\n-\t\t\tgoto done;\n-\t\t}\n-\t\terr = 0;\n-\t\tgoto done;\n-\t}\n-\n-done:\n-\treftable_ref_record_release(&ref);\n-\treftable_iterator_destroy(&it);\n-\treturn err;\n-}\n-\n-static int validate_refname(const char *name)\n-{\n-\twhile (1) {\n-\t\tchar *next = strchr(name, '/');\n-\t\tif (!*name) {\n-\t\t\treturn REFTABLE_REFNAME_ERROR;\n-\t\t}\n-\t\tif (!next) {\n-\t\t\treturn 0;\n-\t\t}\n-\t\tif (next - name == 0 || (next - name == 1 && *name == '.') ||\n-\t\t (next - name == 2 && name[0] == '.' && name[1] == '.'))\n-\t\t\treturn REFTABLE_REFNAME_ERROR;\n-\t\tname = next + 1;\n-\t}\n-\treturn 0;\n-}\n-\n-int validate_ref_record_addition(struct reftable_table tab,\n-\t\t\t\t struct reftable_ref_record *recs, size_t sz)\n-{\n-\tstruct modification mod = {\n-\t\t.tab = tab,\n-\t\t.add = reftable_calloc(sz, sizeof(*mod.add)),\n-\t\t.del = reftable_calloc(sz, sizeof(*mod.del)),\n-\t};\n-\tint i = 0;\n-\tint err = 0;\n-\tfor (; i < sz; i++) {\n-\t\tif (reftable_ref_record_is_deletion(&recs[i])) {\n-\t\t\tmod.del[mod.del_len++] = recs[i].refname;\n-\t\t} else {\n-\t\t\tmod.add[mod.add_len++] = recs[i].refname;\n-\t\t}\n-\t}\n-\n-\terr = modification_validate(&mod);\n-\tmodification_release(&mod);\n-\treturn err;\n-}\n-\n-static void strbuf_trim_component(struct strbuf *sl)\n-{\n-\twhile (sl->len > 0) {\n-\t\tint is_slash = (sl->buf[sl->len - 1] == '/');\n-\t\tstrbuf_setlen(sl, sl->len - 1);\n-\t\tif (is_slash)\n-\t\t\tbreak;\n-\t}\n-}\n-\n-int modification_validate(struct modification *mod)\n-{\n-\tstruct strbuf slashed = STRBUF_INIT;\n-\tint err = 0;\n-\tint i = 0;\n-\tfor (; i < mod->add_len; i++) {\n-\t\terr = validate_refname(mod->add[i]);\n-\t\tif (err)\n-\t\t\tgoto done;\n-\t\tstrbuf_reset(&slashed);\n-\t\tstrbuf_addstr(&slashed, mod->add[i]);\n-\t\tstrbuf_addstr(&slashed, \"/\");\n-\n-\t\terr = modification_has_ref_with_prefix(mod, slashed.buf);\n-\t\tif (err == 0) {\n-\t\t\terr = REFTABLE_NAME_CONFLICT;\n-\t\t\tgoto done;\n-\t\t}\n-\t\tif (err < 0)\n-\t\t\tgoto done;\n-\n-\t\tstrbuf_reset(&slashed);\n-\t\tstrbuf_addstr(&slashed, mod->add[i]);\n-\t\twhile (slashed.len) {\n-\t\t\tstrbuf_trim_component(&slashed);\n-\t\t\terr = modification_has_ref(mod, slashed.buf);\n-\t\t\tif (err == 0) {\n-\t\t\t\terr = REFTABLE_NAME_CONFLICT;\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\t\t\tif (err < 0)\n-\t\t\t\tgoto done;\n-\t\t}\n-\t}\n-\terr = 0;\n-done:\n-\tstrbuf_release(&slashed);\n-\treturn err;\n-}\ndiff --git a/reftable/refname.h b/reftable/refname.h\ndeleted file mode 100644\nindex a24b40fcb4..0000000000\n--- a/reftable/refname.h\n+++ /dev/null\n@@ -1,29 +0,0 @@\n-/*\n- Copyright 2020 Google LLC\n-\n- Use of this source code is governed by a BSD-style\n- license that can be found in the LICENSE file or at\n- https://developers.google.com/open-source/licenses/bsd\n-*/\n-#ifndef REFNAME_H\n-#define REFNAME_H\n-\n-#include \"reftable-record.h\"\n-#include \"reftable-generic.h\"\n-\n-struct modification {\n-\tstruct reftable_table tab;\n-\n-\tchar **add;\n-\tsize_t add_len;\n-\n-\tchar **del;\n-\tsize_t del_len;\n-};\n-\n-int validate_ref_record_addition(struct reftable_table tab,\n-\t\t\t\t struct reftable_ref_record *recs, size_t sz);\n-\n-int modification_validate(struct modification *mod);\n-\n-#endif\ndiff --git a/reftable/refname_test.c b/reftable/refname_test.c\ndeleted file mode 100644\nindex b9cc62554e..0000000000\n--- a/reftable/refname_test.c\n+++ /dev/null\n@@ -1,101 +0,0 @@\n-/*\n-Copyright 2020 Google LLC\n-\n-Use of this source code is governed by a BSD-style\n-license that can be found in the LICENSE file or at\n-https://developers.google.com/open-source/licenses/bsd\n-*/\n-\n-#include \"basics.h\"\n-#include \"block.h\"\n-#include \"blocksource.h\"\n-#include \"reader.h\"\n-#include \"record.h\"\n-#include \"refname.h\"\n-#include \"reftable-error.h\"\n-#include \"reftable-writer.h\"\n-#include \"system.h\"\n-\n-#include \"test_framework.h\"\n-#include \"reftable-tests.h\"\n-\n-struct testcase {\n-\tchar *add;\n-\tchar *del;\n-\tint error_code;\n-};\n-\n-static void test_conflict(void)\n-{\n-\tstruct reftable_write_options opts = { 0 };\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct reftable_writer *w =\n-\t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n-\tstruct reftable_ref_record rec = {\n-\t\t.refname = \"a/b\",\n-\t\t.value_type = REFTABLE_REF_SYMREF,\n-\t\t.value.symref = \"destination\", /* make sure it's not a symref.\n-\t\t\t\t\t\t*/\n-\t\t.update_index = 1,\n-\t};\n-\tint err;\n-\tint i;\n-\tstruct reftable_block_source source = { NULL };\n-\tstruct reftable_reader *rd = NULL;\n-\tstruct reftable_table tab = { NULL };\n-\tstruct testcase cases[] = {\n-\t\t{ \"a/b/c\", NULL, REFTABLE_NAME_CONFLICT },\n-\t\t{ \"b\", NULL, 0 },\n-\t\t{ \"a\", NULL, REFTABLE_NAME_CONFLICT },\n-\t\t{ \"a\", \"a/b\", 0 },\n-\n-\t\t{ \"p/\", NULL, REFTABLE_REFNAME_ERROR },\n-\t\t{ \"p//q\", NULL, REFTABLE_REFNAME_ERROR },\n-\t\t{ \"p/./q\", NULL, REFTABLE_REFNAME_ERROR },\n-\t\t{ \"p/../q\", NULL, REFTABLE_REFNAME_ERROR },\n-\n-\t\t{ \"a/b/c\", \"a/b\", 0 },\n-\t\t{ NULL, \"a//b\", 0 },\n-\t};\n-\treftable_writer_set_limits(w, 1, 1);\n-\n-\terr = reftable_writer_add_ref(w, &rec);\n-\tEXPECT_ERR(err);\n-\n-\terr = reftable_writer_close(w);\n-\tEXPECT_ERR(err);\n-\treftable_writer_free(w);\n-\n-\tblock_source_from_strbuf(&source, &buf);\n-\terr = reftable_new_reader(&rd, &source, \"filename\");\n-\tEXPECT_ERR(err);\n-\n-\treftable_table_from_reader(&tab, rd);\n-\n-\tfor (i = 0; i < ARRAY_SIZE(cases); i++) {\n-\t\tstruct modification mod = {\n-\t\t\t.tab = tab,\n-\t\t};\n-\n-\t\tif (cases[i].add) {\n-\t\t\tmod.add = &cases[i].add;\n-\t\t\tmod.add_len = 1;\n-\t\t}\n-\t\tif (cases[i].del) {\n-\t\t\tmod.del = &cases[i].del;\n-\t\t\tmod.del_len = 1;\n-\t\t}\n-\n-\t\terr = modification_validate(&mod);\n-\t\tEXPECT(err == cases[i].error_code);\n-\t}\n-\n-\treftable_reader_free(rd);\n-\tstrbuf_release(&buf);\n-}\n-\n-int refname_test_main(int argc, const char *argv[])\n-{\n-\tRUN_TEST(test_conflict);\n-\treturn 0;\n-}\ndiff --git a/reftable/reftable-error.h b/reftable/reftable-error.h\nindex 4c457aaaf8..3a5f5b92c6 100644\n--- a/reftable/reftable-error.h\n+++ b/reftable/reftable-error.h\n@@ -48,9 +48,6 @@ enum reftable_error {\n \t/* Wrote a table without blocks. */\n \tREFTABLE_EMPTY_TABLE_ERROR = -8,\n \n-\t/* Dir/file conflict. */\n-\tREFTABLE_NAME_CONFLICT = -9,\n-\n \t/* Invalid ref name. */\n \tREFTABLE_REFNAME_ERROR = -10,\n \ndiff --git a/reftable/reftable-tests.h b/reftable/reftable-tests.h\nindex 0019cbcfa4..114cc3d053 100644\n--- a/reftable/reftable-tests.h\n+++ b/reftable/reftable-tests.h\n@@ -14,7 +14,6 @@ int block_test_main(int argc, const char **argv);\n int merged_test_main(int argc, const char **argv);\n int pq_test_main(int argc, const char **argv);\n int record_test_main(int argc, const char **argv);\n-int refname_test_main(int argc, const char **argv);\n int readwrite_test_main(int argc, const char **argv);\n int stack_test_main(int argc, const char **argv);\n int tree_test_main(int argc, const char **argv);\ndiff --git a/reftable/reftable-writer.h b/reftable/reftable-writer.h\nindex 7c7cae5f99..3c119e2bbb 100644\n--- a/reftable/reftable-writer.h\n+++ b/reftable/reftable-writer.h\n@@ -38,10 +38,6 @@ struct reftable_write_options {\n \t/* Default mode for creating files. If unset, use 0666 (+umask) */\n \tunsigned int default_permissions;\n \n-\t/* boolean: do not check ref names for validity or dir/file conflicts.\n-\t */\n-\tunsigned skip_name_check : 1;\n-\n \t/* boolean: copy log messages exactly. If unset, check that the message\n \t * is a single line, and add '\\n' if missing.\n \t */\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex 1ecf1b9751..e264df5ced 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -12,8 +12,8 @@ license that can be found in the LICENSE file or at\n #include \"system.h\"\n #include \"merged.h\"\n #include \"reader.h\"\n-#include \"refname.h\"\n #include \"reftable-error.h\"\n+#include \"reftable-generic.h\"\n #include \"reftable-record.h\"\n #include \"reftable-merged.h\"\n #include \"writer.h\"\n@@ -27,8 +27,6 @@ static int stack_write_compact(struct reftable_stack *st,\n \t\t\t struct reftable_writer *wr,\n \t\t\t size_t first, size_t last,\n \t\t\t struct reftable_log_expiry_config *config);\n-static int stack_check_addition(struct reftable_stack *st,\n-\t\t\t\tconst char *new_tab_name);\n static void reftable_addition_close(struct reftable_addition *add);\n static int reftable_stack_reload_maybe_reuse(struct reftable_stack *st,\n \t\t\t\t\t int reuse_open);\n@@ -781,10 +779,6 @@ int reftable_addition_add(struct reftable_addition *add,\n \t\tgoto done;\n \t}\n \n-\terr = stack_check_addition(add->stack, get_tempfile_path(tab_file));\n-\tif (err < 0)\n-\t\tgoto done;\n-\n \tif (wr->min_update_index < add->next_update_index) {\n \t\terr = REFTABLE_API_ERROR;\n \t\tgoto done;\n@@ -1340,65 +1334,6 @@ int reftable_stack_read_log(struct reftable_stack *st, const char *refname,\n \treturn err;\n }\n \n-static int stack_check_addition(struct reftable_stack *st,\n-\t\t\t\tconst char *new_tab_name)\n-{\n-\tint err = 0;\n-\tstruct reftable_block_source src = { NULL };\n-\tstruct reftable_reader *rd = NULL;\n-\tstruct reftable_table tab = { NULL };\n-\tstruct reftable_ref_record *refs = NULL;\n-\tstruct reftable_iterator it = { NULL };\n-\tint cap = 0;\n-\tint len = 0;\n-\tint i = 0;\n-\n-\tif (st->config.skip_name_check)\n-\t\treturn 0;\n-\n-\terr = reftable_block_source_from_file(&src, new_tab_name);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_new_reader(&rd, &src, new_tab_name);\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\terr = reftable_reader_seek_ref(rd, &it, \"\");\n-\tif (err > 0) {\n-\t\terr = 0;\n-\t\tgoto done;\n-\t}\n-\tif (err < 0)\n-\t\tgoto done;\n-\n-\twhile (1) {\n-\t\tstruct reftable_ref_record ref = { NULL };\n-\t\terr = reftable_iterator_next_ref(&it, &ref);\n-\t\tif (err > 0)\n-\t\t\tbreak;\n-\t\tif (err < 0)\n-\t\t\tgoto done;\n-\n-\t\tREFTABLE_ALLOC_GROW(refs, len + 1, cap);\n-\t\trefs[len++] = ref;\n-\t}\n-\n-\treftable_table_from_merged_table(&tab, reftable_stack_merged_table(st));\n-\n-\terr = validate_ref_record_addition(tab, refs, len);\n-\n-done:\n-\tfor (i = 0; i < len; i++) {\n-\t\treftable_ref_record_release(&refs[i]);\n-\t}\n-\n-\tfree(refs);\n-\treftable_iterator_destroy(&it);\n-\treftable_reader_free(rd);\n-\treturn err;\n-}\n-\n static int is_table_name(const char *s)\n {\n \tconst char *dot = strrchr(s, '.');\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 0dc9a44648..b88097c3b6 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -353,44 +353,6 @@ static void test_reftable_stack_transaction_api_performs_auto_compaction(void)\n \tclear_dir(dir);\n }\n \n-static void test_reftable_stack_validate_refname(void)\n-{\n-\tstruct reftable_write_options cfg = { 0 };\n-\tstruct reftable_stack *st = NULL;\n-\tint err;\n-\tchar *dir = get_tmp_dir(__LINE__);\n-\n-\tint i;\n-\tstruct reftable_ref_record ref = {\n-\t\t.refname = \"a/b\",\n-\t\t.update_index = 1,\n-\t\t.value_type = REFTABLE_REF_SYMREF,\n-\t\t.value.symref = \"master\",\n-\t};\n-\tchar *additions[] = { \"a\", \"a/b/c\" };\n-\n-\terr = reftable_new_stack(&st, dir, cfg);\n-\tEXPECT_ERR(err);\n-\n-\terr = reftable_stack_add(st, &write_test_ref, &ref);\n-\tEXPECT_ERR(err);\n-\n-\tfor (i = 0; i < ARRAY_SIZE(additions); i++) {\n-\t\tstruct reftable_ref_record ref = {\n-\t\t\t.refname = additions[i],\n-\t\t\t.update_index = 1,\n-\t\t\t.value_type = REFTABLE_REF_SYMREF,\n-\t\t\t.value.symref = \"master\",\n-\t\t};\n-\n-\t\terr = reftable_stack_add(st, &write_test_ref, &ref);\n-\t\tEXPECT(err == REFTABLE_NAME_CONFLICT);\n-\t}\n-\n-\treftable_stack_destroy(st);\n-\tclear_dir(dir);\n-}\n-\n static int write_error(struct reftable_writer *wr, void *arg)\n {\n \treturn *((int *)arg);\n@@ -1097,7 +1059,6 @@ int stack_test_main(int argc, const char *argv[])\n \tRUN_TEST(test_reftable_stack_transaction_api_performs_auto_compaction);\n \tRUN_TEST(test_reftable_stack_update_index_check);\n \tRUN_TEST(test_reftable_stack_uptodate);\n-\tRUN_TEST(test_reftable_stack_validate_refname);\n \tRUN_TEST(test_sizes_to_segments);\n \tRUN_TEST(test_sizes_to_segments_all_equal);\n \tRUN_TEST(test_sizes_to_segments_empty);\ndiff --git a/t/helper/test-reftable.c b/t/helper/test-reftable.c\nindex 00237ef0d9..bae731669c 100644\n--- a/t/helper/test-reftable.c\n+++ b/t/helper/test-reftable.c\n@@ -13,7 +13,6 @@ int cmd__reftable(int argc, const char **argv)\n \treadwrite_test_main(argc, argv);\n \tmerged_test_main(argc, argv);\n \tstack_test_main(argc, argv);\n-\trefname_test_main(argc, argv);\n \treturn 0;\n }\n \n-- \n2.44.GIT\n\n"},{"id":"492529","messageId":"1ca7d9b6cff2eeceddf2dc1c8cb8f5d0216487cc.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 05/11] refs/reftable: don't recompute committer ident","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:11Z","receivedAt":"2024-04-08T12:24:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to write reflog entries we need to compute the committer's\nidentity as it gets encoded in the log record itself. The reftable\nbackend does this via `git_committer_info()` and `split_ident_line()` in\n`fill_reftable_log_record()`, which use the Git config as well as\nenvironment variables to figure out the identity.\n\nWhile most callers would only call `fill_reftable_log_record()` once or\ntwice, `write_transaction_table()` will call it as many times as there\nare queued ref updates. This can be quite a waste of effort when writing\nmany refs with reflog entries in a single transaction.\n\nRefactor the code to pre-compute the committer information. This results\nin a small speedup when writing 100000 refs in a single transaction:\n\n Benchmark 1: update-ref: create many refs (HEAD~)\n Time (mean ± σ): 2.895 s ± 0.020 s [User: 1.516 s, System: 1.374 s]\n Range (min … max): 2.868 s … 2.983 s 100 runs\n\n Benchmark 2: update-ref: create many refs (HEAD)\n Time (mean ± σ): 2.845 s ± 0.017 s [User: 1.461 s, System: 1.379 s]\n Range (min … max): 2.803 s … 2.913 s 100 runs\n\n Summary\n update-ref: create many refs (HEAD) ran\n 1.02 ± 0.01 times faster than update-ref: create many refs (HEAD~)\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/reftable-backend.c | 52 +++++++++++++++++++++++++++--------------\n 1 file changed, 34 insertions(+), 18 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 8a54b0d8b2..a5ef36ffa9 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -171,32 +171,30 @@ static int should_write_log(struct ref_store *refs, const char *refname)\n \t}\n }\n \n-static void fill_reftable_log_record(struct reftable_log_record *log)\n+static void fill_reftable_log_record(struct reftable_log_record *log, const struct ident_split *split)\n {\n-\tconst char *info = git_committer_info(0);\n-\tstruct ident_split split = {0};\n+\tconst char *tz_begin;\n \tint sign = 1;\n \n-\tif (split_ident_line(&split, info, strlen(info)))\n-\t\tBUG(\"failed splitting committer info\");\n-\n \treftable_log_record_release(log);\n \tlog->value_type = REFTABLE_LOG_UPDATE;\n \tlog->value.update.name =\n-\t\txstrndup(split.name_begin, split.name_end - split.name_begin);\n+\t\txstrndup(split->name_begin, split->name_end - split->name_begin);\n \tlog->value.update.email =\n-\t\txstrndup(split.mail_begin, split.mail_end - split.mail_begin);\n-\tlog->value.update.time = atol(split.date_begin);\n-\tif (*split.tz_begin == '-') {\n+\t\txstrndup(split->mail_begin, split->mail_end - split->mail_begin);\n+\tlog->value.update.time = atol(split->date_begin);\n+\n+\ttz_begin = split->tz_begin;\n+\tif (*tz_begin == '-') {\n \t\tsign = -1;\n-\t\tsplit.tz_begin++;\n+\t\ttz_begin++;\n \t}\n-\tif (*split.tz_begin == '+') {\n+\tif (*tz_begin == '+') {\n \t\tsign = 1;\n-\t\tsplit.tz_begin++;\n+\t\ttz_begin++;\n \t}\n \n-\tlog->value.update.tz_offset = sign * atoi(split.tz_begin);\n+\tlog->value.update.tz_offset = sign * atoi(tz_begin);\n }\n \n static int read_ref_without_reload(struct reftable_stack *stack,\n@@ -1018,9 +1016,15 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\treftable_stack_merged_table(arg->stack);\n \tuint64_t ts = reftable_stack_next_update_index(arg->stack);\n \tstruct reftable_log_record *logs = NULL;\n+\tstruct ident_split committer_ident = {0};\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n+\tconst char *committer_info;\n \tint ret = 0;\n \n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n \tQSORT(arg->updates, arg->updates_nr, transaction_update_cmp);\n \n \treftable_writer_set_limits(writer, ts, ts);\n@@ -1086,7 +1090,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\t\tlog = &logs[logs_nr++];\n \t\t\tmemset(log, 0, sizeof(*log));\n \n-\t\t\tfill_reftable_log_record(log);\n+\t\t\tfill_reftable_log_record(log, &committer_ident);\n \t\t\tlog->update_index = ts;\n \t\t\tlog->refname = xstrdup(u->refname);\n \t\t\tmemcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ);\n@@ -1233,9 +1237,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \t\t.value.symref = (char *)create->target,\n \t\t.update_index = ts,\n \t};\n+\tstruct ident_split committer_ident = {0};\n \tstruct reftable_log_record log = {0};\n \tstruct object_id new_oid;\n \tstruct object_id old_oid;\n+\tconst char *committer_info;\n \tint ret;\n \n \treftable_writer_set_limits(writer, ts, ts);\n@@ -1263,7 +1269,11 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \t !should_write_log(&create->refs->base, create->refname))\n \t\treturn 0;\n \n-\tfill_reftable_log_record(&log);\n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n+\tfill_reftable_log_record(&log, &committer_ident);\n \tlog.refname = xstrdup(create->refname);\n \tlog.update_index = ts;\n \tlog.value.update.message = xstrndup(create->logmsg,\n@@ -1339,10 +1349,16 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \tstruct reftable_log_record old_log = {0}, *logs = NULL;\n \tstruct reftable_iterator it = {0};\n \tstruct string_list skip = STRING_LIST_INIT_NODUP;\n+\tstruct ident_split committer_ident = {0};\n \tstruct strbuf errbuf = STRBUF_INIT;\n \tsize_t logs_nr = 0, logs_alloc = 0, i;\n+\tconst char *committer_info;\n \tint ret;\n \n+\tcommitter_info = git_committer_info(0);\n+\tif (split_ident_line(&committer_ident, committer_info, strlen(committer_info)))\n+\t\tBUG(\"failed splitting committer info\");\n+\n \tif (reftable_stack_read_ref(arg->stack, arg->oldname, &old_ref)) {\n \t\tret = error(_(\"refname %s not found\"), arg->oldname);\n \t\tgoto done;\n@@ -1417,7 +1433,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \n \t\tALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n \t\tmemset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));\n-\t\tfill_reftable_log_record(&logs[logs_nr]);\n+\t\tfill_reftable_log_record(&logs[logs_nr], &committer_ident);\n \t\tlogs[logs_nr].refname = (char *)arg->newname;\n \t\tlogs[logs_nr].update_index = deletion_ts;\n \t\tlogs[logs_nr].value.update.message =\n@@ -1449,7 +1465,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \t */\n \tALLOC_GROW(logs, logs_nr + 1, logs_alloc);\n \tmemset(&logs[logs_nr], 0, sizeof(logs[logs_nr]));\n-\tfill_reftable_log_record(&logs[logs_nr]);\n+\tfill_reftable_log_record(&logs[logs_nr], &committer_ident);\n \tlogs[logs_nr].refname = (char *)arg->newname;\n \tlogs[logs_nr].update_index = creation_ts;\n \tlogs[logs_nr].value.update.message =\n-- \n2.44.GIT\n\n"},{"id":"492530","messageId":"deabf821867e11f3f63f406ff88198a8f898b289.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 06/11] reftable/writer: refactorings for `writer_add_record()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:16Z","receivedAt":"2024-04-08T12:24:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Large parts of the reftable library do not conform to Git's typical code\nstyle. Refactor `writer_add_record()` such that it conforms better to it\nand add some documentation that explains some of its more intricate\nbehaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 38 +++++++++++++++++++++++++++-----------\n 1 file changed, 27 insertions(+), 11 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 1d9ff0fbfa..0ad5eb8887 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -209,7 +209,8 @@ static int writer_add_record(struct reftable_writer *w,\n \t\t\t struct reftable_record *rec)\n {\n \tstruct strbuf key = STRBUF_INIT;\n-\tint err = -1;\n+\tint err;\n+\n \treftable_record_key(rec, &key);\n \tif (strbuf_cmp(&w->last_key, &key) >= 0) {\n \t\terr = REFTABLE_API_ERROR;\n@@ -218,27 +219,42 @@ static int writer_add_record(struct reftable_writer *w,\n \n \tstrbuf_reset(&w->last_key);\n \tstrbuf_addbuf(&w->last_key, &key);\n-\tif (!w->block_writer) {\n+\tif (!w->block_writer)\n \t\twriter_reinit_block_writer(w, reftable_record_type(rec));\n-\t}\n \n-\tassert(block_writer_type(w->block_writer) == reftable_record_type(rec));\n+\tif (block_writer_type(w->block_writer) != reftable_record_type(rec))\n+\t\tBUG(\"record of type %d added to writer of type %d\",\n+\t\t reftable_record_type(rec), block_writer_type(w->block_writer));\n \n-\tif (block_writer_add(w->block_writer, rec) == 0) {\n+\t/*\n+\t * Try to add the record to the writer. If this succeeds then we're\n+\t * done. Otherwise the block writer may have hit the block size limit\n+\t * and needs to be flushed.\n+\t */\n+\tif (!block_writer_add(w->block_writer, rec)) {\n \t\terr = 0;\n \t\tgoto done;\n \t}\n \n+\t/*\n+\t * The current block is full, so we need to flush and reinitialize the\n+\t * writer to start writing the next block.\n+\t */\n \terr = writer_flush_block(w);\n-\tif (err < 0) {\n+\tif (err < 0)\n \t\tgoto done;\n-\t}\n-\n \twriter_reinit_block_writer(w, reftable_record_type(rec));\n+\n+\t/*\n+\t * Try to add the record to the writer again. If this still fails then\n+\t * the record does not fit into the block size.\n+\t *\n+\t * TODO: it would be great to have `block_writer_add()` return proper\n+\t * error codes so that we don't have to second-guess the failure\n+\t * mode here.\n+\t */\n \terr = block_writer_add(w->block_writer, rec);\n-\tif (err == -1) {\n-\t\t/* we are writing into memory, so an error can only mean it\n-\t\t * doesn't fit. */\n+\tif (err) {\n \t\terr = REFTABLE_ENTRY_TOO_BIG_ERROR;\n \t\tgoto done;\n \t}\n-- \n2.44.GIT\n\n"},{"id":"492531","messageId":"d47ad49d49916d02b8e62ee34404c025fa030845.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 07/11] reftable/writer: refactorings for `writer_flush_nonempty_block()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:20Z","receivedAt":"2024-04-08T12:24:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Large parts of the reftable library do not conform to Git's typical code\nstyle. Refactor `writer_flush_nonempty_block()` such that it conforms\nbetter to it and add some documentation that explains some of its more\nintricate behaviour.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 72 +++++++++++++++++++++++++++++------------------\n 1 file changed, 44 insertions(+), 28 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 0ad5eb8887..d347ec4cc6 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -659,58 +659,74 @@ static void writer_clear_index(struct reftable_writer *w)\n \tw->index_cap = 0;\n }\n \n-static const int debug = 0;\n-\n static int writer_flush_nonempty_block(struct reftable_writer *w)\n {\n+\tstruct reftable_index_record index_record = {\n+\t\t.last_key = STRBUF_INIT,\n+\t};\n \tuint8_t typ = block_writer_type(w->block_writer);\n-\tstruct reftable_block_stats *bstats =\n-\t\twriter_reftable_block_stats(w, typ);\n-\tuint64_t block_typ_off = (bstats->blocks == 0) ? w->next : 0;\n-\tint raw_bytes = block_writer_finish(w->block_writer);\n-\tint padding = 0;\n-\tint err = 0;\n-\tstruct reftable_index_record ir = { .last_key = STRBUF_INIT };\n+\tstruct reftable_block_stats *bstats;\n+\tint raw_bytes, padding = 0, err;\n+\tuint64_t block_typ_off;\n+\n+\t/*\n+\t * Finish the current block. This will cause the block writer to emit\n+\t * restart points and potentially compress records in case we are\n+\t * writing a log block.\n+\t *\n+\t * Note that this is still happening in memory.\n+\t */\n+\traw_bytes = block_writer_finish(w->block_writer);\n \tif (raw_bytes < 0)\n \t\treturn raw_bytes;\n \n-\tif (!w->opts.unpadded && typ != BLOCK_TYPE_LOG) {\n+\t/*\n+\t * By default, all records except for log records are padded to the\n+\t * block size.\n+\t */\n+\tif (!w->opts.unpadded && typ != BLOCK_TYPE_LOG)\n \t\tpadding = w->opts.block_size - raw_bytes;\n-\t}\n \n-\tif (block_typ_off > 0) {\n+\tbstats = writer_reftable_block_stats(w, typ);\n+\tblock_typ_off = (bstats->blocks == 0) ? w->next : 0;\n+\tif (block_typ_off > 0)\n \t\tbstats->offset = block_typ_off;\n-\t}\n-\n \tbstats->entries += w->block_writer->entries;\n \tbstats->restarts += w->block_writer->restart_len;\n \tbstats->blocks++;\n \tw->stats.blocks++;\n \n-\tif (debug) {\n-\t\tfprintf(stderr, \"block %c off %\" PRIu64 \" sz %d (%d)\\n\", typ,\n-\t\t\tw->next, raw_bytes,\n-\t\t\tget_be24(w->block + w->block_writer->header_off + 1));\n-\t}\n-\n-\tif (w->next == 0) {\n+\t/*\n+\t * If this is the first block we're writing to the table then we need\n+\t * to also write the reftable header.\n+\t */\n+\tif (!w->next)\n \t\twriter_write_header(w, w->block);\n-\t}\n \n \terr = padded_write(w, w->block, raw_bytes, padding);\n \tif (err < 0)\n \t\treturn err;\n \n+\t/*\n+\t * Add an index record for every block that we're writing. If we end up\n+\t * having more than a threshold of index records we will end up writing\n+\t * an index section in `writer_finish_section()`. Each index record\n+\t * contains the last record key of the block it is indexing as well as\n+\t * the offset of that block.\n+\t *\n+\t * Note that this also applies when flushing index blocks, in which\n+\t * case we will end up with a multi-level index.\n+\t */\n \tREFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);\n-\n-\tir.offset = w->next;\n-\tstrbuf_reset(&ir.last_key);\n-\tstrbuf_addbuf(&ir.last_key, &w->block_writer->last_key);\n-\tw->index[w->index_len] = ir;\n-\n+\tindex_record.offset = w->next;\n+\tstrbuf_reset(&index_record.last_key);\n+\tstrbuf_addbuf(&index_record.last_key, &w->block_writer->last_key);\n+\tw->index[w->index_len] = index_record;\n \tw->index_len++;\n+\n \tw->next += padding + raw_bytes;\n \tw->block_writer = NULL;\n+\n \treturn 0;\n }\n \n-- \n2.44.GIT\n\n"},{"id":"492532","messageId":"76d4a1f73b10c0a62707abea80f38fe18f4b8e7b.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 08/11] reftable/writer: unify releasing memory","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:25Z","receivedAt":"2024-04-08T12:24:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"There are two code paths which release memory of the reftable writer:\n\n - `reftable_writer_close()` releases internal state after it has\n written data.\n\n - `reftable_writer_free()` releases the block that was written to and\n the writer itself.\n\nBoth code paths free different parts of the writer, and consequently the\ncaller must make sure to call both. And while callers mostly do this\nalready, this falls apart when a write failure causes the caller to skip\ncalling `reftable_write_close()`.\n\nIntroduce a new function `reftable_writer_release()` that releases all\ninternal state and call it from both paths. Like this it is fine for the\ncaller to not call `reftable_writer_close()`.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 23 +++++++++++++++--------\n 1 file changed, 15 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex d347ec4cc6..4eeb736445 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -149,11 +149,21 @@ void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n \tw->max_update_index = max;\n }\n \n+static void writer_release(struct reftable_writer *w)\n+{\n+\tif (w) {\n+\t\treftable_free(w->block);\n+\t\tw->block = NULL;\n+\t\tblock_writer_release(&w->block_writer_data);\n+\t\tw->block_writer = NULL;\n+\t\twriter_clear_index(w);\n+\t\tstrbuf_release(&w->last_key);\n+\t}\n+}\n+\n void reftable_writer_free(struct reftable_writer *w)\n {\n-\tif (!w)\n-\t\treturn;\n-\treftable_free(w->block);\n+\twriter_release(w);\n \treftable_free(w);\n }\n \n@@ -643,16 +653,13 @@ int reftable_writer_close(struct reftable_writer *w)\n \t}\n \n done:\n-\t/* free up memory. */\n-\tblock_writer_release(&w->block_writer_data);\n-\twriter_clear_index(w);\n-\tstrbuf_release(&w->last_key);\n+\twriter_release(w);\n \treturn err;\n }\n \n static void writer_clear_index(struct reftable_writer *w)\n {\n-\tfor (size_t i = 0; i < w->index_len; i++)\n+\tfor (size_t i = 0; w->index && i < w->index_len; i++)\n \t\tstrbuf_release(&w->index[i].last_key);\n \tFREE_AND_NULL(w->index);\n \tw->index_len = 0;\n-- \n2.44.GIT\n\n"},{"id":"492533","messageId":"722ab0ee281060ad43882471d472e31fc066a339.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 09/11] reftable/writer: reset `last_key` instead of releasing it","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:30Z","receivedAt":"2024-04-08T12:24:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reftable writer tracks the last key that it has written so that it\ncan properly compute the compressed prefix for the next record it is\nabout to write. This last key must be reset whenever we move on to write\nthe next block, which is done in `writer_reinit_block_writer()`. We do\nthis by calling `strbuf_release()` though, which needlessly deallocates\nthe underlying buffer.\n\nConvert the code to use `strbuf_reset()` instead, which saves one\nallocation per block we're about to write. This requires us to also\namend `reftable_writer_free()` to release the buffer's memory now as we\npreviously seemingly relied on `writer_reinit_block_writer()` to release\nthe memory for us. Releasing memory here is the right thing to do\nanyway.\n\nWhile at it, convert a callsite where we truncate the buffer by setting\nits length to zero to instead use `strbuf_reset()`, too.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/writer.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 4eeb736445..10eccaaa07 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -109,7 +109,7 @@ static void writer_reinit_block_writer(struct reftable_writer *w, uint8_t typ)\n \t\tblock_start = header_size(writer_version(w));\n \t}\n \n-\tstrbuf_release(&w->last_key);\n+\tstrbuf_reset(&w->last_key);\n \tblock_writer_init(&w->block_writer_data, typ, w->block,\n \t\t\t w->opts.block_size, block_start,\n \t\t\t hash_size(w->opts.hash_id));\n@@ -478,7 +478,7 @@ static int writer_finish_section(struct reftable_writer *w)\n \tbstats->max_index_level = max_level;\n \n \t/* Reinit lastKey, as the next section can start with any key. */\n-\tw->last_key.len = 0;\n+\tstrbuf_reset(&w->last_key);\n \n \treturn 0;\n }\n-- \n2.44.GIT\n\n"},{"id":"492534","messageId":"962a96003b02ba21511da8ea820bbada2a767468.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 10/11] reftable/block: reuse zstream when writing log blocks","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:35Z","receivedAt":"2024-04-08T12:24:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While most reftable blocks are written to disk as-is, blocks for log\nrecords are compressed with zlib. To compress them we use `compress2()`,\nwhich is a simple wrapper around the more complex `zstream` interface\nthat would require multiple function invocations.\n\nOne downside of this interface is that `compress2()` will reallocate\ninternal state of the `zstream` interface on every single invocation.\nConsequently, as we call `compress2()` for every single log block which\nwe are about to write, this can lead to quite some memory allocation\nchurn.\n\nRefactor the code so that the block writer reuses a `zstream`. This\nsignificantly reduces the number of bytes allocated when writing many\nrefs in a single transaction, as demonstrated by the following benchmark\nthat writes 100k refs in a single transaction.\n\nBefore:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,631,887 allocs, 22,631,736 frees, 1,854,670,793 bytes allocated\n\nAfter:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,620,528 allocs, 22,620,377 frees, 1,245,549,984 bytes allocated\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 80 +++++++++++++++++++++++++++++++-----------------\n reftable/block.h | 1 +\n 2 files changed, 53 insertions(+), 28 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 298e8c56b9..d182561b4d 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -76,6 +76,10 @@ void block_writer_init(struct block_writer *bw, uint8_t typ, uint8_t *buf,\n \tbw->entries = 0;\n \tbw->restart_len = 0;\n \tbw->last_key.len = 0;\n+\tif (!bw->zstream) {\n+\t\tREFTABLE_CALLOC_ARRAY(bw->zstream, 1);\n+\t\tdeflateInit(bw->zstream, 9);\n+\t}\n }\n \n uint8_t block_writer_type(struct block_writer *bw)\n@@ -139,39 +143,57 @@ int block_writer_finish(struct block_writer *w)\n \tw->next += 2;\n \tput_be24(w->buf + 1 + w->header_off, w->next);\n \n+\t/*\n+\t * Log records are stored zlib-compressed. Note that the compression\n+\t * also spans over the restart points we have just written.\n+\t */\n \tif (block_writer_type(w) == BLOCK_TYPE_LOG) {\n \t\tint block_header_skip = 4 + w->header_off;\n-\t\tuLongf src_len = w->next - block_header_skip;\n-\t\tuLongf dest_cap = src_len * 1.001 + 12;\n-\t\tuint8_t *compressed;\n-\n-\t\tREFTABLE_ALLOC_ARRAY(compressed, dest_cap);\n-\n-\t\twhile (1) {\n-\t\t\tuLongf out_dest_len = dest_cap;\n-\t\t\tint zresult = compress2(compressed, &out_dest_len,\n-\t\t\t\t\t\tw->buf + block_header_skip,\n-\t\t\t\t\t\tsrc_len, 9);\n-\t\t\tif (zresult == Z_BUF_ERROR && dest_cap < LONG_MAX) {\n-\t\t\t\tdest_cap *= 2;\n-\t\t\t\tcompressed =\n-\t\t\t\t\treftable_realloc(compressed, dest_cap);\n-\t\t\t\tif (compressed)\n-\t\t\t\t\tcontinue;\n-\t\t\t}\n-\n-\t\t\tif (Z_OK != zresult) {\n-\t\t\t\treftable_free(compressed);\n-\t\t\t\treturn REFTABLE_ZLIB_ERROR;\n-\t\t\t}\n-\n-\t\t\tmemcpy(w->buf + block_header_skip, compressed,\n-\t\t\t out_dest_len);\n-\t\t\tw->next = out_dest_len + block_header_skip;\n+\t\tuLongf src_len = w->next - block_header_skip, compressed_len;\n+\t\tunsigned char *compressed;\n+\t\tint ret;\n+\n+\t\tret = deflateReset(w->zstream);\n+\t\tif (ret != Z_OK)\n+\t\t\treturn REFTABLE_ZLIB_ERROR;\n+\n+\t\t/*\n+\t\t * Precompute the upper bound of how many bytes the compressed\n+\t\t * data may end up with. Combined with `Z_FINISH`, `deflate()`\n+\t\t * is guaranteed to return `Z_STREAM_END`.\n+\t\t */\n+\t\tcompressed_len = deflateBound(w->zstream, src_len);\n+\t\tREFTABLE_ALLOC_ARRAY(compressed, compressed_len);\n+\n+\t\tw->zstream->next_out = compressed;\n+\t\tw->zstream->avail_out = compressed_len;\n+\t\tw->zstream->next_in = w->buf + block_header_skip;\n+\t\tw->zstream->avail_in = src_len;\n+\n+\t\t/*\n+\t\t * We want to perform all decompression in a single step, which\n+\t\t * is why we can pass Z_FINISH here. As we have precomputed the\n+\t\t * deflated buffer's size via `deflateBound()` this function is\n+\t\t * guaranteed to succeed according to the zlib documentation.\n+\t\t */\n+\t\tret = deflate(w->zstream, Z_FINISH);\n+\t\tif (ret != Z_STREAM_END) {\n \t\t\treftable_free(compressed);\n-\t\t\tbreak;\n+\t\t\treturn REFTABLE_ZLIB_ERROR;\n \t\t}\n+\n+\t\t/*\n+\t\t * Overwrite the uncompressed data we have already written and\n+\t\t * adjust the `next` pointer to point right after the\n+\t\t * compressed data.\n+\t\t */\n+\t\tmemcpy(w->buf + block_header_skip, compressed,\n+\t\t w->zstream->total_out);\n+\t\tw->next = w->zstream->total_out + block_header_skip;\n+\n+\t\treftable_free(compressed);\n \t}\n+\n \treturn w->next;\n }\n \n@@ -480,6 +502,8 @@ int block_reader_seek(struct block_reader *br, struct block_iter *it,\n \n void block_writer_release(struct block_writer *bw)\n {\n+\tdeflateEnd(bw->zstream);\n+\tFREE_AND_NULL(bw->zstream);\n \tFREE_AND_NULL(bw->restarts);\n \tstrbuf_release(&bw->last_key);\n \t/* the block is not owned. */\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 47acc62c0a..1375957fc8 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -18,6 +18,7 @@ license that can be found in the LICENSE file or at\n * allocation overhead.\n */\n struct block_writer {\n+\tz_stream *zstream;\n \tuint8_t *buf;\n \tuint32_t block_size;\n \n-- \n2.44.GIT\n\n"},{"id":"492535","messageId":"323892841a3dc31cf0d4c5e614f9f0059a147c1d.1712578837.git.ps@pks.im","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"[PATCH v3 11/11] reftable/block: reuse compressed array","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-08T12:24:40Z","receivedAt":"2024-04-08T12:24:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Similar to the preceding commit, let's reuse the `compressed` array that\nwe use to store compressed data in. This results in a small reduction in\nmemory allocations when writing many refs.\n\nBefore:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,620,528 allocs, 22,620,377 frees, 1,245,549,984 bytes allocated\n\nAfter:\n\n HEAP SUMMARY:\n in use at exit: 671,931 bytes in 151 blocks\n total heap usage: 22,618,257 allocs, 22,618,106 frees, 1,236,351,528 bytes allocated\n\nSo while the reduction in allocations isn't really all that big, it's a\nlow hanging fruit and thus there isn't much of a reason not to pick it.\n\nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 14 +++++---------\n reftable/block.h | 3 +++\n 2 files changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex d182561b4d..fd494876b7 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -150,7 +150,6 @@ int block_writer_finish(struct block_writer *w)\n \tif (block_writer_type(w) == BLOCK_TYPE_LOG) {\n \t\tint block_header_skip = 4 + w->header_off;\n \t\tuLongf src_len = w->next - block_header_skip, compressed_len;\n-\t\tunsigned char *compressed;\n \t\tint ret;\n \n \t\tret = deflateReset(w->zstream);\n@@ -163,9 +162,9 @@ int block_writer_finish(struct block_writer *w)\n \t\t * is guaranteed to return `Z_STREAM_END`.\n \t\t */\n \t\tcompressed_len = deflateBound(w->zstream, src_len);\n-\t\tREFTABLE_ALLOC_ARRAY(compressed, compressed_len);\n+\t\tREFTABLE_ALLOC_GROW(w->compressed, compressed_len, w->compressed_cap);\n \n-\t\tw->zstream->next_out = compressed;\n+\t\tw->zstream->next_out = w->compressed;\n \t\tw->zstream->avail_out = compressed_len;\n \t\tw->zstream->next_in = w->buf + block_header_skip;\n \t\tw->zstream->avail_in = src_len;\n@@ -177,21 +176,17 @@ int block_writer_finish(struct block_writer *w)\n \t\t * guaranteed to succeed according to the zlib documentation.\n \t\t */\n \t\tret = deflate(w->zstream, Z_FINISH);\n-\t\tif (ret != Z_STREAM_END) {\n-\t\t\treftable_free(compressed);\n+\t\tif (ret != Z_STREAM_END)\n \t\t\treturn REFTABLE_ZLIB_ERROR;\n-\t\t}\n \n \t\t/*\n \t\t * Overwrite the uncompressed data we have already written and\n \t\t * adjust the `next` pointer to point right after the\n \t\t * compressed data.\n \t\t */\n-\t\tmemcpy(w->buf + block_header_skip, compressed,\n+\t\tmemcpy(w->buf + block_header_skip, w->compressed,\n \t\t w->zstream->total_out);\n \t\tw->next = w->zstream->total_out + block_header_skip;\n-\n-\t\treftable_free(compressed);\n \t}\n \n \treturn w->next;\n@@ -505,6 +500,7 @@ void block_writer_release(struct block_writer *bw)\n \tdeflateEnd(bw->zstream);\n \tFREE_AND_NULL(bw->zstream);\n \tFREE_AND_NULL(bw->restarts);\n+\tFREE_AND_NULL(bw->compressed);\n \tstrbuf_release(&bw->last_key);\n \t/* the block is not owned. */\n }\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 1375957fc8..657498014c 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -19,6 +19,9 @@ license that can be found in the LICENSE file or at\n */\n struct block_writer {\n \tz_stream *zstream;\n+\tunsigned char *compressed;\n+\tsize_t compressed_cap;\n+\n \tuint8_t *buf;\n \tuint32_t block_size;\n \n-- \n2.44.GIT\n\n"},{"id":"492599","messageId":"xmqq8r1n5rdi.fsf@gitster.g","threadId":"61255","inReplyTo":"cover.1712578837.git.ps@pks.im","subject":"Re: [PATCH v3 00/11] reftable: optimize write performance","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-09T00:09:13Z","receivedAt":"2024-04-09T00:09:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> this is the first version of my patch series that aims to optimize write\n> performance with the reftable backend.\n>\n> Changes compared to v2:\n>\n> - The series now deepends on ps/reftable-binsearch-update at\n> d51d8cc368 (reftable/block: avoid decoding keys when searching\n> restart points, 2024-04-03). This is to resolve a merge conflict\n> with that other series which has landed in \"next\" already.\n>\n> - Dropped the \"reftable_\" prefix from newly introduced internal\n> reftable functions.\n\nWell, since I resolved the conflict and my rerere database already\nknows the resolution, you did not have to do the rebasing yourself.\nAfter undoing the rebase and recreating the merge of this topic into\n'seen', i.e. db20edbf (Merge branch 'ps/reftable-write-optim' into\njch, 2024-04-05), the difference I see between the previous version\nand this iteration I see are the following. Please tell me if that\nis the only change you are expecting, and please yell at me if that\nis not the case---it would serve as a sanity check of my previous\nconflict resolution that will also be applied going forward.\n\nThanks, queued.\n\ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 32438e49b4..10eccaaa07 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -149,7 +149,7 @@ void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n \tw->max_update_index = max;\n }\n \n-static void reftable_writer_release(struct reftable_writer *w)\n+static void writer_release(struct reftable_writer *w)\n {\n \tif (w) {\n \t\treftable_free(w->block);\n@@ -163,7 +163,7 @@ static void reftable_writer_release(struct reftable_writer *w)\n \n void reftable_writer_free(struct reftable_writer *w)\n {\n-\treftable_writer_release(w);\n+\twriter_release(w);\n \treftable_free(w);\n }\n \n@@ -653,7 +653,7 @@ int reftable_writer_close(struct reftable_writer *w)\n \t}\n \n done:\n-\treftable_writer_release(w);\n+\twriter_release(w);\n \treturn err;\n }\n \n"},{"id":"492611","messageId":"ZhSzDxcEYgqauWi8@tanuki","threadId":"61255","inReplyTo":"xmqq8r1n5rdi.fsf@gitster.g","subject":"Re: [PATCH v3 00/11] reftable: optimize write performance","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-04-09T03:16:31Z","receivedAt":"2024-04-09T03:16:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Apr 08, 2024 at 05:09:13PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > this is the first version of my patch series that aims to optimize write\n> > performance with the reftable backend.\n> >\n> > Changes compared to v2:\n> >\n> > - The series now deepends on ps/reftable-binsearch-update at\n> > d51d8cc368 (reftable/block: avoid decoding keys when searching\n> > restart points, 2024-04-03). This is to resolve a merge conflict\n> > with that other series which has landed in \"next\" already.\n> >\n> > - Dropped the \"reftable_\" prefix from newly introduced internal\n> > reftable functions.\n> \n> Well, since I resolved the conflict and my rerere database already\n> knows the resolution, you did not have to do the rebasing yourself.\n> After undoing the rebase and recreating the merge of this topic into\n> 'seen', i.e. db20edbf (Merge branch 'ps/reftable-write-optim' into\n> jch, 2024-04-05), the difference I see between the previous version\n> and this iteration I see are the following. Please tell me if that\n> is the only change you are expecting, and please yell at me if that\n> is not the case---it would serve as a sanity check of my previous\n> conflict resolution that will also be applied going forward.\n> \n> Thanks, queued.\n\nThe resolution looks as expected to me. Thanks!\n\nPatrick\n\n> diff --git a/reftable/writer.c b/reftable/writer.c\n> index 32438e49b4..10eccaaa07 100644\n> --- a/reftable/writer.c\n> +++ b/reftable/writer.c\n> @@ -149,7 +149,7 @@ void reftable_writer_set_limits(struct reftable_writer *w, uint64_t min,\n> \tw->max_update_index = max;\n> }\n> \n> -static void reftable_writer_release(struct reftable_writer *w)\n> +static void writer_release(struct reftable_writer *w)\n> {\n> \tif (w) {\n> \t\treftable_free(w->block);\n> @@ -163,7 +163,7 @@ static void reftable_writer_release(struct reftable_writer *w)\n> \n> void reftable_writer_free(struct reftable_writer *w)\n> {\n> -\treftable_writer_release(w);\n> +\twriter_release(w);\n> \treftable_free(w);\n> }\n> \n> @@ -653,7 +653,7 @@ int reftable_writer_close(struct reftable_writer *w)\n> \t}\n> \n> done:\n> -\treftable_writer_release(w);\n> +\twriter_release(w);\n> \treturn err;\n> }\n> \n"}]}