{"thread":{"id":"66427","subject":"[PATCH] packed-refs: use `fwrite()` when passing refs verbatim","startedAt":"2026-09-30T15:15:38Z","lastAt":"2026-10-06T09:36:33Z","messageCount":7,"participants":["Karthik Nayak","Toon Claes"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"553717","messageId":"20260930-kn-speedup-packed-refs-v1-1-111cd03d9b0e@gmail.com","threadId":"66427","inReplyTo":null,"subject":"[PATCH] packed-refs: use `fwrite()` when passing refs verbatim","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-09-30T15:15:29Z","receivedAt":"2026-09-30T15:15:38Z","isPatch":true,"body":"The `write_with_updates()` function uses a `struct ref_iterator` to\niterate over all refs to write to the temporary packfile. It receives\nthe iterator from `packed_ref_iterator_begin()` which takes a snapshot\nof the 'packed-refs' file.\n\nWhile writing to the new packfile, writes are routed via\n`write_packed_entry()` which uses `fprintf()`. Even for references which\nhaven't changed, we use the same mechanism. Instead, let's track the\nposition of unchanged references in the snapshot iterator and directly\nuse `fwrite()`.\n\nThis removes the unnecessary formatting operation involved. We can see a\nconsistent ~20% performance improvement when deleting from packed\nreferences.\n\nBenchmark 1: update-ref: delete ref (refcount = 100000, revision = master)\n  Time (mean ± σ):      28.7 ms ±   1.7 ms    [User: 22.5 ms, System: 5.9 ms]\n  Range (min … max):    26.7 ms …  33.3 ms    46 runs\n\nBenchmark 2: update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs)\n  Time (mean ± σ):      23.8 ms ±   1.2 ms    [User: 17.5 ms, System: 6.0 ms]\n  Range (min … max):    22.1 ms …  27.7 ms    56 runs\n\nSummary\n  update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs) ran\n    1.21 ± 0.09 times faster than update-ref: delete ref (refformat = files, refcount = 100000, revision = master)\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n refs/packed-backend.c | 43 ++++++++++++++++++++++++++++++++-----------\n 1 file changed, 32 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a73fc6aca7..ef952cdba6 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -879,6 +879,12 @@ struct packed_ref_iterator {\n \t/* The current position in the snapshot's buffer: */\n \tconst char *pos;\n \n+\t/*\n+\t * Start of the current record, set when advancing `pos`. Used to\n+\t * pass records verbatim to `fwrite()`.\n+\t */\n+\tconst char *record_start;\n+\n \t/* The end of the part of the buffer that will be iterated over: */\n \tconst char *eof;\n \n@@ -933,6 +939,7 @@ static int next_record(struct packed_ref_iterator *iter)\n \tif (iter->pos == iter->eof)\n \t\treturn ITER_DONE;\n \n+\titer->record_start = iter->pos;\n \titer->base.ref.flags = REF_ISPACKED;\n \tp = iter->pos;\n \n@@ -1218,17 +1225,27 @@ static struct ref_iterator *packed_ref_iterator_begin(\n \n /*\n  * Write an entry to the packed-refs file for the specified refname.\n- * If peeled is non-NULL, write it as the entry's peeled value. On\n- * error, return a nonzero value and leave errno set at the value left\n- * by the failing call to `fprintf()`.\n+ *\n+ * If the raw data is available, skip the formatting and directly write to\n+ * the file using `fwrite()`. e.g. when deleting references and remaining\n+ * refs need to be written verbatim. Otherwise, use `fprintf()`.\n+ *\n+ * If peeled is non-NULL, write it as the entry's peeled value.\n+ *\n+ * On error, return a nonzero value and leave errno set at the value left\n+ * by the failing call to `fwrite()` or `fprintf()`.\n  */\n-static int write_packed_entry(FILE *fh, const char *refname,\n-\t\t\t      const struct object_id *oid,\n+static int write_packed_entry(FILE *fh, const char *raw, size_t raw_len,\n+\t\t\t      const char *refname, const struct object_id *oid,\n \t\t\t      const struct object_id *peeled)\n {\n-\tif (fprintf(fh, \"%s %s\\n\", oid_to_hex(oid), refname) < 0 ||\n-\t    (peeled && fprintf(fh, \"^%s\\n\", oid_to_hex(peeled)) < 0))\n+\tif (raw) {\n+\t\tif (fwrite(raw, raw_len, 1, fh) != 1)\n+\t\t\treturn -1;\n+\t} else if (fprintf(fh, \"%s %s\\n\", oid_to_hex(oid), refname) < 0 ||\n+\t\t   (peeled && fprintf(fh, \"^%s\\n\", oid_to_hex(peeled)) < 0)) {\n \t\treturn -1;\n+\t}\n \n \treturn 0;\n }\n@@ -1530,9 +1547,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n \t\t}\n \n \t\tif (cmp < 0) {\n-\t\t\t/* Pass the old reference through. */\n-\t\t\tif (write_packed_entry(out, iter->ref.name,\n-\t\t\t\t\t       iter->ref.oid, iter->ref.peeled_oid))\n+\t\t\tconst struct packed_ref_iterator *packed_iter =\n+\t\t\t\t(const struct packed_ref_iterator *)iter;\n+\t\t\tsize_t len = packed_iter->pos - packed_iter->record_start;\n+\n+\t\t\tif (write_packed_entry(out, packed_iter->record_start,\n+\t\t\t\t\t       len, iter->ref.name, iter->ref.oid,\n+\t\t\t\t\t       iter->ref.peeled_oid))\n \t\t\t\tgoto write_error;\n \n \t\t\tif ((ok = ref_iterator_advance(iter)) != ITER_OK) {\n@@ -1551,7 +1572,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n \t\t} else {\n \t\t\tbool peeled = update->flags & REF_HAVE_PEELED;\n \n-\t\t\tif (write_packed_entry(out, update->refname,\n+\t\t\tif (write_packed_entry(out, NULL, 0, update->refname,\n \t\t\t\t\t       &update->new_oid,\n \t\t\t\t\t       peeled ? &update->peeled : NULL))\n \t\t\t\tgoto write_error;\n\n---\nbase-commit: a018953688f1b10bddf91bff8747068f5f4746a4\nchange-id: 20260930-kn-speedup-packed-refs-9868f5d0abe9\n\n\nThanks\n- Karthik\n\n"},{"id":"553868","messageId":"87y0ch5fkv.fsf@dev.null.iotcl.com.invalid","threadId":"66427","inReplyTo":"20260930-kn-speedup-packed-refs-v1-1-111cd03d9b0e@gmail.com","subject":"Re: [PATCH] packed-refs: use `fwrite()` when passing refs verbatim","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-10-01T20:30:24Z","receivedAt":"2026-10-01T20:30:31Z","isPatch":true,"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> The `write_with_updates()` function uses a `struct ref_iterator` to\n> iterate over all refs to write to the temporary packfile. It receives\n\nI would say in general \"packfile\" is only used for objects, but it seems\nthere is one mention of \"packfile\" in this file, so I'm not sure.\n\n> the iterator from `packed_ref_iterator_begin()` which takes a snapshot\n> of the 'packed-refs' file.\n>\n> While writing to the new packfile, writes are routed via\n> `write_packed_entry()` which uses `fprintf()`. Even for references which\n> haven't changed, we use the same mechanism. Instead, let's track the\n> position of unchanged references in the snapshot iterator and directly\n> use `fwrite()`.\n\nThere are a few sanitizations that get lost by using fwrite():\n\n* oid_to_hex() is no longer called, so if an OID would be written in the\n  old packed-ref as uppercase, it would be copied as such\n\n* next_record() uses isspace(3) to split the oid from the refname. If\n  the record would be separated by a tab instead of a space, that would\n  be copied over.\n\n* next_record() calls check_refname_format(), if it ain't good, oidclr()\n  is called. Before this patch, that means the zero OID will be written\n  for such ref. That changes with this patch, and the original OID is\n  written if refname_is_safe() passes.\n  This won't make a difference in Git though, because upon reading back\n  from the packed-refs, the zero OID is read into memory anyway, so\n  there is no observable difference.\n\n* similar to the item above, when the ref is peelable, and REF_ISBROKEN\n  (around line 985), the `peeled_oid` is not filled in. This means for a\n  refname not matching the format will not contain a peeled OID for that\n  ref.\n  For example, in the source file:\n\n    1937b04ead6e53d949a4e2eb97ed743b42692fde refs/tags/a-bad~tag\n    ^67c1263aae5c1750e9d9211995e6d1bb38314fca\n\n  is converted to:\n\n    0000000000000000000000000000000000000000 refs/tags/a-bad~tag\n\n  After this patch, the ref and it's peeled commit OID is copied\n  verbatim.\n\nI'm not saying any of this is bad. These are just some side-effects of\nyour fwrite() approach. And some might be worht mentioning in the commit\nmessage.\n\n>\n> This removes the unnecessary formatting operation involved. We can see a\n> consistent ~20% performance improvement when deleting from packed\n> references.\n>\n> Benchmark 1: update-ref: delete ref (refcount = 100000, revision = master)\n>   Time (mean ± σ):      28.7 ms ±   1.7 ms    [User: 22.5 ms, System: 5.9 ms]\n>   Range (min … max):    26.7 ms …  33.3 ms    46 runs\n>\n> Benchmark 2: update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs)\n>   Time (mean ± σ):      23.8 ms ±   1.2 ms    [User: 17.5 ms, System: 6.0 ms]\n>   Range (min … max):    22.1 ms …  27.7 ms    56 runs\n>\n> Summary\n>   update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs) ran\n>     1.21 ± 0.09 times faster than update-ref: delete ref (refformat = files, refcount = 100000, revision = master)\n\nNot bad.\n\n>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n>  refs/packed-backend.c | 43 ++++++++++++++++++++++++++++++++-----------\n>  1 file changed, 32 insertions(+), 11 deletions(-)\n>\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index a73fc6aca7..ef952cdba6 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -879,6 +879,12 @@ struct packed_ref_iterator {\n>  \t/* The current position in the snapshot's buffer: */\n>  \tconst char *pos;\n>  \n> +\t/*\n> +\t * Start of the current record, set when advancing `pos`. Used to\n> +\t * pass records verbatim to `fwrite()`.\n\nI assume there was a version you've been working on that was calling\nfwrite() directly from ref_transaction_error write_with_updates()? With\nthat being wrapped by a helper function, I think it's better to name\nthat function here.\n\n> +\t */\n> +\tconst char *record_start;\n> +\n>  \t/* The end of the part of the buffer that will be iterated over: */\n>  \tconst char *eof;\n>  \n> @@ -933,6 +939,7 @@ static int next_record(struct packed_ref_iterator *iter)\n>  \tif (iter->pos == iter->eof)\n>  \t\treturn ITER_DONE;\n>  \n> +\titer->record_start = iter->pos;\n>  \titer->base.ref.flags = REF_ISPACKED;\n>  \tp = iter->pos;\n>  \n> @@ -1218,17 +1225,27 @@ static struct ref_iterator *packed_ref_iterator_begin(\n>  \n>  /*\n>   * Write an entry to the packed-refs file for the specified refname.\n> - * If peeled is non-NULL, write it as the entry's peeled value. On\n> - * error, return a nonzero value and leave errno set at the value left\n> - * by the failing call to `fprintf()`.\n> + *\n> + * If the raw data is available, skip the formatting and directly write to\n> + * the file using `fwrite()`. e.g. when deleting references and remaining\n> + * refs need to be written verbatim. Otherwise, use `fprintf()`.\n> + *\n> + * If peeled is non-NULL, write it as the entry's peeled value.\n> + *\n> + * On error, return a nonzero value and leave errno set at the value left\n> + * by the failing call to `fwrite()` or `fprintf()`.\n>   */\n> -static int write_packed_entry(FILE *fh, const char *refname,\n> -\t\t\t      const struct object_id *oid,\n> +static int write_packed_entry(FILE *fh, const char *raw, size_t raw_len,\n> +\t\t\t      const char *refname, const struct object_id *oid,\n>  \t\t\t      const struct object_id *peeled)\n\nI'm not convinced it's worth to have both ways of writing in a single\nfunction.\n\nI rather keep this function as-is and add a function:\n\n    static int write_packed_entry_preformatted(FILE *fh,\n                                               const char *line,\n                                               size_t line_len)\n    {\n    \tif (fwrite(raw, raw_len, 1, fh) != 1)\n    \t\t\treturn -1;\n    \treturn 0;\n    }\n\nOr maybe even:\n\n    static int write_packed_entry_from_iter(FILE *fh,\n                                            struct packed_ref_iterator *packed_iter)\n    {\n    \tsize_t ret = fwrite(packed_iter->record_start,\n                            packed_iter->pos - packed_iter->record_start,\n                            1, fh);\n        if (ret != 1)\n    \t\t\treturn -1;\n    \treturn 0;\n    }\n\n>  {\n> -\tif (fprintf(fh, \"%s %s\\n\", oid_to_hex(oid), refname) < 0 ||\n> -\t    (peeled && fprintf(fh, \"^%s\\n\", oid_to_hex(peeled)) < 0))\n> +\tif (raw) {\n> +\t\tif (fwrite(raw, raw_len, 1, fh) != 1)\n\nI see in some places, for example in fwrite_or_die(), `len` and `1` are\nswapped and the return value is compared against `len`. This is useful\nwhen the length can be 0. But that cannot the case here, so no need to\nchange that.\n\n> +\t\t\treturn -1;\n> +\t} else if (fprintf(fh, \"%s %s\\n\", oid_to_hex(oid), refname) < 0 ||\n> +\t\t   (peeled && fprintf(fh, \"^%s\\n\", oid_to_hex(peeled)) < 0)) {\n\nFor what's it's worth, I think it's really ugly to do this in a single\nif() statement, but that just could be me.\n\n>  \t\treturn -1;\n> +\t}\n>  \n>  \treturn 0;\n>  }\n> @@ -1530,9 +1547,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n>  \t\t}\n>  \n>  \t\tif (cmp < 0) {\n> -\t\t\t/* Pass the old reference through. */\n> -\t\t\tif (write_packed_entry(out, iter->ref.name,\n> -\t\t\t\t\t       iter->ref.oid, iter->ref.peeled_oid))\n> +\t\t\tconst struct packed_ref_iterator *packed_iter =\n> +\t\t\t\t(const struct packed_ref_iterator *)iter;\n> +\t\t\tsize_t len = packed_iter->pos - packed_iter->record_start;\n\nThis is nice. Because packed_iter->pos points at the next record, the\n`len` will include the peeled OID line as well. Which is fwrite(3)'n in\none go. That's a nice win.\n\n> +\n> +\t\t\tif (write_packed_entry(out, packed_iter->record_start,\n> +\t\t\t\t\t       len, iter->ref.name, iter->ref.oid,\n> +\t\t\t\t\t       iter->ref.peeled_oid))\n>  \t\t\t\tgoto write_error;\n>  \n>  \t\t\tif ((ok = ref_iterator_advance(iter)) != ITER_OK) {\n> @@ -1551,7 +1572,7 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n>  \t\t} else {\n>  \t\t\tbool peeled = update->flags & REF_HAVE_PEELED;\n>  \n> -\t\t\tif (write_packed_entry(out, update->refname,\n> +\t\t\tif (write_packed_entry(out, NULL, 0, update->refname,\n>  \t\t\t\t\t       &update->new_oid,\n>  \t\t\t\t\t       peeled ? &update->peeled : NULL))\n>  \t\t\t\tgoto write_error;\n>\n> ---\n> base-commit: a018953688f1b10bddf91bff8747068f5f4746a4\n> change-id: 20260930-kn-speedup-packed-refs-9868f5d0abe9\n>\n>\n> Thanks\n> - Karthik\n>\n\n-- \nLaters,\nToon\n"},{"id":"553879","messageId":"CAOLa=ZRTtE+FMrVZBwSmZ-aVBXFphnQB414-PASCAAXJJCgNww@mail.gmail.com","threadId":"66427","inReplyTo":"87y0ch5fkv.fsf@dev.null.iotcl.com.invalid","subject":"Re: [PATCH] packed-refs: use `fwrite()` when passing refs verbatim","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-10-02T00:16:09Z","receivedAt":"2026-10-02T00:16:12Z","isPatch":true,"body":"Toon Claes <toon@iotcl.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> The `write_with_updates()` function uses a `struct ref_iterator` to\n>> iterate over all refs to write to the temporary packfile. It receives\n>\n> I would say in general \"packfile\" is only used for objects, but it seems\n> there is one mention of \"packfile\" in this file, so I'm not sure.\n>\n\nYou're right, I will change both of those to say 'packed-refs'.\n\n>> the iterator from `packed_ref_iterator_begin()` which takes a snapshot\n>> of the 'packed-refs' file.\n>>\n>> While writing to the new packfile, writes are routed via\n>> `write_packed_entry()` which uses `fprintf()`. Even for references which\n>> haven't changed, we use the same mechanism. Instead, let's track the\n>> position of unchanged references in the snapshot iterator and directly\n>> use `fwrite()`.\n>\n> There are a few sanitizations that get lost by using fwrite():\n>\n> * oid_to_hex() is no longer called, so if an OID would be written in the\n>   old packed-ref as uppercase, it would be copied as such\n>\n> * next_record() uses isspace(3) to split the oid from the refname. If\n>   the record would be separated by a tab instead of a space, that would\n>   be copied over.\n>\n> * next_record() calls check_refname_format(), if it ain't good, oidclr()\n>   is called. Before this patch, that means the zero OID will be written\n>   for such ref. That changes with this patch, and the original OID is\n>   written if refname_is_safe() passes.\n>   This won't make a difference in Git though, because upon reading back\n>   from the packed-refs, the zero OID is read into memory anyway, so\n>   there is no observable difference.\n>\n> * similar to the item above, when the ref is peelable, and REF_ISBROKEN\n>   (around line 985), the `peeled_oid` is not filled in. This means for a\n>   refname not matching the format will not contain a peeled OID for that\n>   ref.\n>   For example, in the source file:\n>\n>     1937b04ead6e53d949a4e2eb97ed743b42692fde refs/tags/a-bad~tag\n>     ^67c1263aae5c1750e9d9211995e6d1bb38314fca\n>\n>   is converted to:\n>\n>     0000000000000000000000000000000000000000 refs/tags/a-bad~tag\n>\n>   After this patch, the ref and it's peeled commit OID is copied\n>   verbatim.\n>\n> I'm not saying any of this is bad. These are just some side-effects of\n> your fwrite() approach. And some might be worht mentioning in the commit\n> message.\n>\n\nWhile you're right, sanitation is not part of this flow, here we're\nsimply deleting a reference from the packed-refs file, the fact that\nsanitation was even happening was a side-effect.\n\nBut I will mention it in the commit message, I think that makes sense.\n\n[snip]\n\n>>\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> ---\n>>  refs/packed-backend.c | 43 ++++++++++++++++++++++++++++++++-----------\n>>  1 file changed, 32 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n>> index a73fc6aca7..ef952cdba6 100644\n>> --- a/refs/packed-backend.c\n>> +++ b/refs/packed-backend.c\n>> @@ -879,6 +879,12 @@ struct packed_ref_iterator {\n>>  \t/* The current position in the snapshot's buffer: */\n>>  \tconst char *pos;\n>>\n>> +\t/*\n>> +\t * Start of the current record, set when advancing `pos`. Used to\n>> +\t * pass records verbatim to `fwrite()`.\n>\n> I assume there was a version you've been working on that was calling\n> fwrite() directly from ref_transaction_error write_with_updates()? With\n> that being wrapped by a helper function, I think it's better to name\n> that function here.\n>\n\nI'm not sure I follow what you mean here.\n\n>> +\t */\n>> +\tconst char *record_start;\n>> +\n>>  \t/* The end of the part of the buffer that will be iterated over: */\n>>  \tconst char *eof;\n>>\n>> @@ -933,6 +939,7 @@ static int next_record(struct packed_ref_iterator *iter)\n>>  \tif (iter->pos == iter->eof)\n>>  \t\treturn ITER_DONE;\n>>\n>> +\titer->record_start = iter->pos;\n>>  \titer->base.ref.flags = REF_ISPACKED;\n>>  \tp = iter->pos;\n>>\n>> @@ -1218,17 +1225,27 @@ static struct ref_iterator *packed_ref_iterator_begin(\n>>\n>>  /*\n>>   * Write an entry to the packed-refs file for the specified refname.\n>> - * If peeled is non-NULL, write it as the entry's peeled value. On\n>> - * error, return a nonzero value and leave errno set at the value left\n>> - * by the failing call to `fprintf()`.\n>> + *\n>> + * If the raw data is available, skip the formatting and directly write to\n>> + * the file using `fwrite()`. e.g. when deleting references and remaining\n>> + * refs need to be written verbatim. Otherwise, use `fprintf()`.\n>> + *\n>> + * If peeled is non-NULL, write it as the entry's peeled value.\n>> + *\n>> + * On error, return a nonzero value and leave errno set at the value left\n>> + * by the failing call to `fwrite()` or `fprintf()`.\n>>   */\n>> -static int write_packed_entry(FILE *fh, const char *refname,\n>> -\t\t\t      const struct object_id *oid,\n>> +static int write_packed_entry(FILE *fh, const char *raw, size_t raw_len,\n>> +\t\t\t      const char *refname, const struct object_id *oid,\n>>  \t\t\t      const struct object_id *peeled)\n>\n> I'm not convinced it's worth to have both ways of writing in a single\n> function.\n>\n> I rather keep this function as-is and add a function:\n>\n>     static int write_packed_entry_preformatted(FILE *fh,\n>                                                const char *line,\n>                                                size_t line_len)\n>     {\n>     \tif (fwrite(raw, raw_len, 1, fh) != 1)\n>     \t\t\treturn -1;\n>     \treturn 0;\n>     }\n>\n> Or maybe even:\n>\n>     static int write_packed_entry_from_iter(FILE *fh,\n>                                             struct packed_ref_iterator *packed_iter)\n>     {\n>     \tsize_t ret = fwrite(packed_iter->record_start,\n>                             packed_iter->pos - packed_iter->record_start,\n>                             1, fh);\n>         if (ret != 1)\n>     \t\t\treturn -1;\n>     \treturn 0;\n>     }\n>\n\nI did it cause it was small enough, but I don't have strong opinions. So\nlet's add another function, perhaps:\n\nstatic int write_packed_entry_raw(FILE *fh, const char *entry, size_t len)\n{\n\tif (fwrite(entry, len, 1, fh) != 1)\n\t\treturn -1;\n\n\treturn 0;\n}\n\n[snip]\n\n>> +\t\t\treturn -1;\n>> +\t} else if (fprintf(fh, \"%s %s\\n\", oid_to_hex(oid), refname) < 0 ||\n>> +\t\t   (peeled && fprintf(fh, \"^%s\\n\", oid_to_hex(peeled)) < 0)) {\n>\n> For what's it's worth, I think it's really ugly to do this in a single\n> if() statement, but that just could be me.\n\n\nI agree, but I don't want to change existing code in this patch as that\nwould simply be a distraction from the purpose.\n\n[snip]\n\n>\n> --\n> Laters,\n> Toon\n\nThanks for the review.\n"},{"id":"553961","messageId":"20261002-kn-speedup-packed-refs-v2-1-2ae75772ebc1@gmail.com","threadId":"66427","inReplyTo":"20260930-kn-speedup-packed-refs-v1-1-111cd03d9b0e@gmail.com","subject":"[PATCH v2] packed-refs: use `fwrite()` when passing refs verbatim","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-10-02T13:03:34Z","receivedAt":"2026-10-02T13:04:57Z","isPatch":true,"body":"The `write_with_updates()` function uses a `struct ref_iterator` to\niterate over all refs to write to the temporary packed-refs file. It\nreceives the iterator from `packed_ref_iterator_begin()` which takes a\nsnapshot of the 'packed-refs' file.\n\nWhile writing to the new packed-refs file, writes are routed via\n`write_packed_entry()` which uses `fprintf()`. Even for references which\nhaven't changed, we use the same mechanism. Instead, let's track the\nposition of unchanged references in the snapshot iterator and directly\nuse `fwrite()`.\n\nWith this, any sanitation which was happening as a side of reformatting\nis now lost. But that was never the job of this section of the code,\nsince the main intention is to simply rewrite the remaining refs post\ndeletion of the selective few.\n\nThis removes the unnecessary formatting operation involved. We can see a\nconsistent ~20% performance improvement when deleting from packed\nreferences.\n\nBenchmark 1: update-ref: delete ref (refcount = 100000, revision = master)\n  Time (mean ± σ):      28.7 ms ±   1.7 ms    [User: 22.5 ms, System: 5.9 ms]\n  Range (min … max):    26.7 ms …  33.3 ms    46 runs\n\nBenchmark 2: update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs)\n  Time (mean ± σ):      23.8 ms ±   1.2 ms    [User: 17.5 ms, System: 6.0 ms]\n  Range (min … max):    22.1 ms …  27.7 ms    56 runs\n\nSummary\n  update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs) ran\n    1.21 ± 0.09 times faster than update-ref: delete ref (refformat = files, refcount = 100000, revision = master)\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\nChanges in v2:\n- Instead of using the existing function, introduce a new\n  `write_packed_entry_raw()`.\n- Modify the commit to also note that we lose sanitization.\n- Link to v1: https://patch.msgid.link/20260930-kn-speedup-packed-refs-v1-1-111cd03d9b0e@gmail.com\n---\n refs/packed-backend.c | 30 +++++++++++++++++++++++++++---\n 1 file changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a73fc6aca7..43ad674cf4 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -879,6 +879,12 @@ struct packed_ref_iterator {\n \t/* The current position in the snapshot's buffer: */\n \tconst char *pos;\n \n+\t/*\n+\t * Start of the current record, set when advancing `pos`. Used to\n+\t * pass records verbatim to `fwrite()`.\n+\t */\n+\tconst char *record_start;\n+\n \t/* The end of the part of the buffer that will be iterated over: */\n \tconst char *eof;\n \n@@ -933,6 +939,7 @@ static int next_record(struct packed_ref_iterator *iter)\n \tif (iter->pos == iter->eof)\n \t\treturn ITER_DONE;\n \n+\titer->record_start = iter->pos;\n \titer->base.ref.flags = REF_ISPACKED;\n \tp = iter->pos;\n \n@@ -1233,6 +1240,19 @@ static int write_packed_entry(FILE *fh, const char *refname,\n \treturn 0;\n }\n \n+/*\n+ * Write an entry to the packed-refs file skip any formatting and directly\n+ * write to  the file using `fwrite()`. e.g. when deleting references and\n+ * remaining refs need to be written verbatim.\n+ */\n+static int write_packed_entry_raw(FILE *fh, const char *entry, size_t len)\n+{\n+\tif (fwrite(entry, len, 1, fh) != 1)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n {\n \tstruct packed_ref_store *refs =\n@@ -1530,9 +1550,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n \t\t}\n \n \t\tif (cmp < 0) {\n-\t\t\t/* Pass the old reference through. */\n-\t\t\tif (write_packed_entry(out, iter->ref.name,\n-\t\t\t\t\t       iter->ref.oid, iter->ref.peeled_oid))\n+\t\t\tconst struct packed_ref_iterator *packed_iter =\n+\t\t\t\t(const struct packed_ref_iterator *)iter;\n+\t\t\tsize_t len = packed_iter->pos - packed_iter->record_start;\n+\n+\t\t\tif (write_packed_entry_raw(out,\n+\t\t\t\t\t\t   packed_iter->record_start,\n+\t\t\t\t\t\t   len))\n \t\t\t\tgoto write_error;\n \n \t\t\tif ((ok = ref_iterator_advance(iter)) != ITER_OK) {\n\n---\nbase-commit: a018953688f1b10bddf91bff8747068f5f4746a4\nchange-id: 20260930-kn-speedup-packed-refs-9868f5d0abe9\n\n\nThanks\n- Karthik\n\n"},{"id":"554210","messageId":"87zewsf13b.fsf@dev.null.iotcl.com.invalid","threadId":"66427","inReplyTo":"20261002-kn-speedup-packed-refs-v2-1-2ae75772ebc1@gmail.com","subject":"Re: [PATCH v2] packed-refs: use `fwrite()` when passing refs verbatim","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-10-05T18:34:32Z","receivedAt":"2026-10-05T18:34:32Z","isPatch":true,"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> The `write_with_updates()` function uses a `struct ref_iterator` to\n> iterate over all refs to write to the temporary packed-refs file. It\n> receives the iterator from `packed_ref_iterator_begin()` which takes a\n> snapshot of the 'packed-refs' file.\n>\n> While writing to the new packed-refs file, writes are routed via\n> `write_packed_entry()` which uses `fprintf()`. Even for references which\n> haven't changed, we use the same mechanism. Instead, let's track the\n> position of unchanged references in the snapshot iterator and directly\n> use `fwrite()`.\n>\n> With this, any sanitation which was happening as a side of reformatting\n\ns/side/side effect/ ?\n\n> is now lost. But that was never the job of this section of the code,\n> since the main intention is to simply rewrite the remaining refs post\n> deletion of the selective few.\n\nAgreed.\n\n>\n> This removes the unnecessary formatting operation involved. We can see a\n> consistent ~20% performance improvement when deleting from packed\n> references.\n>\n> Benchmark 1: update-ref: delete ref (refcount = 100000, revision = master)\n>   Time (mean ± σ):      28.7 ms ±   1.7 ms    [User: 22.5 ms, System: 5.9 ms]\n>   Range (min … max):    26.7 ms …  33.3 ms    46 runs\n>\n> Benchmark 2: update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs)\n>   Time (mean ± σ):      23.8 ms ±   1.2 ms    [User: 17.5 ms, System: 6.0 ms]\n>   Range (min … max):    22.1 ms …  27.7 ms    56 runs\n>\n> Summary\n>   update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs) ran\n>     1.21 ± 0.09 times faster than update-ref: delete ref (refformat = files, refcount = 100000, revision = master)\n>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n> Changes in v2:\n> - Instead of using the existing function, introduce a new\n>   `write_packed_entry_raw()`.\n> - Modify the commit to also note that we lose sanitization.\n\ns/commit/commit message/\n\n> - Link to v1: https://patch.msgid.link/20260930-kn-speedup-packed-refs-v1-1-111cd03d9b0e@gmail.com\n\nOkay, I'm okay with this version.\n\n-- \nLaters,\nToon\n\n"},{"id":"554251","messageId":"20261006-kn-speedup-packed-refs-v3-1-a1c76b1df9e0@gmail.com","threadId":"66427","inReplyTo":"20260930-kn-speedup-packed-refs-v1-1-111cd03d9b0e@gmail.com","subject":"[PATCH v3] packed-refs: use `fwrite()` when passing refs verbatim","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-10-06T09:18:40Z","receivedAt":"2026-10-06T09:18:40Z","isPatch":true,"body":"The `write_with_updates()` function uses a `struct ref_iterator` to\niterate over all refs to write to the temporary packed-refs file. It\nreceives the iterator from `packed_ref_iterator_begin()` which takes a\nsnapshot of the 'packed-refs' file.\n\nWhile writing to the new packed-refs file, writes are routed via\n`write_packed_entry()` which uses `fprintf()`. Even for references which\nhaven't changed, we use the same mechanism. Instead, let's track the\nposition of unchanged references in the snapshot iterator and directly\nuse `fwrite()`.\n\nWith this, any sanitation which was happening as a side effect of\nreformatting is now lost. But that was never the job of this section of\nthe code, since the main intention is to simply rewrite the remaining\nrefs post deletion of the selective few.\n\nThis removes the unnecessary formatting operation involved. We can see a\nconsistent ~20% performance improvement when deleting from packed\nreferences.\n\nBenchmark 1: update-ref: delete ref (refcount = 100000, revision = master)\n  Time (mean ± σ):      28.7 ms ±   1.7 ms    [User: 22.5 ms, System: 5.9 ms]\n  Range (min … max):    26.7 ms …  33.3 ms    46 runs\n\nBenchmark 2: update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs)\n  Time (mean ± σ):      23.8 ms ±   1.2 ms    [User: 17.5 ms, System: 6.0 ms]\n  Range (min … max):    22.1 ms …  27.7 ms    56 runs\n\nSummary\n  update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs) ran\n    1.21 ± 0.09 times faster than update-ref: delete ref (refformat = files, refcount = 100000, revision = master)\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\nChanges in v3:\n- Fixed a typo in the commit message.\n- Link to v2: https://patch.msgid.link/20261002-kn-speedup-packed-refs-v2-1-2ae75772ebc1@gmail.com\n\nChanges in v2:\n- Instead of using the existing function, introduce a new\n  `write_packed_entry_raw()`.\n- Modify the commit to also note that we lose sanitization.\n- Link to v1: https://patch.msgid.link/20260930-kn-speedup-packed-refs-v1-1-111cd03d9b0e@gmail.com\n---\n refs/packed-backend.c | 30 +++++++++++++++++++++++++++---\n 1 file changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex a73fc6aca7..43ad674cf4 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -879,6 +879,12 @@ struct packed_ref_iterator {\n \t/* The current position in the snapshot's buffer: */\n \tconst char *pos;\n \n+\t/*\n+\t * Start of the current record, set when advancing `pos`. Used to\n+\t * pass records verbatim to `fwrite()`.\n+\t */\n+\tconst char *record_start;\n+\n \t/* The end of the part of the buffer that will be iterated over: */\n \tconst char *eof;\n \n@@ -933,6 +939,7 @@ static int next_record(struct packed_ref_iterator *iter)\n \tif (iter->pos == iter->eof)\n \t\treturn ITER_DONE;\n \n+\titer->record_start = iter->pos;\n \titer->base.ref.flags = REF_ISPACKED;\n \tp = iter->pos;\n \n@@ -1233,6 +1240,19 @@ static int write_packed_entry(FILE *fh, const char *refname,\n \treturn 0;\n }\n \n+/*\n+ * Write an entry to the packed-refs file skip any formatting and directly\n+ * write to  the file using `fwrite()`. e.g. when deleting references and\n+ * remaining refs need to be written verbatim.\n+ */\n+static int write_packed_entry_raw(FILE *fh, const char *entry, size_t len)\n+{\n+\tif (fwrite(entry, len, 1, fh) != 1)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n int packed_refs_lock(struct ref_store *ref_store, int flags, struct strbuf *err)\n {\n \tstruct packed_ref_store *refs =\n@@ -1530,9 +1550,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re\n \t\t}\n \n \t\tif (cmp < 0) {\n-\t\t\t/* Pass the old reference through. */\n-\t\t\tif (write_packed_entry(out, iter->ref.name,\n-\t\t\t\t\t       iter->ref.oid, iter->ref.peeled_oid))\n+\t\t\tconst struct packed_ref_iterator *packed_iter =\n+\t\t\t\t(const struct packed_ref_iterator *)iter;\n+\t\t\tsize_t len = packed_iter->pos - packed_iter->record_start;\n+\n+\t\t\tif (write_packed_entry_raw(out,\n+\t\t\t\t\t\t   packed_iter->record_start,\n+\t\t\t\t\t\t   len))\n \t\t\t\tgoto write_error;\n \n \t\t\tif ((ok = ref_iterator_advance(iter)) != ITER_OK) {\n\n---\nbase-commit: a018953688f1b10bddf91bff8747068f5f4746a4\nchange-id: 20260930-kn-speedup-packed-refs-9868f5d0abe9\n\n\nThanks\n- Karthik\n\n\n"},{"id":"554254","messageId":"CAOLa=ZR12Xc2ZV2T4q+HZTa4TfeO8G7GiabZr3EYdrG-n6YA8A@mail.gmail.com","threadId":"66427","inReplyTo":"87zewsf13b.fsf@dev.null.iotcl.com.invalid","subject":"Re: [PATCH v2] packed-refs: use `fwrite()` when passing refs verbatim","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-10-06T09:36:33Z","receivedAt":"2026-10-06T09:36:33Z","isPatch":true,"body":"Toon Claes <toon@iotcl.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> The `write_with_updates()` function uses a `struct ref_iterator` to\n>> iterate over all refs to write to the temporary packed-refs file. It\n>> receives the iterator from `packed_ref_iterator_begin()` which takes a\n>> snapshot of the 'packed-refs' file.\n>>\n>> While writing to the new packed-refs file, writes are routed via\n>> `write_packed_entry()` which uses `fprintf()`. Even for references which\n>> haven't changed, we use the same mechanism. Instead, let's track the\n>> position of unchanged references in the snapshot iterator and directly\n>> use `fwrite()`.\n>>\n>> With this, any sanitation which was happening as a side of reformatting\n>\n> s/side/side effect/ ?\n>\n\nThat's probably better.\n\n>> is now lost. But that was never the job of this section of the code,\n>> since the main intention is to simply rewrite the remaining refs post\n>> deletion of the selective few.\n>\n> Agreed.\n>\n>>\n>> This removes the unnecessary formatting operation involved. We can see a\n>> consistent ~20% performance improvement when deleting from packed\n>> references.\n>>\n>> Benchmark 1: update-ref: delete ref (refcount = 100000, revision = master)\n>>   Time (mean ± σ):      28.7 ms ±   1.7 ms    [User: 22.5 ms, System: 5.9 ms]\n>>   Range (min … max):    26.7 ms …  33.3 ms    46 runs\n>>\n>> Benchmark 2: update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs)\n>>   Time (mean ± σ):      23.8 ms ±   1.2 ms    [User: 17.5 ms, System: 6.0 ms]\n>>   Range (min … max):    22.1 ms …  27.7 ms    56 runs\n>>\n>> Summary\n>>   update-ref: delete ref (refcount = 100000, revision = b4/kn-speedup-packed-refs) ran\n>>     1.21 ± 0.09 times faster than update-ref: delete ref (refformat = files, refcount = 100000, revision = master)\n>>\n>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n>> ---\n>> Changes in v2:\n>> - Instead of using the existing function, introduce a new\n>>   `write_packed_entry_raw()`.\n>> - Modify the commit to also note that we lose sanitization.\n>\n> s/commit/commit message/\n>\n\nThis doesn't go into the commit itself, so I'll leave it as is.\n\n>> - Link to v1: https://patch.msgid.link/20260930-kn-speedup-packed-refs-v1-1-111cd03d9b0e@gmail.com\n>\n> Okay, I'm okay with this version.\n>\n> --\n> Laters,\n> Toon\n\nThanks for the review.\n"}]}