Re: [PATCH] packed-refs: use `fwrite()` when passing refs verbatim

Karthik Nayak [off-list ref] 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.

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 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 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.

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.

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.

-- 
Laters,
Toon