{"thread":{"id":"62539","subject":"[PATCH 0/2] refs: stylistic improvements to ref migration optimization","startedAt":"2024-11-25T07:35:05Z","lastAt":"2024-11-26T08:22:29Z","messageCount":6,"participants":["Patrick Steinhardt","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"508037","messageId":"20241125-pks-refs-migration-optimization-followup-v1-0-0e1b4a2af384@pks.im","threadId":"62539","inReplyTo":null,"subject":"[PATCH 0/2] refs: stylistic improvements to ref migration optimization","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T07:34:41Z","receivedAt":"2024-11-25T07:35:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis small patch series addresses some stylistic nits surfaced in the\nreview of ps/ref-backend-migration-optim. v3 of that series and the\nmerge to `next` have crossed, so I'm resending the changes from v3 on\ntop of what we have in `next`.\n\nThis version is built on top of 6ea2d9d271 (Sync with Git 2.47.1,\n2024-11-25) with ps/ref-backend-migration-optim at d94ac23d3b\n(reftable/block: optimize allocations by using scratch buffer,\n2024-11-20) merged into it.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (2):\n refs: adapt `initial_transaction` flag to be unsigned\n reftable: rename scratch buffer\n\n refs.c | 2 +-\n refs.h | 2 +-\n reftable/block.c | 10 +++++-----\n reftable/block.h | 3 ++-\n reftable/writer.c | 22 +++++++++++-----------\n reftable/writer.h | 3 ++-\n 6 files changed, 22 insertions(+), 20 deletions(-)\n\n\n---\nbase-commit: a468246cedfb3b54b03609be75d9e13f0be1cc29\nchange-id: 20241125-pks-refs-migration-optimization-followup-d766e86b8925\n\n"},{"id":"508038","messageId":"20241125-pks-refs-migration-optimization-followup-v1-1-0e1b4a2af384@pks.im","threadId":"62539","inReplyTo":"20241125-pks-refs-migration-optimization-followup-v1-0-0e1b4a2af384@pks.im","subject":"[PATCH 1/2] refs: adapt `initial_transaction` flag to be unsigned","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T07:34:42Z","receivedAt":"2024-11-25T07:35:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `initial_transaction` flag is tracked as a signed integer, but we\ntypically pass around flags via unsigned integers. Adapt the type\naccordingly.\n\nSuggested-by: Christian Couder \nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 2 +-\n refs.h | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 65eea3eb7734d03f09a22e8edfe5074d532398ff..ee870817466b7d6d6a6619ce0baffe17f3d5a39f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2325,7 +2325,7 @@ int refs_verify_refname_available(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t const struct string_list *extras,\n \t\t\t\t const struct string_list *skip,\n-\t\t\t\t int initial_transaction,\n+\t\t\t\t unsigned int initial_transaction,\n \t\t\t\t struct strbuf *err)\n {\n \tconst char *slash;\ndiff --git a/refs.h b/refs.h\nindex 980bd20cf24e15144aeff991eeba8b27a936d386..95baf194ba9493f4e8f1f70924f0eb713e5bbd49 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -110,7 +110,7 @@ int refs_verify_refname_available(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t const struct string_list *extras,\n \t\t\t\t const struct string_list *skip,\n-\t\t\t\t int initial_transaction,\n+\t\t\t\t unsigned int initial_transaction,\n \t\t\t\t struct strbuf *err);\n \n int refs_ref_exists(struct ref_store *refs, const char *refname);\n\n-- \n2.47.0.274.g962d0b743d.dirty\n\n"},{"id":"508039","messageId":"20241125-pks-refs-migration-optimization-followup-v1-2-0e1b4a2af384@pks.im","threadId":"62539","inReplyTo":"20241125-pks-refs-migration-optimization-followup-v1-0-0e1b4a2af384@pks.im","subject":"[PATCH 2/2] reftable: rename scratch buffer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-11-25T07:34:43Z","receivedAt":"2024-11-25T07:35:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Both `struct block_writer` and `struct reftable_writer` have a `buf`\nmember that is being reused to optimize the number of allocations.\nRename the variable to `scratch` to clarify its intend and provide a\ncomment explaining why it exists.\n\nSuggested-by: Christian Couder \nSigned-off-by: Patrick Steinhardt \n---\n reftable/block.c | 10 +++++-----\n reftable/block.h | 3 ++-\n reftable/writer.c | 22 +++++++++++-----------\n reftable/writer.h | 3 ++-\n 4 files changed, 20 insertions(+), 18 deletions(-)\n\ndiff --git a/reftable/block.c b/reftable/block.c\nindex 1aa7e8cd3cbf0980f6bc20262be89e755d0a4b4b..01980784854cc454938bd2278b94047ff62c20d4 100644\n--- a/reftable/block.c\n+++ b/reftable/block.c\n@@ -115,16 +115,16 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \tint n = 0;\n \tint err;\n \n-\terr = reftable_record_key(rec, &w->buf);\n+\terr = reftable_record_key(rec, &w->scratch);\n \tif (err < 0)\n \t\tgoto done;\n \n-\tif (!w->buf.len) {\n+\tif (!w->scratch.len) {\n \t\terr = REFTABLE_API_ERROR;\n \t\tgoto done;\n \t}\n \n-\tn = reftable_encode_key(&is_restart, out, last, w->buf,\n+\tn = reftable_encode_key(&is_restart, out, last, w->scratch,\n \t\t\t\treftable_record_val_type(rec));\n \tif (n < 0) {\n \t\terr = -1;\n@@ -140,7 +140,7 @@ int block_writer_add(struct block_writer *w, struct reftable_record *rec)\n \tstring_view_consume(&out, n);\n \n \terr = block_writer_register_restart(w, start.len - out.len, is_restart,\n-\t\t\t\t\t &w->buf);\n+\t\t\t\t\t &w->scratch);\n done:\n \treturn err;\n }\n@@ -565,7 +565,7 @@ void block_writer_release(struct block_writer *bw)\n \tREFTABLE_FREE_AND_NULL(bw->zstream);\n \tREFTABLE_FREE_AND_NULL(bw->restarts);\n \tREFTABLE_FREE_AND_NULL(bw->compressed);\n-\treftable_buf_release(&bw->buf);\n+\treftable_buf_release(&bw->scratch);\n \treftable_buf_release(&bw->last_key);\n \t/* the block is not owned. */\n }\ndiff --git a/reftable/block.h b/reftable/block.h\nindex d76f00553073c10185e716e71e2f415ce5dcf7e2..0431e8591f41dedfb96eef304ea63ef2e9e5f5dd 100644\n--- a/reftable/block.h\n+++ b/reftable/block.h\n@@ -39,7 +39,8 @@ struct block_writer {\n \tuint32_t restart_cap;\n \n \tstruct reftable_buf last_key;\n-\tstruct reftable_buf buf;\n+\t/* Scratch buffer used to avoid allocations. */\n+\tstruct reftable_buf scratch;\n \tint entries;\n };\n \ndiff --git a/reftable/writer.c b/reftable/writer.c\nindex 6501376ce826469556a7dfa3c39258847300ae66..be0fae6cb229411258d40b8865c2fdee88fd5bcd 100644\n--- a/reftable/writer.c\n+++ b/reftable/writer.c\n@@ -148,7 +148,7 @@ int reftable_writer_new(struct reftable_writer **out,\n \n \treftable_buf_init(&wp->block_writer_data.last_key);\n \treftable_buf_init(&wp->last_key);\n-\treftable_buf_init(&wp->buf);\n+\treftable_buf_init(&wp->scratch);\n \tREFTABLE_CALLOC_ARRAY(wp->block, opts.block_size);\n \tif (!wp->block) {\n \t\treftable_free(wp);\n@@ -181,7 +181,7 @@ static void writer_release(struct reftable_writer *w)\n \t\tw->block_writer = NULL;\n \t\twriter_clear_index(w);\n \t\treftable_buf_release(&w->last_key);\n-\t\treftable_buf_release(&w->buf);\n+\t\treftable_buf_release(&w->scratch);\n \t}\n }\n \n@@ -253,17 +253,17 @@ static int writer_add_record(struct reftable_writer *w,\n {\n \tint err;\n \n-\terr = reftable_record_key(rec, &w->buf);\n+\terr = reftable_record_key(rec, &w->scratch);\n \tif (err < 0)\n \t\tgoto done;\n \n-\tif (reftable_buf_cmp(&w->last_key, &w->buf) >= 0) {\n+\tif (reftable_buf_cmp(&w->last_key, &w->scratch) >= 0) {\n \t\terr = REFTABLE_API_ERROR;\n \t\tgoto done;\n \t}\n \n \treftable_buf_reset(&w->last_key);\n-\terr = reftable_buf_add(&w->last_key, w->buf.buf, w->buf.len);\n+\terr = reftable_buf_add(&w->last_key, w->scratch.buf, w->scratch.len);\n \tif (err < 0)\n \t\tgoto done;\n \n@@ -339,25 +339,25 @@ int reftable_writer_add_ref(struct reftable_writer *w,\n \t\tgoto out;\n \n \tif (!w->opts.skip_index_objects && reftable_ref_record_val1(ref)) {\n-\t\treftable_buf_reset(&w->buf);\n-\t\terr = reftable_buf_add(&w->buf, (char *)reftable_ref_record_val1(ref),\n+\t\treftable_buf_reset(&w->scratch);\n+\t\terr = reftable_buf_add(&w->scratch, (char *)reftable_ref_record_val1(ref),\n \t\t\t\t hash_size(w->opts.hash_id));\n \t\tif (err < 0)\n \t\t\tgoto out;\n \n-\t\terr = writer_index_hash(w, &w->buf);\n+\t\terr = writer_index_hash(w, &w->scratch);\n \t\tif (err < 0)\n \t\t\tgoto out;\n \t}\n \n \tif (!w->opts.skip_index_objects && reftable_ref_record_val2(ref)) {\n-\t\treftable_buf_reset(&w->buf);\n-\t\terr = reftable_buf_add(&w->buf, reftable_ref_record_val2(ref),\n+\t\treftable_buf_reset(&w->scratch);\n+\t\terr = reftable_buf_add(&w->scratch, reftable_ref_record_val2(ref),\n \t\t\t\t hash_size(w->opts.hash_id));\n \t\tif (err < 0)\n \t\t\tgoto out;\n \n-\t\terr = writer_index_hash(w, &w->buf);\n+\t\terr = writer_index_hash(w, &w->scratch);\n \t\tif (err < 0)\n \t\t\tgoto out;\n \t}\ndiff --git a/reftable/writer.h b/reftable/writer.h\nindex 421a897dccd85ad0532860ff1b4f38b2813d438d..1f4788a430c52c5387b5e97f639e84544d0b9ba2 100644\n--- a/reftable/writer.h\n+++ b/reftable/writer.h\n@@ -20,7 +20,8 @@ struct reftable_writer {\n \tvoid *write_arg;\n \tint pending_padding;\n \tstruct reftable_buf last_key;\n-\tstruct reftable_buf buf;\n+\t/* Scratch buffer used to avoid allocations. */\n+\tstruct reftable_buf scratch;\n \n \t/* offset of next block to write. */\n \tuint64_t next;\n\n-- \n2.47.0.274.g962d0b743d.dirty\n\n"},{"id":"508059","messageId":"CAP8UFD3XYZe_10dBVpORKPq0PgMPDEAsbZAsd=xJB14i9Ocf7A@mail.gmail.com","threadId":"62539","inReplyTo":"20241125-pks-refs-migration-optimization-followup-v1-0-0e1b4a2af384@pks.im","subject":"Re: [PATCH 0/2] refs: stylistic improvements to ref migration optimization","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2024-11-25T09:35:58Z","receivedAt":"2024-11-25T09:36:12Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Mon, Nov 25, 2024 at 8:35 AM Patrick Steinhardt wrote:\n>\n> Hi,\n>\n> this small patch series addresses some stylistic nits surfaced in the\n> review of ps/ref-backend-migration-optim. v3 of that series and the\n> merge to `next` have crossed, so I'm resending the changes from v3 on\n> top of what we have in `next`.\n\nI took a look and both patches in this series look good and correct to me. Ack!\n"},{"id":"508117","messageId":"xmqqplmi51e8.fsf@gitster.g","threadId":"62539","inReplyTo":"CAP8UFD3XYZe_10dBVpORKPq0PgMPDEAsbZAsd=xJB14i9Ocf7A@mail.gmail.com","subject":"Re: [PATCH 0/2] refs: stylistic improvements to ref migration optimization","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-25T23:39:27Z","receivedAt":"2024-11-25T23:39:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Mon, Nov 25, 2024 at 8:35 AM Patrick Steinhardt wrote:\n>>\n>> Hi,\n>>\n>> this small patch series addresses some stylistic nits surfaced in the\n>> review of ps/ref-backend-migration-optim. v3 of that series and the\n>> merge to `next` have crossed, so I'm resending the changes from v3 on\n>> top of what we have in `next`.\n>\n> I took a look and both patches in this series look good and correct to me. Ack!\n\nLooking good. Thanks, both of you.\n"},{"id":"508159","messageId":"xmqqjzcqxv3x.fsf@gitster.g","threadId":"62539","inReplyTo":"20241125-pks-refs-migration-optimization-followup-v1-1-0e1b4a2af384@pks.im","subject":"Re: [PATCH 1/2] refs: adapt `initial_transaction` flag to be unsigned","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-26T08:22:26Z","receivedAt":"2024-11-26T08:22:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The `initial_transaction` flag is tracked as a signed integer, but we\n> typically pass around flags via unsigned integers. Adapt the type\n> accordingly.\n\nIt would not matter while the information in the variable is 1 bit\nand the check is \"is the whole variable 0?\", which is what the\ncurrent code does on this parameter. But when we'd start using the\nparameter as a flag word to stuff more bits, this starts to matter.\n\nGood future-proofing. Thanks.\n"}]}