Re: [PATCH v3 0/4] fetch/receive: use batched reference updates

[Junio C Hamano <gitster@pobox.com>]

Karthik Nayak <karthik.188@gmail.com> writes:

> The git-fetch(1) and git-receive-pack(1) commands update references as
> part of the flow. Each reference update is treated as a single entity
> and a transaction is created for each update.
>
> This can be really slow, specifically in reference backends where there
> are optimizations which ensure a single transaction with 'N' reference
> update perform much faster than 'N' individual transactions. Also having
> 'N' individual transactions has buildup and teardown costs. These costs
> add up in repositories with a large number of references.
>
> Also specifically in the reftable backend, 'N' individual transactions
> would also trigger auto-compaction for every transaction.
>
> The reasoning for using individual transactions is because we want to
> allow partial updates of references in these commands. Using a single
> transaction would be an all-or-nothing scenario.
>
> Recently we introduced an in-between solution called batched reference
> updates in 23fc8e4f61 (refs: implement batch reference update support,
> 2025-04-08). This allows us to batch a set of reference updates, where
> individual updates can pass/fail without affecting the batch.
>
> This patch series, modifies both 'git-fetch(1)' and
> 'git-receive-pack(1)' to use this mechanism. With this, we see a
> significant performance boost:
>
> +---------------------+---------------+------------------+
> |                     | files backend | reftable backend |
> +---------------------+---------------+------------------+
> | git-fetch(1)        | 1.25x         | 22x              |
> | git-receive-pack(1) | 1.21x         | 18x              |
> +---------------------+---------------+------------------+

Very nice.

> The first and third patch handle the changes for 'git-fetch(1)' and
> 'git-receive-pack(1)' respectively. The second patch fixes a small
> memory leak I encountered while working on this series.
>
> This is based on top of master: 7a1d2bd0a5 (Merge branch 'master' of
> https://github.com/j6t/gitk, 2025-05-09). There were no conflicts
> observed with next or seen.
>
> Junio, since the release window for 2.50 is closing in. I would prefer
> we mark this for 2.51, so perhaps when/if we merge it to 'next', we let
> it bake there till the next release window opens. While all the tests
> pass, any bugs here would be high impact and it would be nice to catch
> it before it hits a release.

I've read the difference since the last iteration, "git diff @{1}",
and everything looked sensible.

Not an issue with this series at all, but one thing I wondered is if
it makes sense to change the type of strmap_get/strmap_put to deal
with "const void *".  That way, it would not be necessary to cast
away the constness like so:

>     -+	strmap_put(failed_refs, refname, ref_transaction_error_msg(err));
>     ++	strmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));

without harming the other side, namely

>     @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com
>      +		if (reported_error)
>      +			cmd->error_string = reported_error;
>      +		else if (strmap_contains(&failed_refs, cmd->ref_name))
>     -+			cmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));
>     ++			cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);

this piece of code.

It may not make sense, and even if it did, of course, it is totally
outside of this series.

Thanks.