{"thread":{"id":"61053","subject":"[PATCH 0/7] reftable: memory optimizations for reflog iteration","startedAt":"2024-03-05T12:10:56Z","lastAt":"2024-03-21T13:10:05Z","messageCount":20,"participants":["Patrick Steinhardt","Karthik Nayak","Junio C Hamano","Josh Steadmon","James Liu"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"489952","messageId":"cover.1709640322.git.ps@pks.im","threadId":"61053","inReplyTo":null,"subject":"[PATCH 0/7] reftable: memory optimizations for reflog iteration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T12:10:51Z","receivedAt":"2024-03-05T12:10:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series does the same as all the preceding patch series that\noptimized how the reftable library iterates through refs, but for\nreflogs instead.\n\nThe goal of this patch series is to arrive at a constant number of\nallocations when iterating refs. This is achieved in mostly the same way\nwe did it for ref iteration, namely by reusing already-allocated memory.\nOverall, this brings us down from 8 allocations per reflog record to\nessentially 0 allocations per reflog. Iterating through 1 million\nreflogs with `git reflog list` thus goes down from 8.068m allocations to\nonly around 68.5k.\n\nThis series is built on top of \"master\" at b387623c12 (The third batch,\n2024-03-01) with Junio's \"ps/reftable-iteration-perf-part2\" at\n43f70eaea0 (refs/reftable: precompute prefix length, 2024-03-04) merged\ninto it.\n\nPatrick\n\nPatrick Steinhardt (7):\n  refs/reftable: reload correct stack when creating reflog iter\n  reftable/record: convert old and new object IDs to arrays\n  reftable/record: avoid copying author info\n  reftable/record: reuse refnames when decoding log records\n  reftable/record: reuse message when decoding log records\n  reftable/record: use scratch buffer when decoding records\n  refs/reftable: track last log record name via strbuf\n\n refs/reftable-backend.c    |  52 +++++----------\n reftable/block.c           |   4 +-\n reftable/block.h           |   2 +\n reftable/merged_test.c     |  11 ++--\n reftable/readwrite_test.c  |  62 +++++++-----------\n reftable/record.c          | 129 ++++++++++++++-----------------------\n reftable/record.h          |   5 +-\n reftable/record_test.c     |  68 ++++++++++---------\n reftable/reftable-record.h |   6 +-\n reftable/stack_test.c      |  26 ++++----\n 10 files changed, 154 insertions(+), 211 deletions(-)\n\n-- \n2.44.0\n\n"},{"id":"489953","messageId":"b0414221ecad1920c84f4ab498e55edec57f06b6.1709640322.git.ps@pks.im","threadId":"61053","inReplyTo":"cover.1709640322.git.ps@pks.im","subject":"[PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T12:10:55Z","receivedAt":"2024-03-05T12:10:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When creating a new reflog iterator, we first have to reload the stack\nthat the iterator is being created. This is done so that any concurrent\nwrites to the stack are reflected. But `reflog_iterator_for_stack()`\nalways reloads the main stack, which is wrong.\n\nFix this and reload the correct stack.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/reftable-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 249a618b5a..f04be942ac 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1682,7 +1682,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n \tif (ret)\n \t\tgoto done;\n \n-\tret = reftable_stack_reload(refs->main_stack);\n+\tret = reftable_stack_reload(stack);\n \tif (ret < 0)\n \t\tgoto done;\n \n-- \n2.44.0\n\n"},{"id":"489954","messageId":"b63722b35dbf44c408517b6b657b8ff7c81df325.1709640322.git.ps@pks.im","threadId":"61053","inReplyTo":"cover.1709640322.git.ps@pks.im","subject":"[PATCH 2/7] reftable/record: convert old and new object IDs to arrays","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T12:10:59Z","receivedAt":"2024-03-05T12:11:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 7af607c58d (reftable/record: store \"val1\" hashes as static arrays,\n2024-01-03) and b31e3cc620 (reftable/record: store \"val2\" hashes as\nstatic arrays, 2024-01-03) we have converted ref records to store their\nobject IDs in a static array. Convert log records to do the same so that\ntheir old and new object IDs are arrays, too.\n\nThis change results in two allocations less per log record that we're\niterating over. Before:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 8,068,495 allocs, 8,068,373 frees, 401,011,862 bytes allocated\n\nAfter:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 6,068,489 allocs, 6,068,367 frees, 361,011,822 bytes allocated\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/reftable-backend.c    | 39 ++++++------------------\n reftable/merged_test.c     | 11 +++----\n reftable/readwrite_test.c  | 62 +++++++++++++++-----------------------\n reftable/record.c          | 59 ++++++------------------------------\n reftable/record_test.c     | 11 -------\n reftable/reftable-record.h |  4 +--\n reftable/stack_test.c      | 26 +++++++---------\n 7 files changed, 61 insertions(+), 151 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex f04be942ac..2de57c047a 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -171,23 +171,6 @@ static int should_write_log(struct ref_store *refs, const char *refname)\n \t}\n }\n \n-static void clear_reftable_log_record(struct reftable_log_record *log)\n-{\n-\tswitch (log->value_type) {\n-\tcase REFTABLE_LOG_UPDATE:\n-\t\t/*\n-\t\t * When we write log records, the hashes are owned by the\n-\t\t * caller and thus shouldn't be free'd.\n-\t\t */\n-\t\tlog->value.update.old_hash = NULL;\n-\t\tlog->value.update.new_hash = NULL;\n-\t\tbreak;\n-\tcase REFTABLE_LOG_DELETION:\n-\t\tbreak;\n-\t}\n-\treftable_log_record_release(log);\n-}\n-\n static void fill_reftable_log_record(struct reftable_log_record *log)\n {\n \tconst char *info = git_committer_info(0);\n@@ -1102,8 +1085,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n \t\t\tfill_reftable_log_record(log);\n \t\t\tlog->update_index = ts;\n \t\t\tlog->refname = xstrdup(u->refname);\n-\t\t\tlog->value.update.new_hash = u->new_oid.hash;\n-\t\t\tlog->value.update.old_hash = tx_update->current_oid.hash;\n+\t\t\tmemcpy(log->value.update.new_hash, u->new_oid.hash, GIT_MAX_RAWSZ);\n+\t\t\tmemcpy(log->value.update.old_hash, tx_update->current_oid.hash, GIT_MAX_RAWSZ);\n \t\t\tlog->value.update.message =\n \t\t\t\txstrndup(u->msg, arg->refs->write_options.block_size / 2);\n \t\t}\n@@ -1158,7 +1141,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data\n done:\n \tassert(ret != REFTABLE_API_ERROR);\n \tfor (i = 0; i < logs_nr; i++)\n-\t\tclear_reftable_log_record(&logs[i]);\n+\t\treftable_log_record_release(&logs[i]);\n \tfree(logs);\n \treturn ret;\n }\n@@ -1275,13 +1258,13 @@ static int write_create_symref_table(struct reftable_writer *writer, void *cb_da\n \tlog.update_index = ts;\n \tlog.value.update.message = xstrndup(create->logmsg,\n \t\t\t\t\t    create->refs->write_options.block_size / 2);\n-\tlog.value.update.new_hash = new_oid.hash;\n+\tmemcpy(log.value.update.new_hash, new_oid.hash, GIT_MAX_RAWSZ);\n \tif (refs_resolve_ref_unsafe(&create->refs->base, create->refname,\n \t\t\t\t    RESOLVE_REF_READING, &old_oid, NULL))\n-\t\tlog.value.update.old_hash = old_oid.hash;\n+\t\tmemcpy(log.value.update.old_hash, old_oid.hash, GIT_MAX_RAWSZ);\n \n \tret = reftable_writer_add_log(writer, &log);\n-\tclear_reftable_log_record(&log);\n+\treftable_log_record_release(&log);\n \treturn ret;\n }\n \n@@ -1420,7 +1403,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \t\tlogs[logs_nr].update_index = deletion_ts;\n \t\tlogs[logs_nr].value.update.message =\n \t\t\txstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);\n-\t\tlogs[logs_nr].value.update.old_hash = old_ref.value.val1;\n+\t\tmemcpy(logs[logs_nr].value.update.old_hash, old_ref.value.val1, GIT_MAX_RAWSZ);\n \t\tlogs_nr++;\n \n \t\tret = read_ref_without_reload(arg->stack, \"HEAD\", &head_oid, &head_referent, &head_type);\n@@ -1452,7 +1435,7 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \tlogs[logs_nr].update_index = creation_ts;\n \tlogs[logs_nr].value.update.message =\n \t\txstrndup(arg->logmsg, arg->refs->write_options.block_size / 2);\n-\tlogs[logs_nr].value.update.new_hash = old_ref.value.val1;\n+\tmemcpy(logs[logs_nr].value.update.new_hash, old_ref.value.val1, GIT_MAX_RAWSZ);\n \tlogs_nr++;\n \n \t/*\n@@ -1515,10 +1498,6 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)\n \tfor (i = 0; i < logs_nr; i++) {\n \t\tif (!strcmp(logs[i].refname, \"HEAD\"))\n \t\t\tcontinue;\n-\t\tif (logs[i].value.update.old_hash == old_ref.value.val1)\n-\t\t\tlogs[i].value.update.old_hash = NULL;\n-\t\tif (logs[i].value.update.new_hash == old_ref.value.val1)\n-\t\t\tlogs[i].value.update.new_hash = NULL;\n \t\tlogs[i].refname = NULL;\n \t\treftable_log_record_release(&logs[i]);\n \t}\n@@ -2180,7 +2159,7 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,\n \t\t\tdest->value_type = REFTABLE_LOG_DELETION;\n \t\t} else {\n \t\t\tif ((flags & EXPIRE_REFLOGS_REWRITE) && last_hash)\n-\t\t\t\tdest->value.update.old_hash = last_hash;\n+\t\t\t\tmemcpy(dest->value.update.old_hash, last_hash, GIT_MAX_RAWSZ);\n \t\t\tlast_hash = logs[i].value.update.new_hash;\n \t\t}\n \t}\ndiff --git a/reftable/merged_test.c b/reftable/merged_test.c\nindex d0f77a3b8f..530fc82d1c 100644\n--- a/reftable/merged_test.c\n+++ b/reftable/merged_test.c\n@@ -289,16 +289,13 @@ merged_table_from_log_records(struct reftable_log_record **logs,\n \n static void test_merged_logs(void)\n {\n-\tuint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };\n-\tuint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };\n-\tuint8_t hash3[GIT_SHA1_RAWSZ] = { 3 };\n \tstruct reftable_log_record r1[] = {\n \t\t{\n \t\t\t.refname = \"a\",\n \t\t\t.update_index = 2,\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t\t.value.update = {\n-\t\t\t\t.old_hash = hash2,\n+\t\t\t\t.old_hash = { 2 },\n \t\t\t\t/* deletion */\n \t\t\t\t.name = \"jane doe\",\n \t\t\t\t.email = \"jane@invalid\",\n@@ -310,8 +307,8 @@ static void test_merged_logs(void)\n \t\t\t.update_index = 1,\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t\t.value.update = {\n-\t\t\t\t.old_hash = hash1,\n-\t\t\t\t.new_hash = hash2,\n+\t\t\t\t.old_hash = { 1 },\n+\t\t\t\t.new_hash = { 2 },\n \t\t\t\t.name = \"jane doe\",\n \t\t\t\t.email = \"jane@invalid\",\n \t\t\t\t.message = \"message1\",\n@@ -324,7 +321,7 @@ static void test_merged_logs(void)\n \t\t\t.update_index = 3,\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t\t.value.update = {\n-\t\t\t\t.new_hash = hash3,\n+\t\t\t\t.new_hash = { 3 },\n \t\t\t\t.name = \"jane doe\",\n \t\t\t\t.email = \"jane@invalid\",\n \t\t\t\t.message = \"message3\",\ndiff --git a/reftable/readwrite_test.c b/reftable/readwrite_test.c\nindex 363fe0f998..a6dbd214c5 100644\n--- a/reftable/readwrite_test.c\n+++ b/reftable/readwrite_test.c\n@@ -77,18 +77,15 @@ static void write_table(char ***names, struct strbuf *buf, int N,\n \t}\n \n \tfor (i = 0; i < N; i++) {\n-\t\tuint8_t hash[GIT_SHA256_RAWSZ] = { 0 };\n \t\tchar name[100];\n \t\tint n;\n \n-\t\tset_test_hash(hash, i);\n-\n \t\tsnprintf(name, sizeof(name), \"refs/heads/branch%02d\", i);\n \n \t\tlog.refname = name;\n \t\tlog.update_index = update_index;\n \t\tlog.value_type = REFTABLE_LOG_UPDATE;\n-\t\tlog.value.update.new_hash = hash;\n+\t\tset_test_hash(log.value.update.new_hash, i);\n \t\tlog.value.update.message = \"message\";\n \n \t\tn = reftable_writer_add_log(w, &log);\n@@ -137,13 +134,10 @@ static void test_log_buffer_size(void)\n \t/* This tests buffer extension for log compression. Must use a random\n \t   hash, to ensure that the compressed part is larger than the original.\n \t*/\n-\tuint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];\n \tfor (i = 0; i < GIT_SHA1_RAWSZ; i++) {\n-\t\thash1[i] = (uint8_t)(git_rand() % 256);\n-\t\thash2[i] = (uint8_t)(git_rand() % 256);\n+\t\tlog.value.update.old_hash[i] = (uint8_t)(git_rand() % 256);\n+\t\tlog.value.update.new_hash[i] = (uint8_t)(git_rand() % 256);\n \t}\n-\tlog.value.update.old_hash = hash1;\n-\tlog.value.update.new_hash = hash2;\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n \tEXPECT_ERR(err);\n@@ -161,25 +155,26 @@ static void test_log_overflow(void)\n \t\t.block_size = ARRAY_SIZE(msg),\n \t};\n \tint err;\n-\tstruct reftable_log_record\n-\t\tlog = { .refname = \"refs/heads/master\",\n-\t\t\t.update_index = 0xa,\n-\t\t\t.value_type = REFTABLE_LOG_UPDATE,\n-\t\t\t.value = { .update = {\n-\t\t\t\t\t   .name = \"Han-Wen Nienhuys\",\n-\t\t\t\t\t   .email = \"hanwen@google.com\",\n-\t\t\t\t\t   .tz_offset = 100,\n-\t\t\t\t\t   .time = 0x5e430672,\n-\t\t\t\t\t   .message = msg,\n-\t\t\t\t   } } };\n+\tstruct reftable_log_record log = {\n+\t\t.refname = \"refs/heads/master\",\n+\t\t.update_index = 0xa,\n+\t\t.value_type = REFTABLE_LOG_UPDATE,\n+\t\t.value = {\n+\t\t\t.update = {\n+\t\t\t\t.old_hash = { 1 },\n+\t\t\t\t.new_hash = { 2 },\n+\t\t\t\t.name = \"Han-Wen Nienhuys\",\n+\t\t\t\t.email = \"hanwen@google.com\",\n+\t\t\t\t.tz_offset = 100,\n+\t\t\t\t.time = 0x5e430672,\n+\t\t\t\t.message = msg,\n+\t\t\t},\n+\t\t},\n+\t};\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \n-\tuint8_t hash1[GIT_SHA1_RAWSZ]  = {1}, hash2[GIT_SHA1_RAWSZ] = { 2 };\n-\n \tmemset(msg, 'x', sizeof(msg) - 1);\n-\tlog.value.update.old_hash = hash1;\n-\tlog.value.update.new_hash = hash2;\n \treftable_writer_set_limits(w, update_index, update_index);\n \terr = reftable_writer_add_log(w, &log);\n \tEXPECT(err == REFTABLE_ENTRY_TOO_BIG_ERROR);\n@@ -219,16 +214,13 @@ static void test_log_write_read(void)\n \t\tEXPECT_ERR(err);\n \t}\n \tfor (i = 0; i < N; i++) {\n-\t\tuint8_t hash1[GIT_SHA1_RAWSZ], hash2[GIT_SHA1_RAWSZ];\n \t\tstruct reftable_log_record log = { NULL };\n-\t\tset_test_hash(hash1, i);\n-\t\tset_test_hash(hash2, i + 1);\n \n \t\tlog.refname = names[i];\n \t\tlog.update_index = i;\n \t\tlog.value_type = REFTABLE_LOG_UPDATE;\n-\t\tlog.value.update.old_hash = hash1;\n-\t\tlog.value.update.new_hash = hash2;\n+\t\tset_test_hash(log.value.update.old_hash, i);\n+\t\tset_test_hash(log.value.update.new_hash, i + 1);\n \n \t\terr = reftable_writer_add_log(w, &log);\n \t\tEXPECT_ERR(err);\n@@ -298,18 +290,15 @@ static void test_log_zlib_corruption(void)\n \tstruct reftable_writer *w =\n \t\treftable_new_writer(&strbuf_add_void, &noop_flush, &buf, &opts);\n \tconst struct reftable_stats *stats = NULL;\n-\tuint8_t hash1[GIT_SHA1_RAWSZ] = { 1 };\n-\tuint8_t hash2[GIT_SHA1_RAWSZ] = { 2 };\n \tchar message[100] = { 0 };\n \tint err, i, n;\n-\n \tstruct reftable_log_record log = {\n \t\t.refname = \"refname\",\n \t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t.value = {\n \t\t\t.update = {\n-\t\t\t\t.new_hash = hash1,\n-\t\t\t\t.old_hash = hash2,\n+\t\t\t\t.new_hash = { 1 },\n+\t\t\t\t.old_hash = { 2 },\n \t\t\t\t.name = \"My Name\",\n \t\t\t\t.email = \"myname@invalid\",\n \t\t\t\t.message = message,\n@@ -821,13 +810,12 @@ static void test_write_multiple_indices(void)\n \t}\n \n \tfor (i = 0; i < 100; i++) {\n-\t\tunsigned char hash[GIT_SHA1_RAWSZ] = {i};\n \t\tstruct reftable_log_record log = {\n \t\t\t.update_index = 1,\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t\t.value.update = {\n-\t\t\t\t.old_hash = hash,\n-\t\t\t\t.new_hash = hash,\n+\t\t\t\t.old_hash = { i },\n+\t\t\t\t.new_hash = { i },\n \t\t\t},\n \t\t};\n \ndiff --git a/reftable/record.c b/reftable/record.c\nindex 367de04600..8d2cd6b081 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -763,16 +763,10 @@ static void reftable_log_record_copy_from(void *rec, const void *src_rec,\n \t\t\t\txstrdup(dst->value.update.message);\n \t\t}\n \n-\t\tif (dst->value.update.new_hash) {\n-\t\t\tREFTABLE_ALLOC_ARRAY(dst->value.update.new_hash, hash_size);\n-\t\t\tmemcpy(dst->value.update.new_hash,\n-\t\t\t       src->value.update.new_hash, hash_size);\n-\t\t}\n-\t\tif (dst->value.update.old_hash) {\n-\t\t\tREFTABLE_ALLOC_ARRAY(dst->value.update.old_hash, hash_size);\n-\t\t\tmemcpy(dst->value.update.old_hash,\n-\t\t\t       src->value.update.old_hash, hash_size);\n-\t\t}\n+\t\tmemcpy(dst->value.update.new_hash,\n+\t\t       src->value.update.new_hash, hash_size);\n+\t\tmemcpy(dst->value.update.old_hash,\n+\t\t       src->value.update.old_hash, hash_size);\n \t\tbreak;\n \t}\n }\n@@ -790,8 +784,6 @@ void reftable_log_record_release(struct reftable_log_record *r)\n \tcase REFTABLE_LOG_DELETION:\n \t\tbreak;\n \tcase REFTABLE_LOG_UPDATE:\n-\t\treftable_free(r->value.update.new_hash);\n-\t\treftable_free(r->value.update.old_hash);\n \t\treftable_free(r->value.update.name);\n \t\treftable_free(r->value.update.email);\n \t\treftable_free(r->value.update.message);\n@@ -808,33 +800,20 @@ static uint8_t reftable_log_record_val_type(const void *rec)\n \treturn reftable_log_record_is_deletion(log) ? 0 : 1;\n }\n \n-static uint8_t zero[GIT_SHA256_RAWSZ] = { 0 };\n-\n static int reftable_log_record_encode(const void *rec, struct string_view s,\n \t\t\t\t      int hash_size)\n {\n \tconst struct reftable_log_record *r = rec;\n \tstruct string_view start = s;\n \tint n = 0;\n-\tuint8_t *oldh = NULL;\n-\tuint8_t *newh = NULL;\n \tif (reftable_log_record_is_deletion(r))\n \t\treturn 0;\n \n-\toldh = r->value.update.old_hash;\n-\tnewh = r->value.update.new_hash;\n-\tif (!oldh) {\n-\t\toldh = zero;\n-\t}\n-\tif (!newh) {\n-\t\tnewh = zero;\n-\t}\n-\n \tif (s.len < 2 * hash_size)\n \t\treturn -1;\n \n-\tmemcpy(s.buf, oldh, hash_size);\n-\tmemcpy(s.buf + hash_size, newh, hash_size);\n+\tmemcpy(s.buf, r->value.update.old_hash, hash_size);\n+\tmemcpy(s.buf + hash_size, r->value.update.new_hash, hash_size);\n \tstring_view_consume(&s, 2 * hash_size);\n \n \tn = encode_string(r->value.update.name ? r->value.update.name : \"\", s);\n@@ -891,8 +870,6 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \tif (val_type != r->value_type) {\n \t\tswitch (r->value_type) {\n \t\tcase REFTABLE_LOG_UPDATE:\n-\t\t\tFREE_AND_NULL(r->value.update.old_hash);\n-\t\t\tFREE_AND_NULL(r->value.update.new_hash);\n \t\t\tFREE_AND_NULL(r->value.update.message);\n \t\t\tFREE_AND_NULL(r->value.update.email);\n \t\t\tFREE_AND_NULL(r->value.update.name);\n@@ -909,11 +886,6 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \tif (in.len < 2 * hash_size)\n \t\treturn REFTABLE_FORMAT_ERROR;\n \n-\tr->value.update.old_hash =\n-\t\treftable_realloc(r->value.update.old_hash, hash_size);\n-\tr->value.update.new_hash =\n-\t\treftable_realloc(r->value.update.new_hash, hash_size);\n-\n \tmemcpy(r->value.update.old_hash, in.buf, hash_size);\n \tmemcpy(r->value.update.new_hash, in.buf + hash_size, hash_size);\n \n@@ -983,17 +955,6 @@ static int null_streq(char *a, char *b)\n \treturn 0 == strcmp(a, b);\n }\n \n-static int zero_hash_eq(uint8_t *a, uint8_t *b, int sz)\n-{\n-\tif (!a)\n-\t\ta = zero;\n-\n-\tif (!b)\n-\t\tb = zero;\n-\n-\treturn !memcmp(a, b, sz);\n-}\n-\n static int reftable_log_record_equal_void(const void *a,\n \t\t\t\t\t  const void *b, int hash_size)\n {\n@@ -1037,10 +998,10 @@ int reftable_log_record_equal(const struct reftable_log_record *a,\n \t\t\t\t  b->value.update.email) &&\n \t\t       null_streq(a->value.update.message,\n \t\t\t\t  b->value.update.message) &&\n-\t\t       zero_hash_eq(a->value.update.old_hash,\n-\t\t\t\t    b->value.update.old_hash, hash_size) &&\n-\t\t       zero_hash_eq(a->value.update.new_hash,\n-\t\t\t\t    b->value.update.new_hash, hash_size);\n+\t\t       !memcmp(a->value.update.old_hash,\n+\t\t\t       b->value.update.old_hash, hash_size) &&\n+\t\t       !memcmp(a->value.update.new_hash,\n+\t\t\t       b->value.update.new_hash, hash_size);\n \t}\n \n \tabort();\ndiff --git a/reftable/record_test.c b/reftable/record_test.c\nindex 89209894d8..57e56138c0 100644\n--- a/reftable/record_test.c\n+++ b/reftable/record_test.c\n@@ -183,8 +183,6 @@ static void test_reftable_log_record_roundtrip(void)\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t\t.value = {\n \t\t\t\t.update = {\n-\t\t\t\t\t.old_hash = reftable_malloc(GIT_SHA1_RAWSZ),\n-\t\t\t\t\t.new_hash = reftable_malloc(GIT_SHA1_RAWSZ),\n \t\t\t\t\t.name = xstrdup(\"han-wen\"),\n \t\t\t\t\t.email = xstrdup(\"hanwen@google.com\"),\n \t\t\t\t\t.message = xstrdup(\"test\"),\n@@ -202,13 +200,6 @@ static void test_reftable_log_record_roundtrip(void)\n \t\t\t.refname = xstrdup(\"branch\"),\n \t\t\t.update_index = 33,\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n-\t\t\t.value = {\n-\t\t\t\t.update = {\n-\t\t\t\t\t.old_hash = reftable_malloc(GIT_SHA1_RAWSZ),\n-\t\t\t\t\t.new_hash = reftable_malloc(GIT_SHA1_RAWSZ),\n-\t\t\t\t\t/* rest of fields left empty. */\n-\t\t\t\t},\n-\t\t\t},\n \t\t}\n \t};\n \tset_test_hash(in[0].value.update.new_hash, 1);\n@@ -231,8 +222,6 @@ static void test_reftable_log_record_roundtrip(void)\n \t\t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t\t\t.value = {\n \t\t\t\t\t.update = {\n-\t\t\t\t\t\t.new_hash = reftable_calloc(GIT_SHA1_RAWSZ, 1),\n-\t\t\t\t\t\t.old_hash = reftable_calloc(GIT_SHA1_RAWSZ, 1),\n \t\t\t\t\t\t.name = xstrdup(\"old name\"),\n \t\t\t\t\t\t.email = xstrdup(\"old@email\"),\n \t\t\t\t\t\t.message = xstrdup(\"old message\"),\ndiff --git a/reftable/reftable-record.h b/reftable/reftable-record.h\nindex e657001d42..2659ea008c 100644\n--- a/reftable/reftable-record.h\n+++ b/reftable/reftable-record.h\n@@ -88,8 +88,8 @@ struct reftable_log_record {\n \n \tunion {\n \t\tstruct {\n-\t\t\tuint8_t *new_hash;\n-\t\t\tuint8_t *old_hash;\n+\t\t\tunsigned char new_hash[GIT_MAX_RAWSZ];\n+\t\t\tunsigned char old_hash[GIT_MAX_RAWSZ];\n \t\t\tchar *name;\n \t\t\tchar *email;\n \t\t\tuint64_t time;\ndiff --git a/reftable/stack_test.c b/reftable/stack_test.c\nindex 509f486623..7336757cf5 100644\n--- a/reftable/stack_test.c\n+++ b/reftable/stack_test.c\n@@ -468,8 +468,6 @@ static void test_reftable_stack_add(void)\n \t\tlogs[i].refname = xstrdup(buf);\n \t\tlogs[i].update_index = N + i + 1;\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n-\n-\t\tlogs[i].value.update.new_hash = reftable_malloc(GIT_SHA1_RAWSZ);\n \t\tlogs[i].value.update.email = xstrdup(\"identity@invalid\");\n \t\tset_test_hash(logs[i].value.update.new_hash, i);\n \t}\n@@ -547,16 +545,17 @@ static void test_reftable_stack_log_normalize(void)\n \t};\n \tstruct reftable_stack *st = NULL;\n \tchar *dir = get_tmp_dir(__LINE__);\n-\n-\tuint8_t h1[GIT_SHA1_RAWSZ] = { 0x01 }, h2[GIT_SHA1_RAWSZ] = { 0x02 };\n-\n-\tstruct reftable_log_record input = { .refname = \"branch\",\n-\t\t\t\t\t     .update_index = 1,\n-\t\t\t\t\t     .value_type = REFTABLE_LOG_UPDATE,\n-\t\t\t\t\t     .value = { .update = {\n-\t\t\t\t\t\t\t\t.new_hash = h1,\n-\t\t\t\t\t\t\t\t.old_hash = h2,\n-\t\t\t\t\t\t\t} } };\n+\tstruct reftable_log_record input = {\n+\t\t.refname = \"branch\",\n+\t\t.update_index = 1,\n+\t\t.value_type = REFTABLE_LOG_UPDATE,\n+\t\t.value = {\n+\t\t\t.update = {\n+\t\t\t\t.new_hash = { 1 },\n+\t\t\t\t.old_hash = { 2 },\n+\t\t\t},\n+\t\t},\n+\t};\n \tstruct reftable_log_record dest = {\n \t\t.update_index = 0,\n \t};\n@@ -627,8 +626,6 @@ static void test_reftable_stack_tombstone(void)\n \t\tlogs[i].update_index = 42;\n \t\tif (i % 2 == 0) {\n \t\t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n-\t\t\tlogs[i].value.update.new_hash =\n-\t\t\t\treftable_malloc(GIT_SHA1_RAWSZ);\n \t\t\tset_test_hash(logs[i].value.update.new_hash, i);\n \t\t\tlogs[i].value.update.email =\n \t\t\t\txstrdup(\"identity@invalid\");\n@@ -810,7 +807,6 @@ static void test_reflog_expire(void)\n \t\tlogs[i].update_index = i;\n \t\tlogs[i].value_type = REFTABLE_LOG_UPDATE;\n \t\tlogs[i].value.update.time = i;\n-\t\tlogs[i].value.update.new_hash = reftable_malloc(GIT_SHA1_RAWSZ);\n \t\tlogs[i].value.update.email = xstrdup(\"identity@invalid\");\n \t\tset_test_hash(logs[i].value.update.new_hash, i);\n \t}\n-- \n2.44.0\n\n"},{"id":"489955","messageId":"6f568e4ccb67a7af8279352153d052c5f9a88234.1709640322.git.ps@pks.im","threadId":"61053","inReplyTo":"cover.1709640322.git.ps@pks.im","subject":"[PATCH 3/7] reftable/record: avoid copying author info","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T12:11:03Z","receivedAt":"2024-03-05T12:11:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Each reflog entry contains information regarding the authorship of who\nhas made the change. This authorship information is not the same as that\nof any of the commits that the reflog entry references, but instead\ncorresponds to the local user that has executed the command. Thus, it is\nalmost always the case that all reflog entries have the same author.\n\nWe can make use of this fact when decoding reftable records: instead of\nfreeing and then reallocating the authorship information of log records,\nwe can special-case when the next record during an iteration has the\nexact same authorship as the preceding record. If so, then there is no\nneed to reallocate the respective fields.\n\nThis change results in two allocations less per log record that we're\niterating over in the most common case. Before:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 6,068,489 allocs, 6,068,367 frees, 361,011,822 bytes allocated\n\nAfter:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 4,068,487 allocs, 4,068,365 frees, 332,011,793 bytes allocated\n\nAn alternative would be to store the capacity of both name and email and\nthen use `REFTABLE_ALLOC_GROW()` to conditionally reallocate the array.\nBut reftable records are copied around quite a lot, and thus we need to\nbe a bit mindful of the overall record size. Furthermore, a memory\ncomparison should also be more efficient than having to copy over memory\neven if we wouldn't have to allocate a new array every time.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/record.c | 29 +++++++++++++++++++++--------\n 1 file changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 8d2cd6b081..d816de6d93 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -896,10 +896,19 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \t\tgoto done;\n \tstring_view_consume(&in, n);\n \n-\tr->value.update.name =\n-\t\treftable_realloc(r->value.update.name, dest.len + 1);\n-\tmemcpy(r->value.update.name, dest.buf, dest.len);\n-\tr->value.update.name[dest.len] = 0;\n+\t/*\n+\t * In almost all cases we can expect the reflog name to not change for\n+\t * reflog entries as they are tied to the local identity, not to the\n+\t * target commits. As an optimization for this common case we can thus\n+\t * skip copying over the name in case it's accurate already.\n+\t */\n+\tif (!r->value.update.name ||\n+\t    strcmp(r->value.update.name, dest.buf)) {\n+\t\tr->value.update.name =\n+\t\t\treftable_realloc(r->value.update.name, dest.len + 1);\n+\t\tmemcpy(r->value.update.name, dest.buf, dest.len);\n+\t\tr->value.update.name[dest.len] = 0;\n+\t}\n \n \tstrbuf_reset(&dest);\n \tn = decode_string(&dest, in);\n@@ -907,10 +916,14 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \t\tgoto done;\n \tstring_view_consume(&in, n);\n \n-\tr->value.update.email =\n-\t\treftable_realloc(r->value.update.email, dest.len + 1);\n-\tmemcpy(r->value.update.email, dest.buf, dest.len);\n-\tr->value.update.email[dest.len] = 0;\n+\t/* Same as above, but for the reflog email. */\n+\tif (!r->value.update.email ||\n+\t    strcmp(r->value.update.email, dest.buf)) {\n+\t\tr->value.update.email =\n+\t\t\treftable_realloc(r->value.update.email, dest.len + 1);\n+\t\tmemcpy(r->value.update.email, dest.buf, dest.len);\n+\t\tr->value.update.email[dest.len] = 0;\n+\t}\n \n \tts = 0;\n \tn = get_var_int(&ts, &in);\n-- \n2.44.0\n\n"},{"id":"489956","messageId":"f2b59e58a4e54e2a3d1df2c73c4f7acd92a57604.1709640322.git.ps@pks.im","threadId":"61053","inReplyTo":"cover.1709640322.git.ps@pks.im","subject":"[PATCH 4/7] reftable/record: reuse refnames when decoding log records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T12:11:07Z","receivedAt":"2024-03-05T12:11:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When decoding a log record we always reallocate their refname arrays.\nThis results in quite a lot of needless allocation churn.\n\nRefactor the code to grow the array as required only. Like this, we\nshould usually only end up reallocating the array a small handful of\ntimes when iterating over many refs. Before:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 4,068,487 allocs, 4,068,365 frees, 332,011,793 bytes allocated\n\nAfter:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 3,068,488 allocs, 3,068,366 frees, 307,122,961 bytes allocated\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/record.c          | 2 +-\n reftable/reftable-record.h | 1 +\n 2 files changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex d816de6d93..82780b2d06 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -861,7 +861,7 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \tif (key.len <= 9 || key.buf[key.len - 9] != 0)\n \t\treturn REFTABLE_FORMAT_ERROR;\n \n-\tr->refname = reftable_realloc(r->refname, key.len - 8);\n+\tREFTABLE_ALLOC_GROW(r->refname, key.len - 8, r->refname_cap);\n \tmemcpy(r->refname, key.buf, key.len - 8);\n \tts = get_be64(key.buf + key.len - 8);\n \ndiff --git a/reftable/reftable-record.h b/reftable/reftable-record.h\nindex 2659ea008c..d28f38175c 100644\n--- a/reftable/reftable-record.h\n+++ b/reftable/reftable-record.h\n@@ -74,6 +74,7 @@ int reftable_ref_record_equal(const struct reftable_ref_record *a,\n /* reftable_log_record holds a reflog entry */\n struct reftable_log_record {\n \tchar *refname;\n+\tsize_t refname_cap;\n \tuint64_t update_index; /* logical timestamp of a transactional update.\n \t\t\t\t*/\n \n-- \n2.44.0\n\n"},{"id":"489957","messageId":"b71be7c8ce59c00c638486dfb2cb1c7866e195cd.1709640322.git.ps@pks.im","threadId":"61053","inReplyTo":"cover.1709640322.git.ps@pks.im","subject":"[PATCH 5/7] reftable/record: reuse message when decoding log records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T12:11:12Z","receivedAt":"2024-03-05T12:11:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Same as the preceding commit we can allocate log messages as needed when\ndecoding log records, thus further reducing the number of allocations.\nBefore:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 3,068,488 allocs, 3,068,366 frees, 307,122,961 bytes allocated\n\nAfter:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 2,068,487 allocs, 2,068,365 frees, 305,122,946 bytes allocated\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/record.c          | 5 +++--\n reftable/reftable-record.h | 1 +\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 82780b2d06..7c86877586 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -871,6 +871,7 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \t\tswitch (r->value_type) {\n \t\tcase REFTABLE_LOG_UPDATE:\n \t\t\tFREE_AND_NULL(r->value.update.message);\n+\t\t\tr->value.update.message_cap = 0;\n \t\t\tFREE_AND_NULL(r->value.update.email);\n \t\t\tFREE_AND_NULL(r->value.update.name);\n \t\t\tbreak;\n@@ -943,8 +944,8 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \t\tgoto done;\n \tstring_view_consume(&in, n);\n \n-\tr->value.update.message =\n-\t\treftable_realloc(r->value.update.message, dest.len + 1);\n+\tREFTABLE_ALLOC_GROW(r->value.update.message, dest.len + 1,\n+\t\t\t    r->value.update.message_cap);\n \tmemcpy(r->value.update.message, dest.buf, dest.len);\n \tr->value.update.message[dest.len] = 0;\n \ndiff --git a/reftable/reftable-record.h b/reftable/reftable-record.h\nindex d28f38175c..2a2943cd13 100644\n--- a/reftable/reftable-record.h\n+++ b/reftable/reftable-record.h\n@@ -96,6 +96,7 @@ struct reftable_log_record {\n \t\t\tuint64_t time;\n \t\t\tint16_t tz_offset;\n \t\t\tchar *message;\n+\t\t\tsize_t message_cap;\n \t\t} update;\n \t} value;\n };\n-- \n2.44.0\n\n"},{"id":"489958","messageId":"e0a9057593f761917a3e6fb9a77045400e2b9d1c.1709640322.git.ps@pks.im","threadId":"61053","inReplyTo":"cover.1709640322.git.ps@pks.im","subject":"[PATCH 6/7] reftable/record: use scratch buffer when decoding records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T12:11:16Z","receivedAt":"2024-03-05T12:11:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When decoding log records we need a temporary buffer to decode the\nreflog entry's name, mail address and message. As this buffer is local\nto the function we thus have to reallocate it for every single log\nrecord which we're about to decode, which is inefficient.\n\nRefactor the code such that callers need to pass in a scratch buffer,\nwhich allows us to reuse it for multiple decodes. This reduces the\nnumber of allocations when iterating through reflogs. Before:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 2,068,487 allocs, 2,068,365 frees, 305,122,946 bytes allocated\n\nAfter:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 1,068,485 allocs, 1,068,363 frees, 281,122,886 bytes allocated\n\nNote that this commit also drop some redundant calls to `strbuf_reset()`\nright before calling `decode_string()`. The latter already knows to\nreset the buffer, so there is no need for these.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/block.c       |  4 ++-\n reftable/block.h       |  2 ++\n reftable/record.c      | 52 ++++++++++++++++++--------------------\n reftable/record.h      |  5 ++--\n reftable/record_test.c | 57 ++++++++++++++++++++++++++----------------\n 5 files changed, 68 insertions(+), 52 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex ad9074dba6..b67300a734 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -332,7 +332,8 @@ int block_iter_next(struct block_iter *it, struct reftable_record *rec)\n \t\treturn REFTABLE_FORMAT_ERROR;\n \n \tstring_view_consume(&in, n);\n-\tn = reftable_record_decode(rec, it->last_key, extra, in, it->br->hash_size);\n+\tn = reftable_record_decode(rec, it->last_key, extra, in, it->br->hash_size,\n+\t\t\t\t   &it->scratch);\n \tif (n < 0)\n \t\treturn -1;\n \tstring_view_consume(&in, n);\n@@ -369,6 +370,7 @@ int block_iter_seek(struct block_iter *it, struct strbuf *want)\n void block_iter_close(struct block_iter *it)\n {\n \tstrbuf_release(&it->last_key);\n+\tstrbuf_release(&it->scratch);\n }\n \n int block_reader_seek(struct block_reader *br, struct block_iter *it,\ndiff --git a/reftable/block.h b/reftable/block.h\nindex 51699af233..47acc62c0a 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -84,10 +84,12 @@ struct block_iter {\n \n \t/* key for last entry we read. */\n \tstruct strbuf last_key;\n+\tstruct strbuf scratch;\n };\n \n #define BLOCK_ITER_INIT { \\\n \t.last_key = STRBUF_INIT, \\\n+\t.scratch = STRBUF_INIT, \\\n }\n \n /* initializes a block reader. */\ndiff --git a/reftable/record.c b/reftable/record.c\nindex 7c86877586..060244337f 100644\n--- a/reftable/record.c\n+++ b/reftable/record.c\n@@ -374,7 +374,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n \n static int reftable_ref_record_decode(void *rec, struct strbuf key,\n \t\t\t\t      uint8_t val_type, struct string_view in,\n-\t\t\t\t      int hash_size)\n+\t\t\t\t      int hash_size, struct strbuf *scratch)\n {\n \tstruct reftable_ref_record *r = rec;\n \tstruct string_view start = in;\n@@ -425,13 +425,12 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,\n \t\tbreak;\n \n \tcase REFTABLE_REF_SYMREF: {\n-\t\tstruct strbuf dest = STRBUF_INIT;\n-\t\tint n = decode_string(&dest, in);\n+\t\tint n = decode_string(scratch, in);\n \t\tif (n < 0) {\n \t\t\treturn -1;\n \t\t}\n \t\tstring_view_consume(&in, n);\n-\t\tr->value.symref = dest.buf;\n+\t\tr->value.symref = strbuf_detach(scratch, NULL);\n \t} break;\n \n \tcase REFTABLE_REF_DELETION:\n@@ -579,7 +578,7 @@ static int reftable_obj_record_encode(const void *rec, struct string_view s,\n \n static int reftable_obj_record_decode(void *rec, struct strbuf key,\n \t\t\t\t      uint8_t val_type, struct string_view in,\n-\t\t\t\t      int hash_size)\n+\t\t\t\t      int hash_size, struct strbuf *scratch UNUSED)\n {\n \tstruct string_view start = in;\n \tstruct reftable_obj_record *r = rec;\n@@ -849,13 +848,12 @@ static int reftable_log_record_encode(const void *rec, struct string_view s,\n \n static int reftable_log_record_decode(void *rec, struct strbuf key,\n \t\t\t\t      uint8_t val_type, struct string_view in,\n-\t\t\t\t      int hash_size)\n+\t\t\t\t      int hash_size, struct strbuf *scratch)\n {\n \tstruct string_view start = in;\n \tstruct reftable_log_record *r = rec;\n \tuint64_t max = 0;\n \tuint64_t ts = 0;\n-\tstruct strbuf dest = STRBUF_INIT;\n \tint n;\n \n \tif (key.len <= 9 || key.buf[key.len - 9] != 0)\n@@ -892,7 +890,7 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \n \tstring_view_consume(&in, 2 * hash_size);\n \n-\tn = decode_string(&dest, in);\n+\tn = decode_string(scratch, in);\n \tif (n < 0)\n \t\tgoto done;\n \tstring_view_consume(&in, n);\n@@ -904,26 +902,25 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \t * skip copying over the name in case it's accurate already.\n \t */\n \tif (!r->value.update.name ||\n-\t    strcmp(r->value.update.name, dest.buf)) {\n+\t    strcmp(r->value.update.name, scratch->buf)) {\n \t\tr->value.update.name =\n-\t\t\treftable_realloc(r->value.update.name, dest.len + 1);\n-\t\tmemcpy(r->value.update.name, dest.buf, dest.len);\n-\t\tr->value.update.name[dest.len] = 0;\n+\t\t\treftable_realloc(r->value.update.name, scratch->len + 1);\n+\t\tmemcpy(r->value.update.name, scratch->buf, scratch->len);\n+\t\tr->value.update.name[scratch->len] = 0;\n \t}\n \n-\tstrbuf_reset(&dest);\n-\tn = decode_string(&dest, in);\n+\tn = decode_string(scratch, in);\n \tif (n < 0)\n \t\tgoto done;\n \tstring_view_consume(&in, n);\n \n \t/* Same as above, but for the reflog email. */\n \tif (!r->value.update.email ||\n-\t    strcmp(r->value.update.email, dest.buf)) {\n+\t    strcmp(r->value.update.email, scratch->buf)) {\n \t\tr->value.update.email =\n-\t\t\treftable_realloc(r->value.update.email, dest.len + 1);\n-\t\tmemcpy(r->value.update.email, dest.buf, dest.len);\n-\t\tr->value.update.email[dest.len] = 0;\n+\t\t\treftable_realloc(r->value.update.email, scratch->len + 1);\n+\t\tmemcpy(r->value.update.email, scratch->buf, scratch->len);\n+\t\tr->value.update.email[scratch->len] = 0;\n \t}\n \n \tts = 0;\n@@ -938,22 +935,19 @@ static int reftable_log_record_decode(void *rec, struct strbuf key,\n \tr->value.update.tz_offset = get_be16(in.buf);\n \tstring_view_consume(&in, 2);\n \n-\tstrbuf_reset(&dest);\n-\tn = decode_string(&dest, in);\n+\tn = decode_string(scratch, in);\n \tif (n < 0)\n \t\tgoto done;\n \tstring_view_consume(&in, n);\n \n-\tREFTABLE_ALLOC_GROW(r->value.update.message, dest.len + 1,\n+\tREFTABLE_ALLOC_GROW(r->value.update.message, scratch->len + 1,\n \t\t\t    r->value.update.message_cap);\n-\tmemcpy(r->value.update.message, dest.buf, dest.len);\n-\tr->value.update.message[dest.len] = 0;\n+\tmemcpy(r->value.update.message, scratch->buf, scratch->len);\n+\tr->value.update.message[scratch->len] = 0;\n \n-\tstrbuf_release(&dest);\n \treturn start.len - in.len;\n \n done:\n-\tstrbuf_release(&dest);\n \treturn REFTABLE_FORMAT_ERROR;\n }\n \n@@ -1093,7 +1087,7 @@ static int reftable_index_record_encode(const void *rec, struct string_view out,\n \n static int reftable_index_record_decode(void *rec, struct strbuf key,\n \t\t\t\t\tuint8_t val_type, struct string_view in,\n-\t\t\t\t\tint hash_size)\n+\t\t\t\t\tint hash_size, struct strbuf *scratch UNUSED)\n {\n \tstruct string_view start = in;\n \tstruct reftable_index_record *r = rec;\n@@ -1174,10 +1168,12 @@ uint8_t reftable_record_val_type(struct reftable_record *rec)\n }\n \n int reftable_record_decode(struct reftable_record *rec, struct strbuf key,\n-\t\t\t   uint8_t extra, struct string_view src, int hash_size)\n+\t\t\t   uint8_t extra, struct string_view src, int hash_size,\n+\t\t\t   struct strbuf *scratch)\n {\n \treturn reftable_record_vtable(rec)->decode(reftable_record_data(rec),\n-\t\t\t\t\t\t   key, extra, src, hash_size);\n+\t\t\t\t\t\t   key, extra, src, hash_size,\n+\t\t\t\t\t\t   scratch);\n }\n \n void reftable_record_release(struct reftable_record *rec)\ndiff --git a/reftable/record.h b/reftable/record.h\nindex 5e8304e052..826ee1c55c 100644\n--- a/reftable/record.h\n+++ b/reftable/record.h\n@@ -55,7 +55,8 @@ struct reftable_record_vtable {\n \n \t/* decode data from `src` into the record. */\n \tint (*decode)(void *rec, struct strbuf key, uint8_t extra,\n-\t\t      struct string_view src, int hash_size);\n+\t\t      struct string_view src, int hash_size,\n+\t\t      struct strbuf *scratch);\n \n \t/* deallocate and null the record. */\n \tvoid (*release)(void *rec);\n@@ -138,7 +139,7 @@ int reftable_record_encode(struct reftable_record *rec, struct string_view dest,\n \t\t\t   int hash_size);\n int reftable_record_decode(struct reftable_record *rec, struct strbuf key,\n \t\t\t   uint8_t extra, struct string_view src,\n-\t\t\t   int hash_size);\n+\t\t\t   int hash_size, struct strbuf *scratch);\n int reftable_record_is_deletion(struct reftable_record *rec);\n \n static inline uint8_t reftable_record_type(struct reftable_record *rec)\ndiff --git a/reftable/record_test.c b/reftable/record_test.c\nindex 57e56138c0..c158ee79ff 100644\n--- a/reftable/record_test.c\n+++ b/reftable/record_test.c\n@@ -99,6 +99,7 @@ static void set_hash(uint8_t *h, int j)\n \n static void test_reftable_ref_record_roundtrip(void)\n {\n+\tstruct strbuf scratch = STRBUF_INIT;\n \tint i = 0;\n \n \tfor (i = REFTABLE_REF_DELETION; i < REFTABLE_NR_REF_VALUETYPES; i++) {\n@@ -140,7 +141,7 @@ static void test_reftable_ref_record_roundtrip(void)\n \t\tEXPECT(n > 0);\n \n \t\t/* decode into a non-zero reftable_record to test for leaks. */\n-\t\tm = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ);\n+\t\tm = reftable_record_decode(&out, key, i, dest, GIT_SHA1_RAWSZ, &scratch);\n \t\tEXPECT(n == m);\n \n \t\tEXPECT(reftable_ref_record_equal(&in.u.ref, &out.u.ref,\n@@ -150,6 +151,8 @@ static void test_reftable_ref_record_roundtrip(void)\n \t\tstrbuf_release(&key);\n \t\treftable_record_release(&out);\n \t}\n+\n+\tstrbuf_release(&scratch);\n }\n \n static void test_reftable_log_record_equal(void)\n@@ -175,7 +178,6 @@ static void test_reftable_log_record_equal(void)\n static void test_reftable_log_record_roundtrip(void)\n {\n \tint i;\n-\n \tstruct reftable_log_record in[] = {\n \t\t{\n \t\t\t.refname = xstrdup(\"refs/heads/master\"),\n@@ -202,6 +204,8 @@ static void test_reftable_log_record_roundtrip(void)\n \t\t\t.value_type = REFTABLE_LOG_UPDATE,\n \t\t}\n \t};\n+\tstruct strbuf scratch = STRBUF_INIT;\n+\n \tset_test_hash(in[0].value.update.new_hash, 1);\n \tset_test_hash(in[0].value.update.old_hash, 2);\n \tset_test_hash(in[2].value.update.new_hash, 3);\n@@ -241,7 +245,7 @@ static void test_reftable_log_record_roundtrip(void)\n \t\tEXPECT(n >= 0);\n \t\tvaltype = reftable_record_val_type(&rec);\n \t\tm = reftable_record_decode(&out, key, valtype, dest,\n-\t\t\t\t\t   GIT_SHA1_RAWSZ);\n+\t\t\t\t\t   GIT_SHA1_RAWSZ, &scratch);\n \t\tEXPECT(n == m);\n \n \t\tEXPECT(reftable_log_record_equal(&in[i], &out.u.log,\n@@ -250,6 +254,8 @@ static void test_reftable_log_record_roundtrip(void)\n \t\tstrbuf_release(&key);\n \t\treftable_record_release(&out);\n \t}\n+\n+\tstrbuf_release(&scratch);\n }\n \n static void test_u24_roundtrip(void)\n@@ -299,23 +305,27 @@ static void test_reftable_obj_record_roundtrip(void)\n {\n \tuint8_t testHash1[GIT_SHA1_RAWSZ] = { 1, 2, 3, 4, 0 };\n \tuint64_t till9[] = { 1, 2, 3, 4, 500, 600, 700, 800, 9000 };\n-\tstruct reftable_obj_record recs[3] = { {\n-\t\t\t\t\t\t       .hash_prefix = testHash1,\n-\t\t\t\t\t\t       .hash_prefix_len = 5,\n-\t\t\t\t\t\t       .offsets = till9,\n-\t\t\t\t\t\t       .offset_len = 3,\n-\t\t\t\t\t       },\n-\t\t\t\t\t       {\n-\t\t\t\t\t\t       .hash_prefix = testHash1,\n-\t\t\t\t\t\t       .hash_prefix_len = 5,\n-\t\t\t\t\t\t       .offsets = till9,\n-\t\t\t\t\t\t       .offset_len = 9,\n-\t\t\t\t\t       },\n-\t\t\t\t\t       {\n-\t\t\t\t\t\t       .hash_prefix = testHash1,\n-\t\t\t\t\t\t       .hash_prefix_len = 5,\n-\t\t\t\t\t       } };\n+\tstruct reftable_obj_record recs[3] = {\n+\t\t{\n+\t\t\t.hash_prefix = testHash1,\n+\t\t\t.hash_prefix_len = 5,\n+\t\t\t.offsets = till9,\n+\t\t\t.offset_len = 3,\n+\t\t},\n+\t\t{\n+\t\t\t.hash_prefix = testHash1,\n+\t\t\t.hash_prefix_len = 5,\n+\t\t\t.offsets = till9,\n+\t\t\t.offset_len = 9,\n+\t\t},\n+\t\t{\n+\t\t\t.hash_prefix = testHash1,\n+\t\t\t.hash_prefix_len = 5,\n+\t\t},\n+\t};\n+\tstruct strbuf scratch = STRBUF_INIT;\n \tint i = 0;\n+\n \tfor (i = 0; i < ARRAY_SIZE(recs); i++) {\n \t\tuint8_t buffer[1024] = { 0 };\n \t\tstruct string_view dest = {\n@@ -339,13 +349,15 @@ static void test_reftable_obj_record_roundtrip(void)\n \t\tEXPECT(n > 0);\n \t\textra = reftable_record_val_type(&in);\n \t\tm = reftable_record_decode(&out, key, extra, dest,\n-\t\t\t\t\t   GIT_SHA1_RAWSZ);\n+\t\t\t\t\t   GIT_SHA1_RAWSZ, &scratch);\n \t\tEXPECT(n == m);\n \n \t\tEXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));\n \t\tstrbuf_release(&key);\n \t\treftable_record_release(&out);\n \t}\n+\n+\tstrbuf_release(&scratch);\n }\n \n static void test_reftable_index_record_roundtrip(void)\n@@ -362,6 +374,7 @@ static void test_reftable_index_record_roundtrip(void)\n \t\t.buf = buffer,\n \t\t.len = sizeof(buffer),\n \t};\n+\tstruct strbuf scratch = STRBUF_INIT;\n \tstruct strbuf key = STRBUF_INIT;\n \tstruct reftable_record out = {\n \t\t.type = BLOCK_TYPE_INDEX,\n@@ -379,13 +392,15 @@ static void test_reftable_index_record_roundtrip(void)\n \tEXPECT(n > 0);\n \n \textra = reftable_record_val_type(&in);\n-\tm = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ);\n+\tm = reftable_record_decode(&out, key, extra, dest, GIT_SHA1_RAWSZ,\n+\t\t\t\t   &scratch);\n \tEXPECT(m == n);\n \n \tEXPECT(reftable_record_equal(&in, &out, GIT_SHA1_RAWSZ));\n \n \treftable_record_release(&out);\n \tstrbuf_release(&key);\n+\tstrbuf_release(&scratch);\n \tstrbuf_release(&in.u.idx.last_key);\n }\n \n-- \n2.44.0\n\n"},{"id":"489959","messageId":"f56dd2177e42927dd4cfa88c2adc344f42dbefac.1709640322.git.ps@pks.im","threadId":"61053","inReplyTo":"cover.1709640322.git.ps@pks.im","subject":"[PATCH 7/7] refs/reftable: track last log record name via strbuf","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-05T12:11:20Z","receivedAt":"2024-03-05T12:11:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The reflog iterator enumerates all reflogs known to a ref backend. In\nthe \"reftable\" backend there is no way to list all existing reflogs\ndirectly. Instead, we have to iterate through all reflog entries and\ndiscard all those redundant entries for which we have already returned a\nreflog entry.\n\nThis logic is implemented by tracking the last reflog name that we have\nemitted to the iterator's user. If the next log record has the same name\nwe simply skip it until we find another record with a different refname.\n\nThis last reflog name is stored in a simple C string, which requires us\nto free and reallocate it whenever we need to update the reflog name.\nConvert it to use a `struct strbuf` instead, which reduces the number of\nallocations. Before:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 1,068,485 allocs, 1,068,363 frees, 281,122,886 bytes allocated\n\nAfter:\n\n    HEAP SUMMARY:\n        in use at exit: 13,473 bytes in 122 blocks\n      total heap usage: 68,485 allocs, 68,363 frees, 256,234,072 bytes allocated\n\nNote that even after this change we still allocate quite a lot of data,\neven though the number of allocations does not scale with the number of\nlog records anymore. This remainder comes mostly from decompressing the\nlog blocks, where we decompress each block into newly allocated memory.\nThis will be addressed at a later point in time.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n refs/reftable-backend.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 2de57c047a..12960d93ff 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1575,7 +1575,7 @@ struct reftable_reflog_iterator {\n \tstruct reftable_ref_store *refs;\n \tstruct reftable_iterator iter;\n \tstruct reftable_log_record log;\n-\tchar *last_name;\n+\tstruct strbuf last_name;\n \tint err;\n };\n \n@@ -1594,15 +1594,15 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)\n \t\t * we've already produced this name. This could be faster by\n \t\t * seeking directly to reflog@update_index==0.\n \t\t */\n-\t\tif (iter->last_name && !strcmp(iter->log.refname, iter->last_name))\n+\t\tif (!strcmp(iter->log.refname, iter->last_name.buf))\n \t\t\tcontinue;\n \n \t\tif (check_refname_format(iter->log.refname,\n \t\t\t\t\t REFNAME_ALLOW_ONELEVEL))\n \t\t\tcontinue;\n \n-\t\tfree(iter->last_name);\n-\t\titer->last_name = xstrdup(iter->log.refname);\n+\t\tstrbuf_reset(&iter->last_name);\n+\t\tstrbuf_addstr(&iter->last_name, iter->log.refname);\n \t\titer->base.refname = iter->log.refname;\n \n \t\tbreak;\n@@ -1635,7 +1635,7 @@ static int reftable_reflog_iterator_abort(struct ref_iterator *ref_iterator)\n \t\t(struct reftable_reflog_iterator *)ref_iterator;\n \treftable_log_record_release(&iter->log);\n \treftable_iterator_destroy(&iter->iter);\n-\tfree(iter->last_name);\n+\tstrbuf_release(&iter->last_name);\n \tfree(iter);\n \treturn ITER_DONE;\n }\n@@ -1655,6 +1655,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n \n \titer = xcalloc(1, sizeof(*iter));\n \tbase_ref_iterator_init(&iter->base, &reftable_reflog_iterator_vtable);\n+\tstrbuf_init(&iter->last_name, 0);\n \titer->refs = refs;\n \n \tret = refs->err;\n-- \n2.44.0\n\n"},{"id":"490094","messageId":"CAOLa=ZRQ6sYLBqjWiMXGS7rcv8pJ7jSEneiJ8ZcHtALrrcwMeg@mail.gmail.com","threadId":"61053","inReplyTo":"b0414221ecad1920c84f4ab498e55edec57f06b6.1709640322.git.ps@pks.im","subject":"Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-03-06T16:13:39Z","receivedAt":"2024-03-06T16:13:42Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When creating a new reflog iterator, we first have to reload the stack\n> that the iterator is being created. This is done so that any concurrent\n\nNit: s/created./created for.\n"},{"id":"490106","messageId":"xmqqr0gnqmhl.fsf@gitster.g","threadId":"61053","inReplyTo":"CAOLa=ZRQ6sYLBqjWiMXGS7rcv8pJ7jSEneiJ8ZcHtALrrcwMeg@mail.gmail.com","subject":"Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T17:49:26Z","receivedAt":"2024-03-06T17:49:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>> When creating a new reflog iterator, we first have to reload the stack\n>> that the iterator is being created. This is done so that any concurrent\n>\n> Nit: s/created./created for.\n\nYeah, I couldn't grok that sentence while reviewing it.\n\nThanks.\n"},{"id":"490134","messageId":"ZelYDafU7fExs0NP@tanuki","threadId":"61053","inReplyTo":"CAOLa=ZRQ6sYLBqjWiMXGS7rcv8pJ7jSEneiJ8ZcHtALrrcwMeg@mail.gmail.com","subject":"Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-07T06:00:45Z","receivedAt":"2024-03-07T06:00:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 06, 2024 at 08:13:39AM -0800, Karthik Nayak wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > When creating a new reflog iterator, we first have to reload the stack\n> > that the iterator is being created. This is done so that any concurrent\n> \n> Nit: s/created./created for.\n\nIndeed, thanks. I've fixed this locally and will wait for more comments\nbefore sending a v2.\n\nPatrick\n"},{"id":"490380","messageId":"Ze9Or6vctPJGOV_l@google.com","threadId":"61053","inReplyTo":"b0414221ecad1920c84f4ab498e55edec57f06b6.1709640322.git.ps@pks.im","subject":"Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-11T18:34:23Z","receivedAt":"2024-03-11T18:34:36Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.05 13:10, Patrick Steinhardt wrote:\n> When creating a new reflog iterator, we first have to reload the stack\n> that the iterator is being created. This is done so that any concurrent\n> writes to the stack are reflected. But `reflog_iterator_for_stack()`\n> always reloads the main stack, which is wrong.\n> \n> Fix this and reload the correct stack.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  refs/reftable-backend.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 249a618b5a..f04be942ac 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1682,7 +1682,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n>  \tif (ret)\n>  \t\tgoto done;\n>  \n> -\tret = reftable_stack_reload(refs->main_stack);\n> +\tret = reftable_stack_reload(stack);\n>  \tif (ret < 0)\n>  \t\tgoto done;\n>  \n> -- \n> 2.44.0\n> \n\nIs it possible to write a test to demonstrate the bug that was fixed\nhere, or is it too much of a race condition to reliably trigger?\n"},{"id":"490392","messageId":"Ze9cLWCZoBqcfr2B@google.com","threadId":"61053","inReplyTo":"e0a9057593f761917a3e6fb9a77045400e2b9d1c.1709640322.git.ps@pks.im","subject":"Re: [PATCH 6/7] reftable/record: use scratch buffer when decoding records","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-11T19:31:57Z","receivedAt":"2024-03-11T19:32:03Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.05 13:11, Patrick Steinhardt wrote:\n> When decoding log records we need a temporary buffer to decode the\n> reflog entry's name, mail address and message. As this buffer is local\n> to the function we thus have to reallocate it for every single log\n> record which we're about to decode, which is inefficient.\n> \n> Refactor the code such that callers need to pass in a scratch buffer,\n> which allows us to reuse it for multiple decodes. This reduces the\n> number of allocations when iterating through reflogs. Before:\n> \n>     HEAP SUMMARY:\n>         in use at exit: 13,473 bytes in 122 blocks\n>       total heap usage: 2,068,487 allocs, 2,068,365 frees, 305,122,946 bytes allocated\n> \n> After:\n> \n>     HEAP SUMMARY:\n>         in use at exit: 13,473 bytes in 122 blocks\n>       total heap usage: 1,068,485 allocs, 1,068,363 frees, 281,122,886 bytes allocated\n> \n> Note that this commit also drop some redundant calls to `strbuf_reset()`\n> right before calling `decode_string()`. The latter already knows to\n> reset the buffer, so there is no need for these.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/block.c       |  4 ++-\n>  reftable/block.h       |  2 ++\n>  reftable/record.c      | 52 ++++++++++++++++++--------------------\n>  reftable/record.h      |  5 ++--\n>  reftable/record_test.c | 57 ++++++++++++++++++++++++++----------------\n>  5 files changed, 68 insertions(+), 52 deletions(-)\n\n[snip]\n\n> diff --git a/reftable/record.c b/reftable/record.c\n> index 7c86877586..060244337f 100644\n> --- a/reftable/record.c\n> +++ b/reftable/record.c\n> @@ -374,7 +374,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n>  \n>  static int reftable_ref_record_decode(void *rec, struct strbuf key,\n>  \t\t\t\t      uint8_t val_type, struct string_view in,\n> -\t\t\t\t      int hash_size)\n> +\t\t\t\t      int hash_size, struct strbuf *scratch)\n>  {\n>  \tstruct reftable_ref_record *r = rec;\n>  \tstruct string_view start = in;\n> @@ -425,13 +425,12 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,\n>  \t\tbreak;\n>  \n>  \tcase REFTABLE_REF_SYMREF: {\n> -\t\tstruct strbuf dest = STRBUF_INIT;\n> -\t\tint n = decode_string(&dest, in);\n> +\t\tint n = decode_string(scratch, in);\n>  \t\tif (n < 0) {\n>  \t\t\treturn -1;\n>  \t\t}\n>  \t\tstring_view_consume(&in, n);\n> -\t\tr->value.symref = dest.buf;\n> +\t\tr->value.symref = strbuf_detach(scratch, NULL);\n>  \t} break;\n\nI had to dig into this to convince myself that we aren't leaking memory\nhere, but IIUC this gets cleaned up eventually by\nreftable_ref_record_release(), right?\n"},{"id":"490393","messageId":"Ze9eFkJqu4cqePPU@google.com","threadId":"61053","inReplyTo":"e0a9057593f761917a3e6fb9a77045400e2b9d1c.1709640322.git.ps@pks.im","subject":"Re: [PATCH 6/7] reftable/record: use scratch buffer when decoding records","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-11T19:40:06Z","receivedAt":"2024-03-11T19:40:12Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.05 13:11, Patrick Steinhardt wrote:\n> When decoding log records we need a temporary buffer to decode the\n> reflog entry's name, mail address and message. As this buffer is local\n> to the function we thus have to reallocate it for every single log\n> record which we're about to decode, which is inefficient.\n> \n> Refactor the code such that callers need to pass in a scratch buffer,\n> which allows us to reuse it for multiple decodes. This reduces the\n> number of allocations when iterating through reflogs. Before:\n> \n>     HEAP SUMMARY:\n>         in use at exit: 13,473 bytes in 122 blocks\n>       total heap usage: 2,068,487 allocs, 2,068,365 frees, 305,122,946 bytes allocated\n> \n> After:\n> \n>     HEAP SUMMARY:\n>         in use at exit: 13,473 bytes in 122 blocks\n>       total heap usage: 1,068,485 allocs, 1,068,363 frees, 281,122,886 bytes allocated\n> \n> Note that this commit also drop some redundant calls to `strbuf_reset()`\n> right before calling `decode_string()`. The latter already knows to\n> reset the buffer, so there is no need for these.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  reftable/block.c       |  4 ++-\n>  reftable/block.h       |  2 ++\n>  reftable/record.c      | 52 ++++++++++++++++++--------------------\n>  reftable/record.h      |  5 ++--\n>  reftable/record_test.c | 57 ++++++++++++++++++++++++++----------------\n>  5 files changed, 68 insertions(+), 52 deletions(-)\n\nAt first glance I was feeling somewhat negatively about this change, as\nkeeping persistent scratch space buffers means that either the owners\nor the users of the scratch space need to be more careful about making\nsure it's reset and that they're not accumulating cruft in between\nvarious calls.\n\nHowever, it appears we already have similar scratch space usage in the\nsideband and cat-file code, so I guess we are OK with the pattern in\ngeneral, and we can rely on tests to make sure things are good in\npractice.\n"},{"id":"490394","messageId":"Ze9eX-aaWoVaqsPP@google.com","threadId":"61053","inReplyTo":"cover.1709640322.git.ps@pks.im","subject":"Re: [PATCH 0/7] reftable: memory optimizations for reflog iteration","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2024-03-11T19:41:19Z","receivedAt":"2024-03-11T19:41:25Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2024.03.05 13:10, Patrick Steinhardt wrote:\n> Hi,\n> \n> this patch series does the same as all the preceding patch series that\n> optimized how the reftable library iterates through refs, but for\n> reflogs instead.\n> \n> The goal of this patch series is to arrive at a constant number of\n> allocations when iterating refs. This is achieved in mostly the same way\n> we did it for ref iteration, namely by reusing already-allocated memory.\n> Overall, this brings us down from 8 allocations per reflog record to\n> essentially 0 allocations per reflog. Iterating through 1 million\n> reflogs with `git reflog list` thus goes down from 8.068m allocations to\n> only around 68.5k.\n> \n> This series is built on top of \"master\" at b387623c12 (The third batch,\n> 2024-03-01) with Junio's \"ps/reftable-iteration-perf-part2\" at\n> 43f70eaea0 (refs/reftable: precompute prefix length, 2024-03-04) merged\n> into it.\n> \n> Patrick\n> \n> Patrick Steinhardt (7):\n>   refs/reftable: reload correct stack when creating reflog iter\n>   reftable/record: convert old and new object IDs to arrays\n>   reftable/record: avoid copying author info\n>   reftable/record: reuse refnames when decoding log records\n>   reftable/record: reuse message when decoding log records\n>   reftable/record: use scratch buffer when decoding records\n>   refs/reftable: track last log record name via strbuf\n> \n>  refs/reftable-backend.c    |  52 +++++----------\n>  reftable/block.c           |   4 +-\n>  reftable/block.h           |   2 +\n>  reftable/merged_test.c     |  11 ++--\n>  reftable/readwrite_test.c  |  62 +++++++-----------\n>  reftable/record.c          | 129 ++++++++++++++-----------------------\n>  reftable/record.h          |   5 +-\n>  reftable/record_test.c     |  68 ++++++++++---------\n>  reftable/reftable-record.h |   6 +-\n>  reftable/stack_test.c      |  26 ++++----\n>  10 files changed, 154 insertions(+), 211 deletions(-)\n> \n> -- \n> 2.44.0\n> \n\nThis series looks good to me (with one request that we add a test for\npatch #1 if possible).\n\nReviewed-by: Josh Steadmon <steadmon@google.com>\n"},{"id":"490430","messageId":"Ze-SnjgaLNoMvzW5@tanuki","threadId":"61053","inReplyTo":"Ze9Or6vctPJGOV_l@google.com","subject":"Re: [PATCH 1/7] refs/reftable: reload correct stack when creating reflog iter","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-11T23:24:14Z","receivedAt":"2024-03-11T23:24:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 11, 2024 at 11:34:23AM -0700, Josh Steadmon wrote:\n> On 2024.03.05 13:10, Patrick Steinhardt wrote:\n> > When creating a new reflog iterator, we first have to reload the stack\n> > that the iterator is being created. This is done so that any concurrent\n> > writes to the stack are reflected. But `reflog_iterator_for_stack()`\n> > always reloads the main stack, which is wrong.\n> > \n> > Fix this and reload the correct stack.\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  refs/reftable-backend.c | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n> > \n> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> > index 249a618b5a..f04be942ac 100644\n> > --- a/refs/reftable-backend.c\n> > +++ b/refs/reftable-backend.c\n> > @@ -1682,7 +1682,7 @@ static struct reftable_reflog_iterator *reflog_iterator_for_stack(struct reftabl\n> >  \tif (ret)\n> >  \t\tgoto done;\n> >  \n> > -\tret = reftable_stack_reload(refs->main_stack);\n> > +\tret = reftable_stack_reload(stack);\n> >  \tif (ret < 0)\n> >  \t\tgoto done;\n> >  \n> > -- \n> > 2.44.0\n> > \n> \n> Is it possible to write a test to demonstrate the bug that was fixed\n> here, or is it too much of a race condition to reliably trigger?\n\nI wouldn't really know how to test for this in a way that is even\nsomewhat reliably, unfortunately. You have to have at least two\nconcurrent commands, one reading and one writing, where the first\ncommand loads the worktree stack and then tries to create an iter as\nabove.\n\nNow if this was part of the reftable library we could do it rather\neasily via t0032, which contains unit tests for the reftable library.\nBut in the reftable backend it's much harder.\n\nPatrick\n"},{"id":"490431","messageId":"Ze-SzWD7p4hwz8nG@tanuki","threadId":"61053","inReplyTo":"Ze9cLWCZoBqcfr2B@google.com","subject":"Re: [PATCH 6/7] reftable/record: use scratch buffer when decoding records","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-11T23:25:01Z","receivedAt":"2024-03-11T23:25:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 11, 2024 at 12:31:57PM -0700, Josh Steadmon wrote:\n> On 2024.03.05 13:11, Patrick Steinhardt wrote:\n[snip]\n> > diff --git a/reftable/record.c b/reftable/record.c\n> > index 7c86877586..060244337f 100644\n> > --- a/reftable/record.c\n> > +++ b/reftable/record.c\n> > @@ -374,7 +374,7 @@ static int reftable_ref_record_encode(const void *rec, struct string_view s,\n> >  \n> >  static int reftable_ref_record_decode(void *rec, struct strbuf key,\n> >  \t\t\t\t      uint8_t val_type, struct string_view in,\n> > -\t\t\t\t      int hash_size)\n> > +\t\t\t\t      int hash_size, struct strbuf *scratch)\n> >  {\n> >  \tstruct reftable_ref_record *r = rec;\n> >  \tstruct string_view start = in;\n> > @@ -425,13 +425,12 @@ static int reftable_ref_record_decode(void *rec, struct strbuf key,\n> >  \t\tbreak;\n> >  \n> >  \tcase REFTABLE_REF_SYMREF: {\n> > -\t\tstruct strbuf dest = STRBUF_INIT;\n> > -\t\tint n = decode_string(&dest, in);\n> > +\t\tint n = decode_string(scratch, in);\n> >  \t\tif (n < 0) {\n> >  \t\t\treturn -1;\n> >  \t\t}\n> >  \t\tstring_view_consume(&in, n);\n> > -\t\tr->value.symref = dest.buf;\n> > +\t\tr->value.symref = strbuf_detach(scratch, NULL);\n> >  \t} break;\n> \n> I had to dig into this to convince myself that we aren't leaking memory\n> here, but IIUC this gets cleaned up eventually by\n> reftable_ref_record_release(), right?\n\nYes, exactly.\n\nPatrick\n"},{"id":"490432","messageId":"Ze-S99lx6GWXe210@tanuki","threadId":"61053","inReplyTo":"Ze9eX-aaWoVaqsPP@google.com","subject":"Re: [PATCH 0/7] reftable: memory optimizations for reflog iteration","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-11T23:25:43Z","receivedAt":"2024-03-11T23:25:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Mar 11, 2024 at 12:41:19PM -0700, Josh Steadmon wrote:\n> On 2024.03.05 13:10, Patrick Steinhardt wrote:\n> > Hi,\n> > \n> > this patch series does the same as all the preceding patch series that\n> > optimized how the reftable library iterates through refs, but for\n> > reflogs instead.\n> > \n> > The goal of this patch series is to arrive at a constant number of\n> > allocations when iterating refs. This is achieved in mostly the same way\n> > we did it for ref iteration, namely by reusing already-allocated memory.\n> > Overall, this brings us down from 8 allocations per reflog record to\n> > essentially 0 allocations per reflog. Iterating through 1 million\n> > reflogs with `git reflog list` thus goes down from 8.068m allocations to\n> > only around 68.5k.\n> > \n> > This series is built on top of \"master\" at b387623c12 (The third batch,\n> > 2024-03-01) with Junio's \"ps/reftable-iteration-perf-part2\" at\n> > 43f70eaea0 (refs/reftable: precompute prefix length, 2024-03-04) merged\n> > into it.\n> > \n> > Patrick\n> > \n> > Patrick Steinhardt (7):\n> >   refs/reftable: reload correct stack when creating reflog iter\n> >   reftable/record: convert old and new object IDs to arrays\n> >   reftable/record: avoid copying author info\n> >   reftable/record: reuse refnames when decoding log records\n> >   reftable/record: reuse message when decoding log records\n> >   reftable/record: use scratch buffer when decoding records\n> >   refs/reftable: track last log record name via strbuf\n> > \n> >  refs/reftable-backend.c    |  52 +++++----------\n> >  reftable/block.c           |   4 +-\n> >  reftable/block.h           |   2 +\n> >  reftable/merged_test.c     |  11 ++--\n> >  reftable/readwrite_test.c  |  62 +++++++-----------\n> >  reftable/record.c          | 129 ++++++++++++++-----------------------\n> >  reftable/record.h          |   5 +-\n> >  reftable/record_test.c     |  68 ++++++++++---------\n> >  reftable/reftable-record.h |   6 +-\n> >  reftable/stack_test.c      |  26 ++++----\n> >  10 files changed, 154 insertions(+), 211 deletions(-)\n> > \n> > -- \n> > 2.44.0\n> > \n> \n> This series looks good to me (with one request that we add a test for\n> patch #1 if possible).\n> \n> Reviewed-by: Josh Steadmon <steadmon@google.com>\n\nThanks for your review!\n\nPatrick\n"},{"id":"490523","messageId":"CZS7Z199NZUG.2EM4STQ664P9W@jamesliu.io","threadId":"61053","inReplyTo":"6f568e4ccb67a7af8279352153d052c5f9a88234.1709640322.git.ps@pks.im","subject":"Re: [PATCH 3/7] reftable/record: avoid copying author info","fromName":"James Liu","fromEmail":"james@jamesliu.io","sentAt":"2024-03-13T01:09:23Z","receivedAt":"2024-03-13T01:09:28Z","isPatch":true,"sender":{"key":"james@jamesliu.io","avatar":"https://avatars.githubusercontent.com/u/4476568?v=4"},"body":"On Tue Mar 5, 2024 at 11:11 PM AEDT, Patrick Steinhardt wrote:\n> Each reflog entry contains information regarding the authorship of who\n> has made the change. This authorship information is not the same as that\n> of any of the commits that the reflog entry references, but instead\n> corresponds to the local user that has executed the command. Thus, it is\n> almost always the case that all reflog entries have the same author.\n\nWhat are your thoughts on simplifying this explanation a little bit? I\ngave it a try below:\n\nEach reflog entry contains authorship information indicating who has made\nthe change. The author here corresponds to the local user who has executed\nthe command rather than the author of the referenced commits. Thus, it is\nalmost always the case that all reflog entries have the same author.\n\n\nCheers,\nJames\n"},{"id":"491133","messageId":"ZfwxqBTLFYHI0SGh@tanuki","threadId":"61053","inReplyTo":"CZS7Z199NZUG.2EM4STQ664P9W@jamesliu.io","subject":"Re: [PATCH 3/7] reftable/record: avoid copying author info","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-03-21T13:10:00Z","receivedAt":"2024-03-21T13:10:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 13, 2024 at 12:09:23PM +1100, James Liu wrote:\n> On Tue Mar 5, 2024 at 11:11 PM AEDT, Patrick Steinhardt wrote:\n> > Each reflog entry contains information regarding the authorship of who\n> > has made the change. This authorship information is not the same as that\n> > of any of the commits that the reflog entry references, but instead\n> > corresponds to the local user that has executed the command. Thus, it is\n> > almost always the case that all reflog entries have the same author.\n> \n> What are your thoughts on simplifying this explanation a little bit? I\n> gave it a try below:\n> \n> Each reflog entry contains authorship information indicating who has made\n> the change. The author here corresponds to the local user who has executed\n> the command rather than the author of the referenced commits. Thus, it is\n> almost always the case that all reflog entries have the same author.\n\nThat would've been a bit shorter indeed. But the patch series has been\nmerged to `next` by now, so I'll leave it at that.\n\nThanks!\n\nPatrick\n"}]}