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