Junio C Hamano <gitster@xxxxxxxxx> writes: > Karthik Nayak <karthik.188@xxxxxxxxx> 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. > Thanks for the review! > 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. It definitely does, The only other typecast I did find for `strmap_put` was within 'strmap.h'. Nevertheless, I think it makes sense to make that change. strmap shouldn't modify the data provided. Perhaps #leftoverbits. Thanks
Attachment:
signature.asc
Description: PGP signature
