{"thread":{"id":"56331","subject":"[PATCH 1/6] fetch: speed up lookup of want refs via commit-graph","startedAt":"2021-08-20T10:08:41Z","lastAt":"2021-09-08T00:09:02Z","messageCount":48,"participants":["Patrick Steinhardt","Derrick Stolee","Junio C Hamano","René Scharfe","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"433206","messageId":"6872979c4557204821d788dc3f5e1c8bef0a773c.1629452412.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"[PATCH 1/6] fetch: speed up lookup of want refs via commit-graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-20T10:08:28Z","receivedAt":"2021-08-20T10:08:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When updating our local refs based on the refs fetched from the remote,\nwe need to iterate through all requested refs and load their respective\ncommits such that we can determine whether they need to be appended to\nFETCH_HEAD or not. In cases where we're fetching from a remote with\nexceedingly many refs, resolving these refs can be quite expensive given\nthat we repeatedly need to unpack object headers for each of the\nreferenced objects.\n\nSpeed this up by opportunistcally trying to resolve object IDs via the\ncommit graph: more likely than not, they're going to be a commit anyway,\nand this lets us avoid having to unpack object headers completely in\ncase the object is a commit that is part of the commit-graph. This\nsignificantly speeds up mirror-fetches in a real-world repository with\n2.3M refs:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     56.942 s ±  0.449 s    [User: 53.360 s, System: 5.356 s]\n      Range (min … max):   56.372 s … 57.533 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     33.657 s ±  0.167 s    [User: 30.302 s, System: 5.181 s]\n      Range (min … max):   33.454 s … 33.844 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.69 ± 0.02 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 405afe9bdf..73f5b286d5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1131,11 +1131,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tcontinue;\n \t\t\t}\n \n-\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n-\t\t\t\t\t\t\t\t&rm->old_oid,\n-\t\t\t\t\t\t\t\t1);\n-\t\t\tif (!commit)\n-\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n+\t\t\tif (!commit) {\n+\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\t\t\t&rm->old_oid,\n+\t\t\t\t\t\t\t\t\t1);\n+\t\t\t\tif (!commit)\n+\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t\t\t}\n \n \t\t\tif (rm->fetch_head_status != want_status)\n \t\t\t\tcontinue;\n-- \n2.33.0\n\n"},{"id":"433207","messageId":"cover.1629452412.git.ps@pks.im","threadId":"56331","inReplyTo":null,"subject":"[PATCH 0/6] Speed up mirror-fetches with many refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-20T10:08:24Z","receivedAt":"2021-08-20T10:08:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI've taken another look at fetches in the context of repos with a huge\namount of refs. This time around, I've taken a look at mirror fetches:\nin our notorious repo with 2.3M refs, these mirror fetches can take up\nto several minutes of time even if there are no changes at all.\n\nAs it turns out, many of the issues are again caused by loading and\ndereferencing refs. This patch series thus mostly focusses on optimizing\nthe patterns there, where the biggest win is to opportunistically load\nrefs via commit-graphs. The following numbers were all calculated for a\nmirror-fetch of above 2.3M refs repo on the local disk:\n\n    - Patch 1 speeds up the way we look up commits when appending to\n      FETCH_HEAD via the commit-graph, resulting in a ~40% speedup.\n\n    - Patch 2 optimizes the way we check for object existence for a 7%\n      speedup.\n\n    - Patch 3 is a cleanup patch which changes the iterator functions\n      passed to our connectivity checks. I was hoping for a speedup\n      given that we can now avoid copying objects (which could have an\n      effect with 2.3M copied OIDs), but unfortunately it didn't. In any\n      case, I still think that the end result is much cleaner.\n\n    - Patch 4 optimizes git-fetch-pack(1) to use the commit-graph. This\n      is a small win of about ~2%. It's debatable whether this patch is\n      worth it.\n\n    - Patch 5 is a preparatory commit which refactors `fetch_refs()` to\n      be more readily extendable.\n\n    - Patch 6 optimizes an edge case where we're doing two connectivity\n      checks even if the first connectivity check noticed we already had\n      all objects locally available, skipping the fetch. This brings a\n      15% speedup.\n\nIn combination with my previous optimizations for git-fetch-pack(1) and\nthe connectivity check, this improves performance from 71s\n(ps/fetch-pack-load-refs-optim), to 54s (ps/connectivity-optim) to 26s\n(this series).\n\nNote that this series depends on ps/connectivity-optim and thus only\napplies on top of next.\n\nPatrick\n\n[1]: <08519b8ab6f395cffbcd5e530bfba6aaf64241a2.1628085347.git.ps@pks.im>\n\n\nPatrick Steinhardt (6):\n  fetch: speed up lookup of want refs via commit-graph\n  fetch: avoid unpacking headers in object existence check\n  connected: refactor iterator to return next object ID directly\n  fetch-pack: optimize loading of refs via commit graph\n  fetch: refactor fetch refs to be more extendable\n  fetch: avoid second connectivity check if we already have all objects\n\n builtin/clone.c        |  8 ++--\n builtin/fetch.c        | 84 +++++++++++++++++++++++-------------------\n builtin/receive-pack.c | 17 ++++-----\n connected.c            | 15 ++++----\n connected.h            |  2 +-\n fetch-pack.c           | 14 ++++---\n 6 files changed, 74 insertions(+), 66 deletions(-)\n\n-- \n2.33.0\n\n"},{"id":"433208","messageId":"d3dac607f2235c5913621813c443aa10b99c8fe8.1629452412.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"[PATCH 2/6] fetch: avoid unpacking headers in object existence check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-20T10:08:32Z","receivedAt":"2021-08-20T10:08:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When updating local refs after the fetch has transferred all objects, we\ndo an object existence test as a safety guard to avoid updating a ref to\nan object which we don't have. We do so via `oid_object_info()`: if it\nreturns an error, then we know the object does not exist.\n\nOne side effect of `oid_object_info()` is that it parses the object's\ntype, and to do so it must unpack the object header. This is completely\npointless: we don't care for the type, but only want to assert that the\nobject exists.\n\nRefactor the code to use `repo_has_object_file()`, which both makes the\ncode's intent clearer and is also faster because it does not unpack\nobject headers. In a real-world repo with 2.3M refs, this results in a\nsmall speedup when doing a mirror-fetch:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     33.686 s ±  0.176 s    [User: 30.119 s, System: 5.262 s]\n      Range (min … max):   33.512 s … 33.944 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     31.247 s ±  0.195 s    [User: 28.135 s, System: 5.066 s]\n      Range (min … max):   30.948 s … 31.472 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.08 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 73f5b286d5..5fd0f7c791 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -846,13 +846,11 @@ static int update_local_ref(struct ref *ref,\n \t\t\t    int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n-\tenum object_type type;\n \tstruct branch *current_branch = branch_get(NULL);\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n-\ttype = oid_object_info(the_repository, &ref->new_oid, NULL);\n-\tif (type < 0)\n+\tif (!repo_has_object_file(the_repository, &ref->new_oid))\n \t\tdie(_(\"object %s not found\"), oid_to_hex(&ref->new_oid));\n \n \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n-- \n2.33.0\n\n"},{"id":"433209","messageId":"3bdad7bc8b0debd44138a4d3df5744d5a245475d.1629452412.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"[PATCH 3/6] connected: refactor iterator to return next object ID directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-20T10:08:37Z","receivedAt":"2021-08-20T10:08:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The object ID iterator used by the connectivity checks returns the next\nobject ID via an out-parameter and then uses a return code to indicate\nwhether an item was found. This is a bit roundabout: instead of a\nseparate error code, we can just retrun the next object ID directly and\nuse `NULL` pointers as indicator that the iterator got no items left.\nFurthermore, this avoids a copy of the object ID.\n\nRefactor the iterator and all its implementations to return object IDs\ndirectly. While I was honestly hoping for a small speedup given that we\ncan now avoid a copy, both versions perform the same. Still, the end\nresult is easier to understand and thus it makes sense to keep this\nrefactoring regardless.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/clone.c        |  8 +++-----\n builtin/fetch.c        |  7 +++----\n builtin/receive-pack.c | 17 +++++++----------\n connected.c            | 15 ++++++++-------\n connected.h            |  2 +-\n fetch-pack.c           |  7 +++----\n 6 files changed, 25 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a74558f30c..817a651936 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -544,7 +544,7 @@ static void write_followtags(const struct ref *refs, const char *msg)\n \t}\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n@@ -555,13 +555,11 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \t */\n \twhile (ref && !ref->peer_ref)\n \t\tref = ref->next;\n-\t/* Returning -1 notes \"end of list\" to the caller. */\n \tif (!ref)\n-\t\treturn -1;\n+\t\treturn NULL;\n \n-\toidcpy(oid, &ref->old_oid);\n \t*rm = ref->next;\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n static void update_remote_refs(const struct ref *refs,\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5fd0f7c791..76ce73ccf5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -962,7 +962,7 @@ static int update_local_ref(struct ref *ref,\n \t}\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n@@ -970,10 +970,9 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \twhile (ref && ref->status == REF_STATUS_REJECT_SHALLOW)\n \t\tref = ref->next;\n \tif (!ref)\n-\t\treturn -1; /* end of the list */\n+\t\treturn NULL;\n \t*rm = ref->next;\n-\toidcpy(oid, &ref->old_oid);\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n struct fetch_head {\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex a419de5b38..0abda033bc 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1307,7 +1307,7 @@ static void refuse_unconfigured_deny_delete_current(void)\n \trp_error(\"%s\", _(refuse_unconfigured_deny_delete_current_msg));\n }\n \n-static int command_singleton_iterator(void *cb_data, struct object_id *oid);\n+static struct object_id *command_singleton_iterator(void *cb_data);\n static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n {\n \tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n@@ -1725,16 +1725,15 @@ static void check_aliased_updates(struct command *commands)\n \tstring_list_clear(&ref_list, 0);\n }\n \n-static int command_singleton_iterator(void *cb_data, struct object_id *oid)\n+static struct object_id *command_singleton_iterator(void *cb_data)\n {\n \tstruct command **cmd_list = cb_data;\n \tstruct command *cmd = *cmd_list;\n \n \tif (!cmd || is_null_oid(&cmd->new_oid))\n-\t\treturn -1; /* end of list */\n+\t\treturn NULL;\n \t*cmd_list = NULL; /* this returns only one */\n-\toidcpy(oid, &cmd->new_oid);\n-\treturn 0;\n+\treturn &cmd->new_oid;\n }\n \n static void set_connectivity_errors(struct command *commands,\n@@ -1764,7 +1763,7 @@ struct iterate_data {\n \tstruct shallow_info *si;\n };\n \n-static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n+static struct object_id *iterate_receive_command_list(void *cb_data)\n {\n \tstruct iterate_data *data = cb_data;\n \tstruct command **cmd_list = &data->cmds;\n@@ -1775,13 +1774,11 @@ static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n \t\t\t/* to be checked in update_shallow_ref() */\n \t\t\tcontinue;\n \t\tif (!is_null_oid(&cmd->new_oid) && !cmd->skip_update) {\n-\t\t\toidcpy(oid, &cmd->new_oid);\n \t\t\t*cmd_list = cmd->next;\n-\t\t\treturn 0;\n+\t\t\treturn &cmd->new_oid;\n \t\t}\n \t}\n-\t*cmd_list = NULL;\n-\treturn -1; /* end of list */\n+\treturn NULL;\n }\n \n static void reject_updates_to_hidden(struct command *commands)\ndiff --git a/connected.c b/connected.c\nindex b5f9523a5f..374b145355 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -24,7 +24,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \tstruct child_process rev_list = CHILD_PROCESS_INIT;\n \tFILE *rev_list_in;\n \tstruct check_connected_options defaults = CHECK_CONNECTED_INIT;\n-\tstruct object_id oid;\n+\tstruct object_id *oid;\n \tint err = 0;\n \tstruct packed_git *new_pack = NULL;\n \tstruct transport *transport;\n@@ -34,7 +34,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\topt = &defaults;\n \ttransport = opt->transport;\n \n-\tif (fn(cb_data, &oid)) {\n+\toid = fn(cb_data);\n+\tif (!oid) {\n \t\tif (opt->err_fd)\n \t\t\tclose(opt->err_fd);\n \t\treturn err;\n@@ -73,7 +74,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t\t\tif (!p->pack_promisor)\n \t\t\t\t\tcontinue;\n-\t\t\t\tif (find_pack_entry_one(oid.hash, p))\n+\t\t\t\tif (find_pack_entry_one(oid->hash, p))\n \t\t\t\t\tgoto promisor_pack_found;\n \t\t\t}\n \t\t\t/*\n@@ -83,7 +84,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t\tgoto no_promisor_pack_found;\n promisor_pack_found:\n \t\t\t;\n-\t\t} while (!fn(cb_data, &oid));\n+\t\t} while ((oid = fn(cb_data)) != NULL);\n \t\treturn 0;\n \t}\n \n@@ -133,12 +134,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t * are sure the ref is good and not sending it to\n \t\t * rev-list for verification.\n \t\t */\n-\t\tif (new_pack && find_pack_entry_one(oid.hash, new_pack))\n+\t\tif (new_pack && find_pack_entry_one(oid->hash, new_pack))\n \t\t\tcontinue;\n \n-\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(&oid)) < 0)\n+\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(oid)) < 0)\n \t\t\tbreak;\n-\t} while (!fn(cb_data, &oid));\n+\t} while ((oid = fn(cb_data)) != NULL);\n \n \tif (ferror(rev_list_in) || fflush(rev_list_in)) {\n \t\tif (errno != EPIPE && errno != EINVAL)\ndiff --git a/connected.h b/connected.h\nindex 8d5a6b3ad6..56cc95be2d 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -9,7 +9,7 @@ struct transport;\n  * When called after returning the name for the last object, return -1\n  * to signal EOF, otherwise return 0.\n  */\n-typedef int (*oid_iterate_fn)(void *, struct object_id *oid);\n+typedef struct object_id *(*oid_iterate_fn)(void *);\n \n /*\n  * Named-arguments struct for check_connected. All arguments are\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0bf7ed7e47..1a6242cd71 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1912,16 +1912,15 @@ static void update_shallow(struct fetch_pack_args *args,\n \toid_array_clear(&ref);\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n \n \tif (!ref)\n-\t\treturn -1; /* end of the list */\n+\t\treturn NULL;\n \t*rm = ref->next;\n-\toidcpy(oid, &ref->old_oid);\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n struct ref *fetch_pack(struct fetch_pack_args *args,\n-- \n2.33.0\n\n"},{"id":"433210","messageId":"67917af7ceeefe41ae0f6edf69cd61e2ee8c0ea3.1629452412.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"[PATCH 4/6] fetch-pack: optimize loading of refs via commit graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-20T10:08:41Z","receivedAt":"2021-08-20T10:08:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to negotiate a packfile, we need to dereference refs to see\nwhich commits we have in common with the remote. To do so, we first look\nup the object's type -- if it's a tag, we peel until we hit a non-tag\nobject. If we hit a commit eventually, then we return that commit.\n\nIn case the object ID points to a commit directly, we can avoid the\ninitial lookup of the object type by opportunistically looking up the\ncommit via the commit-graph, if available, which gives us a slight speed\nbump of about 2% in a huge repository with about 2.3M refs:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     31.634 s ±  0.258 s    [User: 28.400 s, System: 5.090 s]\n      Range (min … max):   31.280 s … 31.896 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     31.129 s ±  0.543 s    [User: 27.976 s, System: 5.056 s]\n      Range (min … max):   30.172 s … 31.479 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.02 ± 0.02 times faster than 'HEAD~: git-fetch'\n\nIn case this fails, we fall back to the old code which peels the\nobjects to a commit.\n---\n fetch-pack.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 1a6242cd71..c57faf278f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -119,6 +119,11 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n {\n \tenum object_type type;\n \tstruct object_info info = { .typep = &type };\n+\tstruct commit *commit;\n+\n+\tcommit = lookup_commit_in_graph(the_repository, oid);\n+\tif (commit)\n+\t\treturn commit;\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n@@ -139,7 +144,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n \t}\n \n \tif (type == OBJ_COMMIT) {\n-\t\tstruct commit *commit = lookup_commit(the_repository, oid);\n+\t\tcommit = lookup_commit(the_repository, oid);\n \t\tif (!commit || repo_parse_commit(the_repository, commit))\n \t\t\treturn NULL;\n \t\treturn commit;\n-- \n2.33.0\n\n"},{"id":"433211","messageId":"7653f8eabc1eb9c26e06ad3efa3d7e19dc9547cb.1629452412.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"[PATCH 5/6] fetch: refactor fetch refs to be more extendable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-20T10:08:45Z","receivedAt":"2021-08-20T10:08:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Refactor `fetch_refs()` code to make it more extendable by explicitly\nhandling error cases. The refactored code should behave the same.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 33 +++++++++++++++++++++------------\n 1 file changed, 21 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 76ce73ccf5..20fcfe0f45 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1284,20 +1284,29 @@ static int check_exist_and_connected(struct ref *ref_map)\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n {\n-\tint ret = check_exist_and_connected(ref_map);\n-\tif (ret) {\n-\t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n-\t\tret = transport_fetch_refs(transport, ref_map);\n-\t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n-\t}\n+\tint ret;\n+\n+\t/*\n+\t * We don't need to perform a fetch in case we can already satisfy all\n+\t * refs.\n+\t */\n+\tret = check_exist_and_connected(ref_map);\n \tif (!ret)\n-\t\t/*\n-\t\t * Keep the new pack's \".keep\" file around to allow the caller\n-\t\t * time to update refs to reference the new objects.\n-\t\t */\n \t\treturn 0;\n-\ttransport_unlock_pack(transport);\n-\treturn ret;\n+\n+\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n+\tret = transport_fetch_refs(transport, ref_map);\n+\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n+\tif (ret) {\n+\t\ttransport_unlock_pack(transport);\n+\t\treturn ret;\n+\t}\n+\n+\t/*\n+\t * Keep the new pack's \".keep\" file around to allow the caller\n+\t * time to update refs to reference the new objects.\n+\t */\n+\treturn 0;\n }\n \n /* Update local refs based on the ref values fetched from a remote */\n-- \n2.33.0\n\n"},{"id":"433212","messageId":"646ac90e62aab4e4aec595d6848b60233bbe8c77.1629452412.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"[PATCH 6/6] fetch: avoid second connectivity check if we already have all objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-20T10:08:49Z","receivedAt":"2021-08-20T10:08:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching refs, we are doing two connectivity checks:\n\n    - The first one in `fetch_refs()` is done such that we can\n      short-circuit the case where we already have all objects\n      referenced by the updated set of refs.\n\n    - The second one in `store_updated_refs()` does a sanity check that\n      we have all objects after we have fetched the packfile.\n\nWe always execute both connectivity checks, but this is wasteful in case\nthe first connectivity check already notices that we have all objects\nlocally available.\n\nRefactor the code to do both connectivity checks in `fetch_refs()`,\nwhich allows us to easily skip the second connectivity check if we\nalready have all objects available. This refactoring is safe to do given\nthat we always call `fetch_refs()` followed by `consume_refs()`, which\nis the only caller of `store_updated_refs()`.\n\nThis gives us a nice speedup when doing a mirror-fetch in a repository\nwith about 2.3M refs where the fetching repo already has all objects:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     31.232 s ±  0.082 s    [User: 27.901 s, System: 5.178 s]\n      Range (min … max):   31.118 s … 31.301 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     26.616 s ±  0.100 s    [User: 23.675 s, System: 4.752 s]\n      Range (min … max):   26.544 s … 26.788 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.17 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 20fcfe0f45..088a8af13b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1068,7 +1068,7 @@ N_(\"It took %.2f seconds to check forced updates. You can use\\n\"\n    \" to avoid this check.\\n\");\n \n static int store_updated_refs(const char *raw_url, const char *remote_name,\n-\t\t\t      int connectivity_checked, struct ref *ref_map)\n+\t\t\t      struct ref *ref_map)\n {\n \tstruct fetch_head fetch_head;\n \tstruct commit *commit;\n@@ -1090,16 +1090,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \telse\n \t\turl = xstrdup(\"foreign\");\n \n-\tif (!connectivity_checked) {\n-\t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n-\n-\t\trm = ref_map;\n-\t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n-\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n-\t\t\tgoto abort;\n-\t\t}\n-\t}\n-\n \tif (atomic_fetch) {\n \t\ttransaction = ref_transaction_begin(&err);\n \t\tif (!transaction) {\n@@ -1302,6 +1292,18 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n \t\treturn ret;\n \t}\n \n+\t/*\n+\t * If the transport didn't yet check for us, we need to verify\n+\t * ourselves that we have obtained all missing objects now.\n+\t */\n+\tif (!transport->smart_options || !transport->smart_options->connectivity_checked) {\n+\t\tif (check_connected(iterate_ref_map, &ref_map, NULL)) {\n+\t\t\tret = error(_(\"remote did not send all necessary objects\\n\"));\n+\t\t\ttransport_unlock_pack(transport);\n+\t\t\treturn ret;\n+\t\t}\n+\t}\n+\n \t/*\n \t * Keep the new pack's \".keep\" file around to allow the caller\n \t * time to update refs to reference the new objects.\n@@ -1312,13 +1314,10 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n /* Update local refs based on the ref values fetched from a remote */\n static int consume_refs(struct transport *transport, struct ref *ref_map)\n {\n-\tint connectivity_checked = transport->smart_options\n-\t\t? transport->smart_options->connectivity_checked : 0;\n \tint ret;\n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(transport->url,\n \t\t\t\t transport->remote->name,\n-\t\t\t\t connectivity_checked,\n \t\t\t\t ref_map);\n \ttransport_unlock_pack(transport);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n-- \n2.33.0\n\n"},{"id":"433218","messageId":"fce98460-eed6-c874-8c42-bf3a6f5f93a8@gmail.com","threadId":"56331","inReplyTo":"6872979c4557204821d788dc3f5e1c8bef0a773c.1629452412.git.ps@pks.im","subject":"Re: [PATCH 1/6] fetch: speed up lookup of want refs via commit-graph","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T14:27:59Z","receivedAt":"2021-08-20T14:28:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:\n> When updating our local refs based on the refs fetched from the remote,\n> we need to iterate through all requested refs and load their respective\n> commits such that we can determine whether they need to be appended to\n> FETCH_HEAD or not. In cases where we're fetching from a remote with\n> exceedingly many refs, resolving these refs can be quite expensive given\n> that we repeatedly need to unpack object headers for each of the\n> referenced objects.\n> \n> Speed this up by opportunistcally trying to resolve object IDs via the\n> commit graph: more likely than not, they're going to be a commit anyway,\n> and this lets us avoid having to unpack object headers completely in\n> case the object is a commit that is part of the commit-graph. This\n> significantly speeds up mirror-fetches in a real-world repository with\n> 2.3M refs:\n> \n>     Benchmark #1: HEAD~: git-fetch\n>       Time (mean ± σ):     56.942 s ±  0.449 s    [User: 53.360 s, System: 5.356 s]\n>       Range (min … max):   56.372 s … 57.533 s    5 runs\n> \n>     Benchmark #2: HEAD: git-fetch\n>       Time (mean ± σ):     33.657 s ±  0.167 s    [User: 30.302 s, System: 5.181 s]\n>       Range (min … max):   33.454 s … 33.844 s    5 runs\n> \n>     Summary\n>       'HEAD: git-fetch' ran\n>         1.69 ± 0.02 times faster than 'HEAD~: git-fetch'\n\nThese numbers are impressive, and it makes sense that performing a\nbinary search on the OID lookup chunk of the commit-graph is faster\nthan doing a binary search on the OIDs across the pack-index(es).\n\nI do worry about the case where annotated tags greatly outnumber\nbranches, so this binary search is extra overhead and the performance\nmay degrade. Would it be worth checking the ref to see if it lies\nwithin \"refs/heads/\" (or even _not_ in \"refs/tags/\") before doing\nthis commit-graph check?\n\n> -\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n> -\t\t\t\t\t\t\t\t&rm->old_oid,\n> -\t\t\t\t\t\t\t\t1);\n> -\t\t\tif (!commit)\n> -\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n> +\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n> +\t\t\tif (!commit) {\n> +\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n> +\t\t\t\t\t\t\t\t\t&rm->old_oid,\n> +\t\t\t\t\t\t\t\t\t1);\n> +\t\t\t\tif (!commit)\n> +\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n\nnit: I wouldn't nest this last \"if (!commit)\".\n\nCode will work as advertised.\n\nThanks,\n-Stolee\n"},{"id":"433219","messageId":"ffb5da19-937e-fba6-10f8-3993ca5ff0ef@gmail.com","threadId":"56331","inReplyTo":"3bdad7bc8b0debd44138a4d3df5744d5a245475d.1629452412.git.ps@pks.im","subject":"Re: [PATCH 3/6] connected: refactor iterator to return next object ID directly","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T14:32:32Z","receivedAt":"2021-08-20T14:32:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:\n> The object ID iterator used by the connectivity checks returns the next\n> object ID via an out-parameter and then uses a return code to indicate\n> whether an item was found. This is a bit roundabout: instead of a\n> separate error code, we can just retrun the next object ID directly and\n> use `NULL` pointers as indicator that the iterator got no items left.\n> Furthermore, this avoids a copy of the object ID.\n> \n> Refactor the iterator and all its implementations to return object IDs\n> directly. While I was honestly hoping for a small speedup given that we\n> can now avoid a copy, both versions perform the same. Still, the end\n> result is easier to understand and thus it makes sense to keep this\n> refactoring regardless.\n\nIt's too bad about the lack of measurable performance gains, but the\nnew code _is_ doing less, it's just not enough.\n\nI agree that the new code organization is better.\n\nThanks,\n-Stolee\n"},{"id":"433220","messageId":"d280d167-3e62-f73b-5504-9d1d8bdacaab@gmail.com","threadId":"56331","inReplyTo":"67917af7ceeefe41ae0f6edf69cd61e2ee8c0ea3.1629452412.git.ps@pks.im","subject":"Re: [PATCH 4/6] fetch-pack: optimize loading of refs via commit graph","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T14:37:33Z","receivedAt":"2021-08-20T14:37:37Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:\n> In order to negotiate a packfile, we need to dereference refs to see\n> which commits we have in common with the remote. To do so, we first look\n> up the object's type -- if it's a tag, we peel until we hit a non-tag\n> object. If we hit a commit eventually, then we return that commit.\n> \n> In case the object ID points to a commit directly, we can avoid the\n> initial lookup of the object type by opportunistically looking up the\n> commit via the commit-graph, if available, which gives us a slight speed\n> bump of about 2% in a huge repository with about 2.3M refs:\n> \n>     Benchmark #1: HEAD~: git-fetch\n>       Time (mean ± σ):     31.634 s ±  0.258 s    [User: 28.400 s, System: 5.090 s]\n>       Range (min … max):   31.280 s … 31.896 s    5 runs\n> \n>     Benchmark #2: HEAD: git-fetch\n>       Time (mean ± σ):     31.129 s ±  0.543 s    [User: 27.976 s, System: 5.056 s]\n>       Range (min … max):   30.172 s … 31.479 s    5 runs\n> \n>     Summary\n>       'HEAD: git-fetch' ran\n>         1.02 ± 0.02 times faster than 'HEAD~: git-fetch'\n\nThis 2% gain is nice, especially because you are measuring the\nend-to-end scenario. If you use GIT_TRACE2_PERF=1 on a few runs,\nthen you could likely isolate some of the regions from\nmark_complete_and_common_ref() and demonstrate a larger improvement\nin that focused area.\n\n> @@ -119,6 +119,11 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n>  {\n>  \tenum object_type type;\n>  \tstruct object_info info = { .typep = &type };\n> +\tstruct commit *commit;\n> +\n> +\tcommit = lookup_commit_in_graph(the_repository, oid);\n> +\tif (commit)\n> +\t\treturn commit;\n\nObviously a correct thing to do.\n\n>  \tif (type == OBJ_COMMIT) {\n> -\t\tstruct commit *commit = lookup_commit(the_repository, oid);\n> +\t\tcommit = lookup_commit(the_repository, oid);\n\nRe-using the local simplifies this. Good.\n\nThanks,\n-Stolee\n"},{"id":"433221","messageId":"f0da7080-1621-5951-1272-044e4bbf48b7@gmail.com","threadId":"56331","inReplyTo":"7653f8eabc1eb9c26e06ad3efa3d7e19dc9547cb.1629452412.git.ps@pks.im","subject":"Re: [PATCH 5/6] fetch: refactor fetch refs to be more extendable","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T14:41:26Z","receivedAt":"2021-08-20T14:41:30Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:\n> Refactor `fetch_refs()` code to make it more extendable by explicitly\n> handling error cases. The refactored code should behave the same.\n\nIt took unrolling this diff to understand that this code behaves the\nsame, and it's because of the previous code using \"if (!ret) return 0;\"\nto handle two possible ways that 'ret' could become zero.\n\nI agree that the new code makes it clear that we can leave early after\na successful call to check_exist_and_connected() and again after a\nsuccessful call to transport_fetch_refs().\n\nThanks\n-Stolee\n"},{"id":"433222","messageId":"1d5ae688-fa08-6c8c-345f-9b0389e21dd7@gmail.com","threadId":"56331","inReplyTo":"646ac90e62aab4e4aec595d6848b60233bbe8c77.1629452412.git.ps@pks.im","subject":"Re: [PATCH 6/6] fetch: avoid second connectivity check if we already have all objects","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T14:47:11Z","receivedAt":"2021-08-20T14:47:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:\n> When fetching refs, we are doing two connectivity checks:\n> \n>     - The first one in `fetch_refs()` is done such that we can\n>       short-circuit the case where we already have all objects\n>       referenced by the updated set of refs.\n> \n>     - The second one in `store_updated_refs()` does a sanity check that\n>       we have all objects after we have fetched the packfile.\n> \n> We always execute both connectivity checks, but this is wasteful in case\n> the first connectivity check already notices that we have all objects\n> locally available.\n> \n> Refactor the code to do both connectivity checks in `fetch_refs()`,\n> which allows us to easily skip the second connectivity check if we\n> already have all objects available. This refactoring is safe to do given\n> that we always call `fetch_refs()` followed by `consume_refs()`, which\n> is the only caller of `store_updated_refs()`.\n\nShould we try to make it more clear that fetch_refs() must be followed\nby consume_refs() via a comment above the fetch_refs(), or possibly even\nits call sites?\n\nThanks,\n-Stolee\n"},{"id":"433223","messageId":"857d18a1-97b2-b7a6-64d4-4b31daf79e64@gmail.com","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"Re: [PATCH 0/6] Speed up mirror-fetches with many refs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-20T14:50:29Z","receivedAt":"2021-08-20T14:50:34Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:\n...\n> As it turns out, many of the issues are again caused by loading and\n> dereferencing refs. This patch series thus mostly focusses on optimizing\n> the patterns there, where the biggest win is to opportunistically load\n> refs via commit-graphs.\n\nYou caught my attention at \"commit-graph\" and I found your use of them\nto be interesting. You strike a balance in checking the commit-graph\nwhen it is likely to be helpful, and skip the commit-graph when it is\nnot. (For example, PATCH 2 is unlikely to benefit from checking the\ncommit-graph at that point, because we are looking for objects that\nwere just downloaded.)\n\nI read all the patches and checked the full context of the functions\nto see if there were any issues, but found none. My only comments are\nabout the case of many annotated tags (do we slow down?) and some\nnitpicks.\n\nThanks,\n-Stolee\n"},{"id":"433248","messageId":"xmqq7dggvxup.fsf@gitster.g","threadId":"56331","inReplyTo":"fce98460-eed6-c874-8c42-bf3a6f5f93a8@gmail.com","subject":"Re: [PATCH 1/6] fetch: speed up lookup of want refs via commit-graph","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-20T17:18:22Z","receivedAt":"2021-08-20T17:18:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> I do worry about the case where annotated tags greatly outnumber\n> branches, so this binary search is extra overhead and the performance\n> may degrade. Would it be worth checking the ref to see if it lies\n> within \"refs/heads/\" (or even _not_ in \"refs/tags/\") before doing\n> this commit-graph check?\n\nAh, clever.\n"},{"id":"433257","messageId":"xmqqmtpcui4v.fsf@gitster.g","threadId":"56331","inReplyTo":"ffb5da19-937e-fba6-10f8-3993ca5ff0ef@gmail.com","subject":"Re: [PATCH 3/6] connected: refactor iterator to return next object ID directly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-20T17:43:12Z","receivedAt":"2021-08-20T17:43:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:\n>> The object ID iterator used by the connectivity checks returns the next\n>> object ID via an out-parameter and then uses a return code to indicate\n>> whether an item was found. This is a bit roundabout: instead of a\n>> separate error code, we can just retrun the next object ID directly and\n>> use `NULL` pointers as indicator that the iterator got no items left.\n>> Furthermore, this avoids a copy of the object ID.\n>> \n>> Refactor the iterator and all its implementations to return object IDs\n>> directly. While I was honestly hoping for a small speedup given that we\n>> can now avoid a copy, both versions perform the same. Still, the end\n>> result is easier to understand and thus it makes sense to keep this\n>> refactoring regardless.\n>\n> It's too bad about the lack of measurable performance gains, but the\n> new code _is_ doing less, it's just not enough.\n>\n> I agree that the new code organization is better.\n\nThe only potential downside I can think of is the loss of ability to\nconvey the reason why it failed to return one by adding new return\ncodes from the function, which I do not immediately see all that\nuseful future extension anyway, so I agree that \"we find what we\nfound, or NULL if we don't find\" is much straight-forward and easier\nto understand.\n\nNicely done.\n\nThanks.\n"},{"id":"433258","messageId":"5f9e7a4f-d95b-4242-1add-7b20057293a3@web.de","threadId":"56331","inReplyTo":"3bdad7bc8b0debd44138a4d3df5744d5a245475d.1629452412.git.ps@pks.im","subject":"Re: [PATCH 3/6] connected: refactor iterator to return next object ID directly","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-08-20T17:43:06Z","receivedAt":"2021-08-20T17:43:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.08.21 um 12:08 schrieb Patrick Steinhardt:\n> The object ID iterator used by the connectivity checks returns the next\n> object ID via an out-parameter and then uses a return code to indicate\n> whether an item was found. This is a bit roundabout: instead of a\n> separate error code, we can just retrun the next object ID directly and\n\ns/retrun/return/\n\n> use `NULL` pointers as indicator that the iterator got no items left.\n> Furthermore, this avoids a copy of the object ID.\n>\n> Refactor the iterator and all its implementations to return object IDs\n> directly. While I was honestly hoping for a small speedup given that we\n> can now avoid a copy, both versions perform the same. Still, the end\n> result is easier to understand and thus it makes sense to keep this\n> refactoring regardless.\n\ncheck_connected() calls find_pack_entry_one() on the object ID hash,\nwhich copies it anyway.  Perhaps that and caching prevent the expected\nspeedup?\n\nThe private copy made sure check_connected() could not modify the\nobject IDs.  It still only reads them with this patch, but the compiler\nno longer prevents writes.  The iterators could return const pointers\nto restore that guarantee.\n\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/clone.c        |  8 +++-----\n>  builtin/fetch.c        |  7 +++----\n>  builtin/receive-pack.c | 17 +++++++----------\n>  connected.c            | 15 ++++++++-------\n>  connected.h            |  2 +-\n>  fetch-pack.c           |  7 +++----\n>  6 files changed, 25 insertions(+), 31 deletions(-)\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index a74558f30c..817a651936 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -544,7 +544,7 @@ static void write_followtags(const struct ref *refs, const char *msg)\n>  \t}\n>  }\n>\n> -static int iterate_ref_map(void *cb_data, struct object_id *oid)\n> +static struct object_id *iterate_ref_map(void *cb_data)\n>  {\n>  \tstruct ref **rm = cb_data;\n>  \tstruct ref *ref = *rm;\n> @@ -555,13 +555,11 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n>  \t */\n>  \twhile (ref && !ref->peer_ref)\n>  \t\tref = ref->next;\n> -\t/* Returning -1 notes \"end of list\" to the caller. */\n>  \tif (!ref)\n> -\t\treturn -1;\n> +\t\treturn NULL;\n>\n> -\toidcpy(oid, &ref->old_oid);\n>  \t*rm = ref->next;\n> -\treturn 0;\n> +\treturn &ref->old_oid;\n>  }\n>\n>  static void update_remote_refs(const struct ref *refs,\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 5fd0f7c791..76ce73ccf5 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -962,7 +962,7 @@ static int update_local_ref(struct ref *ref,\n>  \t}\n>  }\n>\n> -static int iterate_ref_map(void *cb_data, struct object_id *oid)\n> +static struct object_id *iterate_ref_map(void *cb_data)\n>  {\n>  \tstruct ref **rm = cb_data;\n>  \tstruct ref *ref = *rm;\n> @@ -970,10 +970,9 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n>  \twhile (ref && ref->status == REF_STATUS_REJECT_SHALLOW)\n>  \t\tref = ref->next;\n>  \tif (!ref)\n> -\t\treturn -1; /* end of the list */\n> +\t\treturn NULL;\n>  \t*rm = ref->next;\n> -\toidcpy(oid, &ref->old_oid);\n> -\treturn 0;\n> +\treturn &ref->old_oid;\n>  }\n>\n>  struct fetch_head {\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index a419de5b38..0abda033bc 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1307,7 +1307,7 @@ static void refuse_unconfigured_deny_delete_current(void)\n>  \trp_error(\"%s\", _(refuse_unconfigured_deny_delete_current_msg));\n>  }\n>\n> -static int command_singleton_iterator(void *cb_data, struct object_id *oid);\n> +static struct object_id *command_singleton_iterator(void *cb_data);\n>  static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n>  {\n>  \tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n> @@ -1725,16 +1725,15 @@ static void check_aliased_updates(struct command *commands)\n>  \tstring_list_clear(&ref_list, 0);\n>  }\n>\n> -static int command_singleton_iterator(void *cb_data, struct object_id *oid)\n> +static struct object_id *command_singleton_iterator(void *cb_data)\n>  {\n>  \tstruct command **cmd_list = cb_data;\n>  \tstruct command *cmd = *cmd_list;\n>\n>  \tif (!cmd || is_null_oid(&cmd->new_oid))\n> -\t\treturn -1; /* end of list */\n> +\t\treturn NULL;\n>  \t*cmd_list = NULL; /* this returns only one */\n> -\toidcpy(oid, &cmd->new_oid);\n> -\treturn 0;\n> +\treturn &cmd->new_oid;\n>  }\n>\n>  static void set_connectivity_errors(struct command *commands,\n> @@ -1764,7 +1763,7 @@ struct iterate_data {\n>  \tstruct shallow_info *si;\n>  };\n>\n> -static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n> +static struct object_id *iterate_receive_command_list(void *cb_data)\n>  {\n>  \tstruct iterate_data *data = cb_data;\n>  \tstruct command **cmd_list = &data->cmds;\n> @@ -1775,13 +1774,11 @@ static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n>  \t\t\t/* to be checked in update_shallow_ref() */\n>  \t\t\tcontinue;\n>  \t\tif (!is_null_oid(&cmd->new_oid) && !cmd->skip_update) {\n> -\t\t\toidcpy(oid, &cmd->new_oid);\n>  \t\t\t*cmd_list = cmd->next;\n> -\t\t\treturn 0;\n> +\t\t\treturn &cmd->new_oid;\n>  \t\t}\n>  \t}\n> -\t*cmd_list = NULL;\n> -\treturn -1; /* end of list */\n> +\treturn NULL;\n>  }\n>\n>  static void reject_updates_to_hidden(struct command *commands)\n> diff --git a/connected.c b/connected.c\n> index b5f9523a5f..374b145355 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -24,7 +24,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \tstruct child_process rev_list = CHILD_PROCESS_INIT;\n>  \tFILE *rev_list_in;\n>  \tstruct check_connected_options defaults = CHECK_CONNECTED_INIT;\n> -\tstruct object_id oid;\n> +\tstruct object_id *oid;\n>  \tint err = 0;\n>  \tstruct packed_git *new_pack = NULL;\n>  \tstruct transport *transport;\n> @@ -34,7 +34,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \t\topt = &defaults;\n>  \ttransport = opt->transport;\n>\n> -\tif (fn(cb_data, &oid)) {\n> +\toid = fn(cb_data);\n> +\tif (!oid) {\n>  \t\tif (opt->err_fd)\n>  \t\t\tclose(opt->err_fd);\n>  \t\treturn err;\n> @@ -73,7 +74,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \t\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n>  \t\t\t\tif (!p->pack_promisor)\n>  \t\t\t\t\tcontinue;\n> -\t\t\t\tif (find_pack_entry_one(oid.hash, p))\n> +\t\t\t\tif (find_pack_entry_one(oid->hash, p))\n>  \t\t\t\t\tgoto promisor_pack_found;\n>  \t\t\t}\n>  \t\t\t/*\n> @@ -83,7 +84,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \t\t\tgoto no_promisor_pack_found;\n>  promisor_pack_found:\n>  \t\t\t;\n> -\t\t} while (!fn(cb_data, &oid));\n> +\t\t} while ((oid = fn(cb_data)) != NULL);\n>  \t\treturn 0;\n>  \t}\n>\n> @@ -133,12 +134,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \t\t * are sure the ref is good and not sending it to\n>  \t\t * rev-list for verification.\n>  \t\t */\n> -\t\tif (new_pack && find_pack_entry_one(oid.hash, new_pack))\n> +\t\tif (new_pack && find_pack_entry_one(oid->hash, new_pack))\n>  \t\t\tcontinue;\n>\n> -\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(&oid)) < 0)\n> +\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(oid)) < 0)\n>  \t\t\tbreak;\n> -\t} while (!fn(cb_data, &oid));\n> +\t} while ((oid = fn(cb_data)) != NULL);\n>\n>  \tif (ferror(rev_list_in) || fflush(rev_list_in)) {\n>  \t\tif (errno != EPIPE && errno != EINVAL)\n> diff --git a/connected.h b/connected.h\n> index 8d5a6b3ad6..56cc95be2d 100644\n> --- a/connected.h\n> +++ b/connected.h\n> @@ -9,7 +9,7 @@ struct transport;\n>   * When called after returning the name for the last object, return -1\n>   * to signal EOF, otherwise return 0.\n>   */\n> -typedef int (*oid_iterate_fn)(void *, struct object_id *oid);\n> +typedef struct object_id *(*oid_iterate_fn)(void *);\n\nI.e. this would be safer (requires some more const modifiers throughout\nthe code):\n\ntypedef const struct object_id * (*oid_iterate_fn)(void *);\n\n>\n>  /*\n>   * Named-arguments struct for check_connected. All arguments are\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 0bf7ed7e47..1a6242cd71 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -1912,16 +1912,15 @@ static void update_shallow(struct fetch_pack_args *args,\n>  \toid_array_clear(&ref);\n>  }\n>\n> -static int iterate_ref_map(void *cb_data, struct object_id *oid)\n> +static struct object_id *iterate_ref_map(void *cb_data)\n>  {\n>  \tstruct ref **rm = cb_data;\n>  \tstruct ref *ref = *rm;\n>\n>  \tif (!ref)\n> -\t\treturn -1; /* end of the list */\n> +\t\treturn NULL;\n>  \t*rm = ref->next;\n> -\toidcpy(oid, &ref->old_oid);\n> -\treturn 0;\n> +\treturn &ref->old_oid;\n>  }\n>\n>  struct ref *fetch_pack(struct fetch_pack_args *args,\n>\n\n"},{"id":"433287","messageId":"xmqq1r6nu08l.fsf@gitster.g","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"Re: [PATCH 0/6] Speed up mirror-fetches with many refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-21T00:09:46Z","receivedAt":"2021-08-21T00:09:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I've taken another look at fetches in the context of repos with a huge\n> amount of refs. This time around, I've taken a look at mirror fetches:\n> in our notorious repo with 2.3M refs, these mirror fetches can take up\n> to several minutes of time even if there are no changes at all.\n\nI notice that 4/6 (and no other patch) is not signed-off and wonder\nif there is a reason (e.g. you are not so comfortable with the idea\nbehind the step or the implementation) or a simple oversight?\n\n> Note that this series depends on ps/connectivity-optim and thus only\n> applies on top of next.\n\nIt seems that the dependency of this series is not just 'master'\nplus 'ps/connectivity-optim' but some more stuff from 'next'.  I\nexpact that topics that have been cooking in 'next' during the\nprevious cycle will graduate to 'master' early next week, so perhaps\nit is easier to handle this kind of \"depends on some stuff in\n'next'\" topics after that happens.\n\nThanks.\n"},{"id":"433345","messageId":"YSNEQ9R0TDUNPZPM@ncase","threadId":"56331","inReplyTo":"xmqq7dggvxup.fsf@gitster.g","subject":"Re: [PATCH 1/6] fetch: speed up lookup of want refs via commit-graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-23T06:46:27Z","receivedAt":"2021-08-23T06:46:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 20, 2021 at 10:18:22AM -0700, Junio C Hamano wrote:\n> Derrick Stolee <stolee@gmail.com> writes:\n> \n> > I do worry about the case where annotated tags greatly outnumber\n> > branches, so this binary search is extra overhead and the performance\n> > may degrade. Would it be worth checking the ref to see if it lies\n> > within \"refs/heads/\" (or even _not_ in \"refs/tags/\") before doing\n> > this commit-graph check?\n> \n> Ah, clever.\n\nGood idea. Benchmarks for my test repository (which definitely isn't\nrepresentative, but it's at least some numbers) show that restricting to\n\"refs/heads/\" diminishes almost all the gains, while restricting to\neverything but \"refs/tags/\" performs almost the same (it's a tiny bit\nslower, probably because of the added string comparisons):\n\n    Benchmark #1: all refs: git-fetch\n      Time (mean ± σ):     32.959 s ±  0.282 s    [User: 29.801 s, System: 5.137 s]\n      Range (min … max):   32.760 s … 33.158 s    2 runs\n\n    Benchmark #2: refs/heads: git-fetch\n      Time (mean ± σ):     56.955 s ±  0.002 s    [User: 53.447 s, System: 5.362 s]\n      Range (min … max):   56.953 s … 56.957 s    2 runs\n\n    Benchmark #3: !refs/tags: git-fetch\n      Time (mean ± σ):     33.447 s ±  0.003 s    [User: 30.160 s, System: 5.027 s]\n      Range (min … max):   33.444 s … 33.449 s    2 runs\n\n    Summary\n      'all refs: git-fetch' ran\n        1.01 ± 0.01 times faster than '!refs/tags: git-fetch'\n        1.73 ± 0.01 times faster than 'refs/heads: git-fetch'\n\nThis is easily explained by the fact that the test repo has most of its\nrefs neither in \"refs/tags/\" nor in \"refs/heads/\", but rather in special\nnamespaces like \"refs/merge-requests/\", \"refs/environments/\" or\n\"refs/keep-around/\".\n\nI like the idea of excluding \"refs/tags/\" though: as you point out,\nchances are high that these don't point to commits but to annotated tags\ninstead. So I'll go with that, thanks!\n\nPatrick\n"},{"id":"433346","messageId":"YSNEmsO1ZIQXHs6X@ncase","threadId":"56331","inReplyTo":"5f9e7a4f-d95b-4242-1add-7b20057293a3@web.de","subject":"Re: [PATCH 3/6] connected: refactor iterator to return next object ID directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-23T06:47:54Z","receivedAt":"2021-08-23T06:48:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 20, 2021 at 07:43:06PM +0200, René Scharfe wrote:\n> Am 20.08.21 um 12:08 schrieb Patrick Steinhardt:\n> > The object ID iterator used by the connectivity checks returns the next\n> > object ID via an out-parameter and then uses a return code to indicate\n> > whether an item was found. This is a bit roundabout: instead of a\n> > separate error code, we can just retrun the next object ID directly and\n> \n> s/retrun/return/\n> \n> > use `NULL` pointers as indicator that the iterator got no items left.\n> > Furthermore, this avoids a copy of the object ID.\n> >\n> > Refactor the iterator and all its implementations to return object IDs\n> > directly. While I was honestly hoping for a small speedup given that we\n> > can now avoid a copy, both versions perform the same. Still, the end\n> > result is easier to understand and thus it makes sense to keep this\n> > refactoring regardless.\n> \n> check_connected() calls find_pack_entry_one() on the object ID hash,\n> which copies it anyway.  Perhaps that and caching prevent the expected\n> speedup?\n> \n> The private copy made sure check_connected() could not modify the\n> object IDs.  It still only reads them with this patch, but the compiler\n> no longer prevents writes.  The iterators could return const pointers\n> to restore that guarantee.\n\nRight, will change the signature and re-benchmark. I'd be surprised if\nit significantly changed the picture, but let's see.\n\nPatrick\n"},{"id":"433347","messageId":"YSNFx9FdSWOy6b45@ncase","threadId":"56331","inReplyTo":"1d5ae688-fa08-6c8c-345f-9b0389e21dd7@gmail.com","subject":"Re: [PATCH 6/6] fetch: avoid second connectivity check if we already have all objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-23T06:52:55Z","receivedAt":"2021-08-23T06:53:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 20, 2021 at 10:47:11AM -0400, Derrick Stolee wrote:\n> On 8/20/2021 6:08 AM, Patrick Steinhardt wrote:\n> > When fetching refs, we are doing two connectivity checks:\n> > \n> >     - The first one in `fetch_refs()` is done such that we can\n> >       short-circuit the case where we already have all objects\n> >       referenced by the updated set of refs.\n> > \n> >     - The second one in `store_updated_refs()` does a sanity check that\n> >       we have all objects after we have fetched the packfile.\n> > \n> > We always execute both connectivity checks, but this is wasteful in case\n> > the first connectivity check already notices that we have all objects\n> > locally available.\n> > \n> > Refactor the code to do both connectivity checks in `fetch_refs()`,\n> > which allows us to easily skip the second connectivity check if we\n> > already have all objects available. This refactoring is safe to do given\n> > that we always call `fetch_refs()` followed by `consume_refs()`, which\n> > is the only caller of `store_updated_refs()`.\n> \n> Should we try to make it more clear that fetch_refs() must be followed\n> by consume_refs() via a comment above the fetch_refs(), or possibly even\n> its call sites?\n\nI wasn't quite happy with this outcome, either. How about we instead\nmerge both functions into `fetch_and_consume_refs()`? Both are quite\nshort, and that makes sure we always call them together to make the\nrequirement explicit.\n\nI'll add another patch to do this refactoring.\n\nPatrick\n"},{"id":"433530","messageId":"4a819a68309bf03db2d9a5e5be070e52c3542af8.1629800774.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"[PATCH v2 1/7] fetch: speed up lookup of want refs via commit-graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-24T10:36:55Z","receivedAt":"2021-08-24T10:37:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When updating our local refs based on the refs fetched from the remote,\nwe need to iterate through all requested refs and load their respective\ncommits such that we can determine whether they need to be appended to\nFETCH_HEAD or not. In cases where we're fetching from a remote with\nexceedingly many refs, resolving these refs can be quite expensive given\nthat we repeatedly need to unpack object headers for each of the\nreferenced objects.\n\nSpeed this up by opportunistcally trying to resolve object IDs via the\ncommit graph. We only do so for any refs which are not in \"refs/tags\":\nmore likely than not, these are going to be a commit anyway, and this\nlets us avoid having to unpack object headers completely in case the\nobject is a commit that is part of the commit-graph. This significantly\nspeeds up mirror-fetches in a real-world repository with\n2.3M refs:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     56.482 s ±  0.384 s    [User: 53.340 s, System: 5.365 s]\n      Range (min … max):   56.050 s … 57.045 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     33.727 s ±  0.170 s    [User: 30.252 s, System: 5.194 s]\n      Range (min … max):   33.452 s … 33.871 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.67 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 24 ++++++++++++++++++------\n 1 file changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e064687dbd..91d1301613 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1074,7 +1074,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t      int connectivity_checked, struct ref *ref_map)\n {\n \tstruct fetch_head fetch_head;\n-\tstruct commit *commit;\n \tint url_len, i, rc = 0;\n \tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n \tstruct ref_transaction *transaction = NULL;\n@@ -1122,6 +1121,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t     want_status <= FETCH_HEAD_IGNORE;\n \t     want_status++) {\n \t\tfor (rm = ref_map; rm; rm = rm->next) {\n+\t\t\tstruct commit *commit = NULL;\n \t\t\tstruct ref *ref = NULL;\n \n \t\t\tif (rm->status == REF_STATUS_REJECT_SHALLOW) {\n@@ -1131,11 +1131,23 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tcontinue;\n \t\t\t}\n \n-\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n-\t\t\t\t\t\t\t\t&rm->old_oid,\n-\t\t\t\t\t\t\t\t1);\n-\t\t\tif (!commit)\n-\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t\t\t/*\n+\t\t\t * References in \"refs/tags/\" are often going to point\n+\t\t\t * to annotated tags, which are not part of the\n+\t\t\t * commit-graph. We thus only try to look up refs in\n+\t\t\t * the graph which are not in that namespace to not\n+\t\t\t * regress performance in repositories with many\n+\t\t\t * annotated tags.\n+\t\t\t */\n+\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n+\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n+\t\t\tif (!commit) {\n+\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\t\t\t&rm->old_oid,\n+\t\t\t\t\t\t\t\t\t1);\n+\t\t\t\tif (!commit)\n+\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t\t\t}\n \n \t\t\tif (rm->fetch_head_status != want_status)\n \t\t\t\tcontinue;\n-- \n2.33.0\n\n"},{"id":"433531","messageId":"81ebadabe8e861269336f6446f87acac88371042.1629800774.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"[PATCH v2 2/7] fetch: avoid unpacking headers in object existence check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-24T10:37:00Z","receivedAt":"2021-08-24T10:37:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When updating local refs after the fetch has transferred all objects, we\ndo an object existence test as a safety guard to avoid updating a ref to\nan object which we don't have. We do so via `oid_object_info()`: if it\nreturns an error, then we know the object does not exist.\n\nOne side effect of `oid_object_info()` is that it parses the object's\ntype, and to do so it must unpack the object header. This is completely\npointless: we don't care for the type, but only want to assert that the\nobject exists.\n\nRefactor the code to use `repo_has_object_file()`, which both makes the\ncode's intent clearer and is also faster because it does not unpack\nobject headers. In a real-world repo with 2.3M refs, this results in a\nsmall speedup when doing a mirror-fetch:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     33.686 s ±  0.176 s    [User: 30.119 s, System: 5.262 s]\n      Range (min … max):   33.512 s … 33.944 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     31.247 s ±  0.195 s    [User: 28.135 s, System: 5.066 s]\n      Range (min … max):   30.948 s … 31.472 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.08 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 91d1301613..01513e6aea 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -846,13 +846,11 @@ static int update_local_ref(struct ref *ref,\n \t\t\t    int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n-\tenum object_type type;\n \tstruct branch *current_branch = branch_get(NULL);\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n-\ttype = oid_object_info(the_repository, &ref->new_oid, NULL);\n-\tif (type < 0)\n+\tif (!repo_has_object_file(the_repository, &ref->new_oid))\n \t\tdie(_(\"object %s not found\"), oid_to_hex(&ref->new_oid));\n \n \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n-- \n2.33.0\n\n"},{"id":"433532","messageId":"cover.1629800774.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"[PATCH v2 0/7] Speed up mirror-fetches with many refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-24T10:36:51Z","receivedAt":"2021-08-24T10:37:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series to speed up mirror-fetches\nwith many refs. This topic applies on top of Junio's 9d5700f60b (Merge\nbranch 'ps/connectivity-optim' into jch, 2021-08-23).\n\nChanges compared to v1:\n\n    - Patch 1/7: I've applied Stolee's proposal to only\n      opportunistically load objects via the commit-graph in case the\n      reference is not in refs/tags/ such that we don't regress repos\n      with many annotated tags.\n\n    - Patch 3/7: The return parameter of the iterator is now const to\n      allow further optimizations by the compiler, as suggested by\n      René. I've also re-benchmarked this, and one can now see a very\n      slight performance improvement of ~1%.\n\n    - Patch 4/7: Added my missing DCO, as pointed out by Junio.\n\n    - Patch 5, 6, 7: I've redone these to make it clearer that the\n      refactoring I'm doing doesn't cause us to miss any object\n      connectivity checks. Most importantly, I've merged `fetch_refs()`\n      and `consume_refs()` into `fetch_and_consume_refs()` in 6/7, which\n      makes the optimization where we elide the second connectivity\n      check in 7/7 trivial.\n\nThanks for your feedback!\n\nPatrick\n\nPatrick Steinhardt (7):\n  fetch: speed up lookup of want refs via commit-graph\n  fetch: avoid unpacking headers in object existence check\n  connected: refactor iterator to return next object ID directly\n  fetch-pack: optimize loading of refs via commit graph\n  fetch: refactor fetch refs to be more extendable\n  fetch: merge fetching and consuming refs\n  fetch: avoid second connectivity check if we already have all objects\n\n builtin/clone.c        |  8 ++---\n builtin/fetch.c        | 74 +++++++++++++++++++++++-------------------\n builtin/receive-pack.c | 17 ++++------\n connected.c            | 15 +++++----\n connected.h            |  2 +-\n fetch-pack.c           | 14 +++++---\n 6 files changed, 68 insertions(+), 62 deletions(-)\n\nRange-diff against v1:\n1:  6872979c45 ! 1:  4a819a6830 fetch: speed up lookup of want refs via commit-graph\n    @@ Commit message\n         referenced objects.\n     \n         Speed this up by opportunistcally trying to resolve object IDs via the\n    -    commit graph: more likely than not, they're going to be a commit anyway,\n    -    and this lets us avoid having to unpack object headers completely in\n    -    case the object is a commit that is part of the commit-graph. This\n    -    significantly speeds up mirror-fetches in a real-world repository with\n    +    commit graph. We only do so for any refs which are not in \"refs/tags\":\n    +    more likely than not, these are going to be a commit anyway, and this\n    +    lets us avoid having to unpack object headers completely in case the\n    +    object is a commit that is part of the commit-graph. This significantly\n    +    speeds up mirror-fetches in a real-world repository with\n         2.3M refs:\n     \n             Benchmark #1: HEAD~: git-fetch\n    -          Time (mean ± σ):     56.942 s ±  0.449 s    [User: 53.360 s, System: 5.356 s]\n    -          Range (min … max):   56.372 s … 57.533 s    5 runs\n    +          Time (mean ± σ):     56.482 s ±  0.384 s    [User: 53.340 s, System: 5.365 s]\n    +          Range (min … max):   56.050 s … 57.045 s    5 runs\n     \n             Benchmark #2: HEAD: git-fetch\n    -          Time (mean ± σ):     33.657 s ±  0.167 s    [User: 30.302 s, System: 5.181 s]\n    -          Range (min … max):   33.454 s … 33.844 s    5 runs\n    +          Time (mean ± σ):     33.727 s ±  0.170 s    [User: 30.252 s, System: 5.194 s]\n    +          Range (min … max):   33.452 s … 33.871 s    5 runs\n     \n             Summary\n               'HEAD: git-fetch' ran\n    -            1.69 ± 0.02 times faster than 'HEAD~: git-fetch'\n    +            1.67 ± 0.01 times faster than 'HEAD~: git-fetch'\n     \n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n      ## builtin/fetch.c ##\n    +@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n    + \t\t\t      int connectivity_checked, struct ref *ref_map)\n    + {\n    + \tstruct fetch_head fetch_head;\n    +-\tstruct commit *commit;\n    + \tint url_len, i, rc = 0;\n    + \tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n    + \tstruct ref_transaction *transaction = NULL;\n    +@@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n    + \t     want_status <= FETCH_HEAD_IGNORE;\n    + \t     want_status++) {\n    + \t\tfor (rm = ref_map; rm; rm = rm->next) {\n    ++\t\t\tstruct commit *commit = NULL;\n    + \t\t\tstruct ref *ref = NULL;\n    + \n    + \t\t\tif (rm->status == REF_STATUS_REJECT_SHALLOW) {\n     @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *remote_name,\n      \t\t\t\tcontinue;\n      \t\t\t}\n    @@ builtin/fetch.c: static int store_updated_refs(const char *raw_url, const char *\n     -\t\t\t\t\t\t\t\t1);\n     -\t\t\tif (!commit)\n     -\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n    -+\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n    ++\t\t\t/*\n    ++\t\t\t * References in \"refs/tags/\" are often going to point\n    ++\t\t\t * to annotated tags, which are not part of the\n    ++\t\t\t * commit-graph. We thus only try to look up refs in\n    ++\t\t\t * the graph which are not in that namespace to not\n    ++\t\t\t * regress performance in repositories with many\n    ++\t\t\t * annotated tags.\n    ++\t\t\t */\n    ++\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n    ++\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n     +\t\t\tif (!commit) {\n     +\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n     +\t\t\t\t\t\t\t\t\t&rm->old_oid,\n2:  d3dac607f2 = 2:  81ebadabe8 fetch: avoid unpacking headers in object existence check\n3:  3bdad7bc8b ! 3:  98e981ced9 connected: refactor iterator to return next object ID directly\n    @@ Commit message\n         The object ID iterator used by the connectivity checks returns the next\n         object ID via an out-parameter and then uses a return code to indicate\n         whether an item was found. This is a bit roundabout: instead of a\n    -    separate error code, we can just retrun the next object ID directly and\n    +    separate error code, we can just return the next object ID directly and\n         use `NULL` pointers as indicator that the iterator got no items left.\n         Furthermore, this avoids a copy of the object ID.\n     \n         Refactor the iterator and all its implementations to return object IDs\n    -    directly. While I was honestly hoping for a small speedup given that we\n    -    can now avoid a copy, both versions perform the same. Still, the end\n    -    result is easier to understand and thus it makes sense to keep this\n    -    refactoring regardless.\n    +    directly. This brings a tiny performance improvement when doing a mirror-fetch of a repository with about 2.3M refs:\n    +\n    +        Benchmark #1: 328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch\n    +          Time (mean ± σ):     30.110 s ±  0.148 s    [User: 27.161 s, System: 5.075 s]\n    +          Range (min … max):   29.934 s … 30.406 s    10 runs\n    +\n    +        Benchmark #2: 328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch\n    +          Time (mean ± σ):     29.899 s ±  0.109 s    [User: 26.916 s, System: 5.104 s]\n    +          Range (min … max):   29.696 s … 29.996 s    10 runs\n    +\n    +        Summary\n    +          '328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch' ran\n    +            1.01 ± 0.01 times faster than '328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch'\n    +\n    +    While this 1% speedup could be labelled as statistically insignificant,\n    +    the speedup is consistent on my machine. Furthermore, this is an end to\n    +    end test, so it is expected that the improvement in the connectivity\n    +    check itself is more significant.\n     \n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n     \n    @@ builtin/clone.c: static void write_followtags(const struct ref *refs, const char\n      }\n      \n     -static int iterate_ref_map(void *cb_data, struct object_id *oid)\n    -+static struct object_id *iterate_ref_map(void *cb_data)\n    ++static const struct object_id *iterate_ref_map(void *cb_data)\n      {\n      \tstruct ref **rm = cb_data;\n      \tstruct ref *ref = *rm;\n    @@ builtin/fetch.c: static int update_local_ref(struct ref *ref,\n      }\n      \n     -static int iterate_ref_map(void *cb_data, struct object_id *oid)\n    -+static struct object_id *iterate_ref_map(void *cb_data)\n    ++static const struct object_id *iterate_ref_map(void *cb_data)\n      {\n      \tstruct ref **rm = cb_data;\n      \tstruct ref *ref = *rm;\n    @@ builtin/receive-pack.c: static void refuse_unconfigured_deny_delete_current(void\n      }\n      \n     -static int command_singleton_iterator(void *cb_data, struct object_id *oid);\n    -+static struct object_id *command_singleton_iterator(void *cb_data);\n    ++static const struct object_id *command_singleton_iterator(void *cb_data);\n      static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n      {\n      \tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n    @@ builtin/receive-pack.c: static void check_aliased_updates(struct command *comman\n      }\n      \n     -static int command_singleton_iterator(void *cb_data, struct object_id *oid)\n    -+static struct object_id *command_singleton_iterator(void *cb_data)\n    ++static const struct object_id *command_singleton_iterator(void *cb_data)\n      {\n      \tstruct command **cmd_list = cb_data;\n      \tstruct command *cmd = *cmd_list;\n    @@ builtin/receive-pack.c: struct iterate_data {\n      };\n      \n     -static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n    -+static struct object_id *iterate_receive_command_list(void *cb_data)\n    ++static const struct object_id *iterate_receive_command_list(void *cb_data)\n      {\n      \tstruct iterate_data *data = cb_data;\n      \tstruct command **cmd_list = &data->cmds;\n    @@ connected.c: int check_connected(oid_iterate_fn fn, void *cb_data,\n      \tFILE *rev_list_in;\n      \tstruct check_connected_options defaults = CHECK_CONNECTED_INIT;\n     -\tstruct object_id oid;\n    -+\tstruct object_id *oid;\n    ++\tconst struct object_id *oid;\n      \tint err = 0;\n      \tstruct packed_git *new_pack = NULL;\n      \tstruct transport *transport;\n    @@ connected.h: struct transport;\n       * to signal EOF, otherwise return 0.\n       */\n     -typedef int (*oid_iterate_fn)(void *, struct object_id *oid);\n    -+typedef struct object_id *(*oid_iterate_fn)(void *);\n    ++typedef const struct object_id *(*oid_iterate_fn)(void *);\n      \n      /*\n       * Named-arguments struct for check_connected. All arguments are\n    @@ fetch-pack.c: static void update_shallow(struct fetch_pack_args *args,\n      }\n      \n     -static int iterate_ref_map(void *cb_data, struct object_id *oid)\n    -+static struct object_id *iterate_ref_map(void *cb_data)\n    ++static const struct object_id *iterate_ref_map(void *cb_data)\n      {\n      \tstruct ref **rm = cb_data;\n      \tstruct ref *ref = *rm;\n4:  67917af7ce ! 4:  6311203f08 fetch-pack: optimize loading of refs via commit graph\n    @@ Commit message\n         In case this fails, we fall back to the old code which peels the\n         objects to a commit.\n     \n    +    Signed-off-by: Patrick Steinhardt <ps@pks.im>\n    +\n      ## fetch-pack.c ##\n     @@ fetch-pack.c: static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n      {\n5:  7653f8eabc ! 5:  56a9158ac3 fetch: refactor fetch refs to be more extendable\n    @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n      static int fetch_refs(struct transport *transport, struct ref *ref_map)\n      {\n     -\tint ret = check_exist_and_connected(ref_map);\n    --\tif (ret) {\n    --\t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n    --\t\tret = transport_fetch_refs(transport, ref_map);\n    --\t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n    --\t}\n     +\tint ret;\n     +\n     +\t/*\n    @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n     +\t * refs.\n     +\t */\n     +\tret = check_exist_and_connected(ref_map);\n    - \tif (!ret)\n    + \tif (ret) {\n    + \t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n    + \t\tret = transport_fetch_refs(transport, ref_map);\n    + \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n    ++\t\tif (ret) {\n    ++\t\t\ttransport_unlock_pack(transport);\n    ++\t\t\treturn ret;\n    ++\t\t}\n    + \t}\n    +-\tif (!ret)\n     -\t\t/*\n     -\t\t * Keep the new pack's \".keep\" file around to allow the caller\n     -\t\t * time to update refs to reference the new objects.\n     -\t\t */\n    - \t\treturn 0;\n    +-\t\treturn 0;\n     -\ttransport_unlock_pack(transport);\n     -\treturn ret;\n     +\n    -+\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n    -+\tret = transport_fetch_refs(transport, ref_map);\n    -+\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n    -+\tif (ret) {\n    -+\t\ttransport_unlock_pack(transport);\n    -+\t\treturn ret;\n    -+\t}\n    -+\n     +\t/*\n     +\t * Keep the new pack's \".keep\" file around to allow the caller\n     +\t * time to update refs to reference the new objects.\n6:  646ac90e62 < -:  ---------- fetch: avoid second connectivity check if we already have all objects\n-:  ---------- > 6:  31d9f72edf fetch: merge fetching and consuming refs\n-:  ---------- > 7:  84e39c847f fetch: avoid second connectivity check if we already have all objects\n-- \n2.33.0\n\n"},{"id":"433533","messageId":"98e981ced94b2e75ac043d0707ef025fbdc9542b.1629800774.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"[PATCH v2 3/7] connected: refactor iterator to return next object ID directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-24T10:37:04Z","receivedAt":"2021-08-24T10:37:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The object ID iterator used by the connectivity checks returns the next\nobject ID via an out-parameter and then uses a return code to indicate\nwhether an item was found. This is a bit roundabout: instead of a\nseparate error code, we can just return the next object ID directly and\nuse `NULL` pointers as indicator that the iterator got no items left.\nFurthermore, this avoids a copy of the object ID.\n\nRefactor the iterator and all its implementations to return object IDs\ndirectly. This brings a tiny performance improvement when doing a mirror-fetch of a repository with about 2.3M refs:\n\n    Benchmark #1: 328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch\n      Time (mean ± σ):     30.110 s ±  0.148 s    [User: 27.161 s, System: 5.075 s]\n      Range (min … max):   29.934 s … 30.406 s    10 runs\n\n    Benchmark #2: 328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch\n      Time (mean ± σ):     29.899 s ±  0.109 s    [User: 26.916 s, System: 5.104 s]\n      Range (min … max):   29.696 s … 29.996 s    10 runs\n\n    Summary\n      '328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch' ran\n        1.01 ± 0.01 times faster than '328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch'\n\nWhile this 1% speedup could be labelled as statistically insignificant,\nthe speedup is consistent on my machine. Furthermore, this is an end to\nend test, so it is expected that the improvement in the connectivity\ncheck itself is more significant.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/clone.c        |  8 +++-----\n builtin/fetch.c        |  7 +++----\n builtin/receive-pack.c | 17 +++++++----------\n connected.c            | 15 ++++++++-------\n connected.h            |  2 +-\n fetch-pack.c           |  7 +++----\n 6 files changed, 25 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 66fe66679c..4a1056fcc2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -657,7 +657,7 @@ static void write_followtags(const struct ref *refs, const char *msg)\n \t}\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static const struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n@@ -668,13 +668,11 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \t */\n \twhile (ref && !ref->peer_ref)\n \t\tref = ref->next;\n-\t/* Returning -1 notes \"end of list\" to the caller. */\n \tif (!ref)\n-\t\treturn -1;\n+\t\treturn NULL;\n \n-\toidcpy(oid, &ref->old_oid);\n \t*rm = ref->next;\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n static void update_remote_refs(const struct ref *refs,\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 01513e6aea..cdf0d0d671 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -962,7 +962,7 @@ static int update_local_ref(struct ref *ref,\n \t}\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static const struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n@@ -970,10 +970,9 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \twhile (ref && ref->status == REF_STATUS_REJECT_SHALLOW)\n \t\tref = ref->next;\n \tif (!ref)\n-\t\treturn -1; /* end of the list */\n+\t\treturn NULL;\n \t*rm = ref->next;\n-\toidcpy(oid, &ref->old_oid);\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n struct fetch_head {\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2d1f97e1ca..041e915454 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1306,7 +1306,7 @@ static void refuse_unconfigured_deny_delete_current(void)\n \trp_error(\"%s\", _(refuse_unconfigured_deny_delete_current_msg));\n }\n \n-static int command_singleton_iterator(void *cb_data, struct object_id *oid);\n+static const struct object_id *command_singleton_iterator(void *cb_data);\n static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n {\n \tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n@@ -1731,16 +1731,15 @@ static void check_aliased_updates(struct command *commands)\n \tstring_list_clear(&ref_list, 0);\n }\n \n-static int command_singleton_iterator(void *cb_data, struct object_id *oid)\n+static const struct object_id *command_singleton_iterator(void *cb_data)\n {\n \tstruct command **cmd_list = cb_data;\n \tstruct command *cmd = *cmd_list;\n \n \tif (!cmd || is_null_oid(&cmd->new_oid))\n-\t\treturn -1; /* end of list */\n+\t\treturn NULL;\n \t*cmd_list = NULL; /* this returns only one */\n-\toidcpy(oid, &cmd->new_oid);\n-\treturn 0;\n+\treturn &cmd->new_oid;\n }\n \n static void set_connectivity_errors(struct command *commands,\n@@ -1770,7 +1769,7 @@ struct iterate_data {\n \tstruct shallow_info *si;\n };\n \n-static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n+static const struct object_id *iterate_receive_command_list(void *cb_data)\n {\n \tstruct iterate_data *data = cb_data;\n \tstruct command **cmd_list = &data->cmds;\n@@ -1781,13 +1780,11 @@ static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n \t\t\t/* to be checked in update_shallow_ref() */\n \t\t\tcontinue;\n \t\tif (!is_null_oid(&cmd->new_oid) && !cmd->skip_update) {\n-\t\t\toidcpy(oid, &cmd->new_oid);\n \t\t\t*cmd_list = cmd->next;\n-\t\t\treturn 0;\n+\t\t\treturn &cmd->new_oid;\n \t\t}\n \t}\n-\t*cmd_list = NULL;\n-\treturn -1; /* end of list */\n+\treturn NULL;\n }\n \n static void reject_updates_to_hidden(struct command *commands)\ndiff --git a/connected.c b/connected.c\nindex b5f9523a5f..cf68e37a97 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -24,7 +24,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \tstruct child_process rev_list = CHILD_PROCESS_INIT;\n \tFILE *rev_list_in;\n \tstruct check_connected_options defaults = CHECK_CONNECTED_INIT;\n-\tstruct object_id oid;\n+\tconst struct object_id *oid;\n \tint err = 0;\n \tstruct packed_git *new_pack = NULL;\n \tstruct transport *transport;\n@@ -34,7 +34,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\topt = &defaults;\n \ttransport = opt->transport;\n \n-\tif (fn(cb_data, &oid)) {\n+\toid = fn(cb_data);\n+\tif (!oid) {\n \t\tif (opt->err_fd)\n \t\t\tclose(opt->err_fd);\n \t\treturn err;\n@@ -73,7 +74,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t\t\tif (!p->pack_promisor)\n \t\t\t\t\tcontinue;\n-\t\t\t\tif (find_pack_entry_one(oid.hash, p))\n+\t\t\t\tif (find_pack_entry_one(oid->hash, p))\n \t\t\t\t\tgoto promisor_pack_found;\n \t\t\t}\n \t\t\t/*\n@@ -83,7 +84,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t\tgoto no_promisor_pack_found;\n promisor_pack_found:\n \t\t\t;\n-\t\t} while (!fn(cb_data, &oid));\n+\t\t} while ((oid = fn(cb_data)) != NULL);\n \t\treturn 0;\n \t}\n \n@@ -133,12 +134,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t * are sure the ref is good and not sending it to\n \t\t * rev-list for verification.\n \t\t */\n-\t\tif (new_pack && find_pack_entry_one(oid.hash, new_pack))\n+\t\tif (new_pack && find_pack_entry_one(oid->hash, new_pack))\n \t\t\tcontinue;\n \n-\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(&oid)) < 0)\n+\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(oid)) < 0)\n \t\t\tbreak;\n-\t} while (!fn(cb_data, &oid));\n+\t} while ((oid = fn(cb_data)) != NULL);\n \n \tif (ferror(rev_list_in) || fflush(rev_list_in)) {\n \t\tif (errno != EPIPE && errno != EINVAL)\ndiff --git a/connected.h b/connected.h\nindex 8d5a6b3ad6..6e59c92aa3 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -9,7 +9,7 @@ struct transport;\n  * When called after returning the name for the last object, return -1\n  * to signal EOF, otherwise return 0.\n  */\n-typedef int (*oid_iterate_fn)(void *, struct object_id *oid);\n+typedef const struct object_id *(*oid_iterate_fn)(void *);\n \n /*\n  * Named-arguments struct for check_connected. All arguments are\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0bf7ed7e47..e6ec79f81a 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1912,16 +1912,15 @@ static void update_shallow(struct fetch_pack_args *args,\n \toid_array_clear(&ref);\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static const struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n \n \tif (!ref)\n-\t\treturn -1; /* end of the list */\n+\t\treturn NULL;\n \t*rm = ref->next;\n-\toidcpy(oid, &ref->old_oid);\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n struct ref *fetch_pack(struct fetch_pack_args *args,\n-- \n2.33.0\n\n"},{"id":"433534","messageId":"6311203f08219a8fa3884af5d288340a3fa5f47a.1629800774.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"[PATCH v2 4/7] fetch-pack: optimize loading of refs via commit graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-24T10:37:09Z","receivedAt":"2021-08-24T10:37:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to negotiate a packfile, we need to dereference refs to see\nwhich commits we have in common with the remote. To do so, we first look\nup the object's type -- if it's a tag, we peel until we hit a non-tag\nobject. If we hit a commit eventually, then we return that commit.\n\nIn case the object ID points to a commit directly, we can avoid the\ninitial lookup of the object type by opportunistically looking up the\ncommit via the commit-graph, if available, which gives us a slight speed\nbump of about 2% in a huge repository with about 2.3M refs:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     31.634 s ±  0.258 s    [User: 28.400 s, System: 5.090 s]\n      Range (min … max):   31.280 s … 31.896 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     31.129 s ±  0.543 s    [User: 27.976 s, System: 5.056 s]\n      Range (min … max):   30.172 s … 31.479 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.02 ± 0.02 times faster than 'HEAD~: git-fetch'\n\nIn case this fails, we fall back to the old code which peels the\nobjects to a commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n fetch-pack.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex e6ec79f81a..dc800879cb 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -119,6 +119,11 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n {\n \tenum object_type type;\n \tstruct object_info info = { .typep = &type };\n+\tstruct commit *commit;\n+\n+\tcommit = lookup_commit_in_graph(the_repository, oid);\n+\tif (commit)\n+\t\treturn commit;\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n@@ -139,7 +144,7 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n \t}\n \n \tif (type == OBJ_COMMIT) {\n-\t\tstruct commit *commit = lookup_commit(the_repository, oid);\n+\t\tcommit = lookup_commit(the_repository, oid);\n \t\tif (!commit || repo_parse_commit(the_repository, commit))\n \t\t\treturn NULL;\n \t\treturn commit;\n-- \n2.33.0\n\n"},{"id":"433535","messageId":"56a9158ac331f9911a4347d7d4afc2bbd2cf4d33.1629800774.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"[PATCH v2 5/7] fetch: refactor fetch refs to be more extendable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-24T10:37:13Z","receivedAt":"2021-08-24T10:37:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Refactor `fetch_refs()` code to make it more extendable by explicitly\nhandling error cases. The refactored code should behave the same.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 26 +++++++++++++++++---------\n 1 file changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex cdf0d0d671..da0e283288 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1293,20 +1293,28 @@ static int check_exist_and_connected(struct ref *ref_map)\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n {\n-\tint ret = check_exist_and_connected(ref_map);\n+\tint ret;\n+\n+\t/*\n+\t * We don't need to perform a fetch in case we can already satisfy all\n+\t * refs.\n+\t */\n+\tret = check_exist_and_connected(ref_map);\n \tif (ret) {\n \t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n \t\tret = transport_fetch_refs(transport, ref_map);\n \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n+\t\tif (ret) {\n+\t\t\ttransport_unlock_pack(transport);\n+\t\t\treturn ret;\n+\t\t}\n \t}\n-\tif (!ret)\n-\t\t/*\n-\t\t * Keep the new pack's \".keep\" file around to allow the caller\n-\t\t * time to update refs to reference the new objects.\n-\t\t */\n-\t\treturn 0;\n-\ttransport_unlock_pack(transport);\n-\treturn ret;\n+\n+\t/*\n+\t * Keep the new pack's \".keep\" file around to allow the caller\n+\t * time to update refs to reference the new objects.\n+\t */\n+\treturn 0;\n }\n \n /* Update local refs based on the ref values fetched from a remote */\n-- \n2.33.0\n\n"},{"id":"433536","messageId":"84e39c847f8258f0a0b392a2752dab00237c9e8f.1629800774.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"[PATCH v2 7/7] fetch: avoid second connectivity check if we already have all objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-24T10:37:21Z","receivedAt":"2021-08-24T10:37:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching refs, we are doing two connectivity checks:\n\n    - The first one is done such that we can skip fetching refs in the\n      case where we already have all objects referenced by the updated\n      set of refs.\n\n    - The second one verifies that we have all objects after we have\n      fetched objects.\n\nWe always execute both connectivity checks, but this is wasteful in case\nthe first connectivity check already notices that we have all objects\nlocally available.\n\nSkip the second connectivity check in case we already had all objects\navailable. This gives us a nice speedup when doing a mirror-fetch in a\nrepository with about 2.3M refs where the fetching repo already has all\nobjects:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     30.025 s ±  0.081 s    [User: 27.070 s, System: 4.933 s]\n      Range (min … max):   29.900 s … 30.111 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     25.574 s ±  0.177 s    [User: 22.855 s, System: 4.683 s]\n      Range (min … max):   25.399 s … 25.765 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.17 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a1e17edd8b..e2c952ec67 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1293,7 +1293,7 @@ static int check_exist_and_connected(struct ref *ref_map)\n \n static int fetch_and_consume_refs(struct transport *transport, struct ref *ref_map)\n {\n-\tint connectivity_checked;\n+\tint connectivity_checked = 1;\n \tint ret;\n \n \t/*\n@@ -1307,11 +1307,10 @@ static int fetch_and_consume_refs(struct transport *transport, struct ref *ref_m\n \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n \t\tif (ret)\n \t\t\tgoto out;\n+\t\tconnectivity_checked = transport->smart_options ?\n+\t\t\ttransport->smart_options->connectivity_checked : 0;\n \t}\n \n-\tconnectivity_checked = transport->smart_options\n-\t\t? transport->smart_options->connectivity_checked : 0;\n-\n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(transport->url,\n \t\t\t\t transport->remote->name,\n-- \n2.33.0\n\n"},{"id":"433537","messageId":"31d9f72edf5c178b2e80c30bb7c0a9bc164ca5eb.1629800774.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"[PATCH v2 6/7] fetch: merge fetching and consuming refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-24T10:37:17Z","receivedAt":"2021-08-24T10:37:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The functions `fetch_refs()` and `consume_refs()` must always be called\ntogether such that we first obtain all missing objects and then update\nour local refs to match the remote refs. In a subsequent patch, we'll\nfurther require that `fetch_refs()` must always be called before\n`consume_refs()` such that it can correctly assert that we have all\nobjects after the fetch given that we're about to move the connectivity\ncheck.\n\nMake this requirement explicit by merging both functions into a single\n`fetch_and_consume_refs()` function.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 32 +++++++++++---------------------\n 1 file changed, 11 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex da0e283288..a1e17edd8b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1291,8 +1291,9 @@ static int check_exist_and_connected(struct ref *ref_map)\n \treturn check_connected(iterate_ref_map, &rm, &opt);\n }\n \n-static int fetch_refs(struct transport *transport, struct ref *ref_map)\n+static int fetch_and_consume_refs(struct transport *transport, struct ref *ref_map)\n {\n+\tint connectivity_checked;\n \tint ret;\n \n \t/*\n@@ -1304,32 +1305,22 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n \t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n \t\tret = transport_fetch_refs(transport, ref_map);\n \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n-\t\tif (ret) {\n-\t\t\ttransport_unlock_pack(transport);\n-\t\t\treturn ret;\n-\t\t}\n+\t\tif (ret)\n+\t\t\tgoto out;\n \t}\n \n-\t/*\n-\t * Keep the new pack's \".keep\" file around to allow the caller\n-\t * time to update refs to reference the new objects.\n-\t */\n-\treturn 0;\n-}\n-\n-/* Update local refs based on the ref values fetched from a remote */\n-static int consume_refs(struct transport *transport, struct ref *ref_map)\n-{\n-\tint connectivity_checked = transport->smart_options\n+\tconnectivity_checked = transport->smart_options\n \t\t? transport->smart_options->connectivity_checked : 0;\n-\tint ret;\n+\n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(transport->url,\n \t\t\t\t transport->remote->name,\n \t\t\t\t connectivity_checked,\n \t\t\t\t ref_map);\n-\ttransport_unlock_pack(transport);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n+\n+out:\n+\ttransport_unlock_pack(transport);\n \treturn ret;\n }\n \n@@ -1518,8 +1509,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tif (!fetch_refs(transport, ref_map))\n-\t\tconsume_refs(transport, ref_map);\n+\tfetch_and_consume_refs(transport, ref_map);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n@@ -1610,7 +1600,7 @@ static int do_fetch(struct transport *transport,\n \t\t\t\t   transport->url);\n \t\t}\n \t}\n-\tif (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) {\n+\tif (fetch_and_consume_refs(transport, ref_map)) {\n \t\tfree_refs(ref_map);\n \t\tretcode = 1;\n \t\tgoto cleanup;\n-- \n2.33.0\n\n"},{"id":"433675","messageId":"xmqqfsuylarw.fsf@gitster.g","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"Re: [PATCH v2 0/7] Speed up mirror-fetches with many refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-24T22:48:19Z","receivedAt":"2021-08-24T22:48:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> this is the second version of my patch series to speed up mirror-fetches\n> with many refs. This topic applies on top of Junio's 9d5700f60b (Merge\n> branch 'ps/connectivity-optim' into jch, 2021-08-23).\n\nIt is a horrible commit to base anything on.  You are taking your\npatches hostage to all of these other topics.\n\n    9d5700f60b Merge branch 'ps/connectivity-optim' into jch\n    7ad315de2f Merge branch 'js/log-protocol-version' into jch\n    1726f748f5 Merge branch 'en/ort-becomes-the-default' into jch\n    23aeecb099 Merge branch 'en/merge-strategy-docs' into jch\n    568277d458 Merge branch 'en/pull-conflicting-options' into jch\n    2b316bb006 ### match next\n    4efa9ea0b6 Merge branch 'ps/fetch-pack-load-refs-optim' into jch\n    b305842ee8 Merge branch 'jt/push-negotiation-fixes' into jch\n    83b45616f1 Merge branch 'es/trace2-log-parent-process-name' into jch\n    be89aa8c38 Merge branch 'hn/refs-test-cleanup' into jch\n    256d56ed32 Merge branch 'en/ort-perf-batch-15' into jch\n    7477fbf53a Merge branch 'js/expand-runtime-prefix' into jch\n    b1453dfd30 Merge branch 'ab/bundle-doc' into jch\n    1b66e8e89d Merge branch 'zh/ref-filter-raw-data' into jch\n    1fbf27ddcd Merge branch 'ab/pack-stdin-packs-fix' into jch\n    dcf57bfebb Merge branch 'ab/http-drop-old-curl' into jch\n    93041f7c57 Merge branch 'ds/add-with-sparse-index' into jch\n    814a016195 Merge branch 'jc/bisect-sans-show-branch' into jch\n\nA better way to handle a situation like this is to limit your\ndependencies more explicitly.  If you look at what I did to the last\nround of this topic, you'll see that there is a merge of the\n'ps/connectivity-optim' topic into v2.33 followed by application of\nthe patches, like this:\n\n    1d576ca7b2 fetch: avoid second connectivity check if we already have all objects\n    6768595f10 fetch: refactor fetch refs to be more extendable\n    a615d7cf87 fetch-pack: optimize loading of refs via commit graph\n    bfd04fc24c connected: refactor iterator to return next object ID directly\n    1a387c9f3a fetch: avoid unpacking headers in object existence check\n    f1a4367ec4 fetch: speed up lookup of want refs via commit-graph\n    3628199d4d Merge branch 'ps/connectivity-optim' into ps/fetch-optim\n\nWhat I did to your last round was to merge 'ps/connectivity-optim'\non top of v2.33 and then queue them.  You can do the same for this\nround (you can tell people \"apply these on top of the result of\nmerging topic X, Y and Z on tag V\").\n\n    df52ef2c3a fetch: avoid second connectivity check if we already have all objects\n    c1721680e4 fetch: merge fetching and consuming refs\n    5470cbe1be fetch: refactor fetch refs to be more extendable\n    016a510428 fetch-pack: optimize loading of refs via commit graph\n    f6c7e63cc7 connected: refactor iterator to return next object ID directly\n    17c8e90df3 fetch: avoid unpacking headers in object existence check\n    a54c245004 fetch: speed up lookup of want refs via commit-graph\n    3628199d4d Merge branch 'ps/connectivity-optim' into ps/fetch-optim\n\nI had to adjust [4/7] while applying them on top of the same\n3628199d4d I created for queuing the previous round, and it would be\nappreciated if you can double-check the result.\n\nThanks.\n"},{"id":"433712","messageId":"YSXdVpSbWtjBkREs@ncase","threadId":"56331","inReplyTo":"xmqqfsuylarw.fsf@gitster.g","subject":"Re: [PATCH v2 0/7] Speed up mirror-fetches with many refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-08-25T06:04:06Z","receivedAt":"2021-08-25T06:04:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Aug 24, 2021 at 03:48:19PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> A better way to handle a situation like this is to limit your\n> dependencies more explicitly.  If you look at what I did to the last\n> round of this topic, you'll see that there is a merge of the\n> 'ps/connectivity-optim' topic into v2.33 followed by application of\n> the patches, like this:\n\nI wasn't quite sure how to best handle this, but I'll keep this in mind\nfor future iterations/patch series. Thanks for the explanation.\n\n[snip]\n> I had to adjust [4/7] while applying them on top of the same\n> 3628199d4d I created for queuing the previous round, and it would be\n> appreciated if you can double-check the result.\n\nThe result looks good to me, thanks.\n\nPatrick\n"},{"id":"433737","messageId":"b2b2316e-3d2c-4561-3da8-2d3a5ca243e0@gmail.com","threadId":"56331","inReplyTo":"YSNEQ9R0TDUNPZPM@ncase","subject":"Re: [PATCH 1/6] fetch: speed up lookup of want refs via commit-graph","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-25T14:12:55Z","receivedAt":"2021-08-25T14:13:02Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/23/2021 2:46 AM, Patrick Steinhardt wrote:\n> On Fri, Aug 20, 2021 at 10:18:22AM -0700, Junio C Hamano wrote:\n>> Derrick Stolee <stolee@gmail.com> writes:\n>>\n>>> I do worry about the case where annotated tags greatly outnumber\n>>> branches, so this binary search is extra overhead and the performance\n>>> may degrade. Would it be worth checking the ref to see if it lies\n>>> within \"refs/heads/\" (or even _not_ in \"refs/tags/\") before doing\n>>> this commit-graph check?\n>>\n>> Ah, clever.\n> \n> Good idea. Benchmarks for my test repository (which definitely isn't\n> representative, but it's at least some numbers) show that restricting to\n> \"refs/heads/\" diminishes almost all the gains, while restricting to\n> everything but \"refs/tags/\" performs almost the same (it's a tiny bit\n> slower, probably because of the added string comparisons):\n> \n>     Benchmark #1: all refs: git-fetch\n>       Time (mean ± σ):     32.959 s ±  0.282 s    [User: 29.801 s, System: 5.137 s]\n>       Range (min … max):   32.760 s … 33.158 s    2 runs\n> \n>     Benchmark #2: refs/heads: git-fetch\n>       Time (mean ± σ):     56.955 s ±  0.002 s    [User: 53.447 s, System: 5.362 s]\n>       Range (min … max):   56.953 s … 56.957 s    2 runs\n> \n>     Benchmark #3: !refs/tags: git-fetch\n>       Time (mean ± σ):     33.447 s ±  0.003 s    [User: 30.160 s, System: 5.027 s]\n>       Range (min … max):   33.444 s … 33.449 s    2 runs\n> \n>     Summary\n>       'all refs: git-fetch' ran\n>         1.01 ± 0.01 times faster than '!refs/tags: git-fetch'\n>         1.73 ± 0.01 times faster than 'refs/heads: git-fetch'\n\nThanks for testing both options.\n\n> This is easily explained by the fact that the test repo has most of its\n> refs neither in \"refs/tags/\" nor in \"refs/heads/\", but rather in special\n> namespaces like \"refs/merge-requests/\", \"refs/environments/\" or\n> \"refs/keep-around/\".\n\nThat makes sense to me. GitHub also stores refs like refs/pull/ so I can\nunderstand not wanting to restrict to refs/heads/.\n\n> I like the idea of excluding \"refs/tags/\" though: as you point out,\n> chances are high that these don't point to commits but to annotated tags\n> instead. So I'll go with that, thanks!\n\nYeah, that makes sense as a good way forward.\n\nThanks,\n-Stolee\n\n"},{"id":"433738","messageId":"50329e98-862b-5240-cf6c-cfe4cd7e248c@gmail.com","threadId":"56331","inReplyTo":"4a819a68309bf03db2d9a5e5be070e52c3542af8.1629800774.git.ps@pks.im","subject":"Re: [PATCH v2 1/7] fetch: speed up lookup of want refs via commit-graph","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-25T14:16:06Z","receivedAt":"2021-08-25T14:16:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2021 6:36 AM, Patrick Steinhardt wrote:\n> Speed this up by opportunistcally trying to resolve object IDs via the\n\ns/opportunistcally/opportunistically/ \n\n> +\t\t\t/*\n> +\t\t\t * References in \"refs/tags/\" are often going to point\n> +\t\t\t * to annotated tags, which are not part of the\n> +\t\t\t * commit-graph. We thus only try to look up refs in\n> +\t\t\t * the graph which are not in that namespace to not\n> +\t\t\t * regress performance in repositories with many\n> +\t\t\t * annotated tags.\n> +\t\t\t */\n> +\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n> +\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n\nThis new logic looks good.\n\n> +\t\t\tif (!commit) {\n> +\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n> +\t\t\t\t\t\t\t\t\t&rm->old_oid,\n> +\t\t\t\t\t\t\t\t\t1);\n> +\t\t\t\tif (!commit)\n> +\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n> +\t\t\t}\n\nThanks,\n-Stolee\n"},{"id":"433739","messageId":"0481bb7d-2799-740c-a3de-e16ae05b8f45@gmail.com","threadId":"56331","inReplyTo":"56a9158ac331f9911a4347d7d4afc2bbd2cf4d33.1629800774.git.ps@pks.im","subject":"Re: [PATCH v2 5/7] fetch: refactor fetch refs to be more extendable","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-25T14:19:27Z","receivedAt":"2021-08-25T14:19:31Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:\n> Refactor `fetch_refs()` code to make it more extendable by explicitly\n> handling error cases. The refactored code should behave the same.\n...\n> +\t/*\n> +\t * We don't need to perform a fetch in case we can already satisfy all\n> +\t * refs.\n> +\t */\n> +\tret = check_exist_and_connected(ref_map);\n>  \tif (ret) {\n>  \t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n>  \t\tret = transport_fetch_refs(transport, ref_map);\n>  \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n> +\t\tif (ret) {\n> +\t\t\ttransport_unlock_pack(transport);\n> +\t\t\treturn ret;\n> +\t\t}>  \t}\n\nI see that this nested organization makes it more clear what cases\nlead into this error state.\n\n> -\tif (!ret)\n> -\t\t/*\n> -\t\t * Keep the new pack's \".keep\" file around to allow the caller\n> -\t\t * time to update refs to reference the new objects.\n> -\t\t */\n> -\t\treturn 0;\n> -\ttransport_unlock_pack(transport);\n> -\treturn ret;\n> +\n> +\t/*\n> +\t * Keep the new pack's \".keep\" file around to allow the caller\n> +\t * time to update refs to reference the new objects.\n> +\t */\n> +\treturn 0;\n\nAnd it happens that 'ret' is zero here. Should we keep returning 'ret'\nor perhaps add an \"assert(!ret);\" before the return? The assert()\ndoesn't do much, but at minimum would serve as an extra indicator to\nanyone working in this method in the future.\n\nThanks,\n-Stolee\n"},{"id":"433740","messageId":"439cb382-a4d0-18ad-5475-0cb29fba98e7@gmail.com","threadId":"56331","inReplyTo":"31d9f72edf5c178b2e80c30bb7c0a9bc164ca5eb.1629800774.git.ps@pks.im","subject":"Re: [PATCH v2 6/7] fetch: merge fetching and consuming refs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-25T14:26:28Z","receivedAt":"2021-08-25T14:26:38Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:\n> -\t\tif (ret) {\n> -\t\t\ttransport_unlock_pack(transport);\n> -\t\t\treturn ret;\n> -\t\t}\n> +\t\tif (ret)\n> +\t\t\tgoto out;\n\nYou were just reorganizing this method in the previous patch.\nThis \"goto out\" trick could have applied there instead, which\nwouldn't complicate that patch and would simplify this one.\n\nBut perhaps it would look strange to have the following ending\nto the method, even if for only one patch:\n\n\treturn 0;\n\nout:\n\ttransport_unlock_pack(transport);\n\treturn res;\n}\n\nSo, feel free to ignore me here. Decide based on your taste.\n\n>  \t}\n>  \n> -\t/*\n> -\t * Keep the new pack's \".keep\" file around to allow the caller\n> -\t * time to update refs to reference the new objects.\n> -\t */\n> -\treturn 0;\n> -}\n> -\n> -/* Update local refs based on the ref values fetched from a remote */\n> -static int consume_refs(struct transport *transport, struct ref *ref_map)\n> -{\n> -\tint connectivity_checked = transport->smart_options\n> +\tconnectivity_checked = transport->smart_options\n>  \t\t? transport->smart_options->connectivity_checked : 0;\n> -\tint ret;\n> +\n>  \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n>  \tret = store_updated_refs(transport->url,\n>  \t\t\t\t transport->remote->name,\n>  \t\t\t\t connectivity_checked,\n>  \t\t\t\t ref_map);\n> -\ttransport_unlock_pack(transport);\n>  \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n\nThis transport_unlock_pack() is leaving the trace2 region. I think\nit is unlikely that the loop of unlink_or_warn() calls will take\nsignificant time that affects this region, so it should be fine to\nmove it.\n\n> +\n> +out:\n> +\ttransport_unlock_pack(transport);\n>  \treturn ret;\n>  }\n...\n> -\tif (!fetch_refs(transport, ref_map))\n> -\t\tconsume_refs(transport, ref_map);\n> +\tfetch_and_consume_refs(transport, ref_map);\n>  \n>  \tif (gsecondary) {\n>  \t\ttransport_disconnect(gsecondary);\n> @@ -1610,7 +1600,7 @@ static int do_fetch(struct transport *transport,\n>  \t\t\t\t   transport->url);\n>  \t\t}\n>  \t}\n> -\tif (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) {\n> +\tif (fetch_and_consume_refs(transport, ref_map)) {\n\nThe end goal of making these consumers simpler is worth it.\n\nThanks,\n-Stolee\n"},{"id":"433741","messageId":"fc0c8047-4d46-a349-88d3-f959c25de953@gmail.com","threadId":"56331","inReplyTo":"cover.1629800774.git.ps@pks.im","subject":"Re: [PATCH v2 0/7] Speed up mirror-fetches with many refs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-08-25T14:27:23Z","receivedAt":"2021-08-25T14:27:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/24/2021 6:36 AM, Patrick Steinhardt wrote:\n> Changes compared to v1:\n> \n>     - Patch 1/7: I've applied Stolee's proposal to only\n>       opportunistically load objects via the commit-graph in case the\n>       reference is not in refs/tags/ such that we don't regress repos\n>       with many annotated tags.\n> \n>     - Patch 3/7: The return parameter of the iterator is now const to\n>       allow further optimizations by the compiler, as suggested by\n>       René. I've also re-benchmarked this, and one can now see a very\n>       slight performance improvement of ~1%.\n> \n>     - Patch 4/7: Added my missing DCO, as pointed out by Junio.\n> \n>     - Patch 5, 6, 7: I've redone these to make it clearer that the\n>       refactoring I'm doing doesn't cause us to miss any object\n>       connectivity checks. Most importantly, I've merged `fetch_refs()`\n>       and `consume_refs()` into `fetch_and_consume_refs()` in 6/7, which\n>       makes the optimization where we elide the second connectivity\n>       check in 7/7 trivial.\n\nThese changes are positive. My read through this set of patches\nhad only a few nit-picks.\n\nThanks,\n-Stolee\n"},{"id":"433787","messageId":"878s0pccnj.fsf@evledraar.gmail.com","threadId":"56331","inReplyTo":"d3dac607f2235c5913621813c443aa10b99c8fe8.1629452412.git.ps@pks.im","subject":"Re: [PATCH 2/6] fetch: avoid unpacking headers in object existence check","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-25T23:44:03Z","receivedAt":"2021-08-25T23:44:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Aug 20 2021, Patrick Steinhardt wrote:\n\n> [[PGP Signed Part:Undecided]]\n> When updating local refs after the fetch has transferred all objects, we\n> do an object existence test as a safety guard to avoid updating a ref to\n> an object which we don't have. We do so via `oid_object_info()`: if it\n> returns an error, then we know the object does not exist.\n>\n> One side effect of `oid_object_info()` is that it parses the object's\n> type, and to do so it must unpack the object header. This is completely\n> pointless: we don't care for the type, but only want to assert that the\n> object exists.\n>\n> Refactor the code to use `repo_has_object_file()`, which both makes the\n> code's intent clearer and is also faster because it does not unpack\n> object headers. In a real-world repo with 2.3M refs, this results in a\n> small speedup when doing a mirror-fetch:\n>\n>     Benchmark #1: HEAD~: git-fetch\n>       Time (mean ± σ):     33.686 s ±  0.176 s    [User: 30.119 s, System: 5.262 s]\n>       Range (min … max):   33.512 s … 33.944 s    5 runs\n>\n>     Benchmark #2: HEAD: git-fetch\n>       Time (mean ± σ):     31.247 s ±  0.195 s    [User: 28.135 s, System: 5.066 s]\n>       Range (min … max):   30.948 s … 31.472 s    5 runs\n>\n>     Summary\n>       'HEAD: git-fetch' ran\n>         1.08 ± 0.01 times faster than 'HEAD~: git-fetch'\n>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/fetch.c | 4 +---\n>  1 file changed, 1 insertion(+), 3 deletions(-)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 73f5b286d5..5fd0f7c791 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -846,13 +846,11 @@ static int update_local_ref(struct ref *ref,\n>  \t\t\t    int summary_width)\n>  {\n>  \tstruct commit *current = NULL, *updated;\n> -\tenum object_type type;\n>  \tstruct branch *current_branch = branch_get(NULL);\n>  \tconst char *pretty_ref = prettify_refname(ref->name);\n>  \tint fast_forward = 0;\n>  \n> -\ttype = oid_object_info(the_repository, &ref->new_oid, NULL);\n> -\tif (type < 0)\n> +\tif (!repo_has_object_file(the_repository, &ref->new_oid))\n>  \t\tdie(_(\"object %s not found\"), oid_to_hex(&ref->new_oid));\n>  \n>  \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n\nI tried grepping the source for any other candidates for a migration to\nrepo_has_object_file(), but this is the only \"type = oid_object_info\" I\ncould find that didn't care about the type, perhaps there's some callers\nof *_extended() that could be moved over, but that's less likely, and I\ndidn't check...\n"},{"id":"434428","messageId":"YS92mY5WMIEuXvG/@ncase","threadId":"56331","inReplyTo":"0481bb7d-2799-740c-a3de-e16ae05b8f45@gmail.com","subject":"Re: [PATCH v2 5/7] fetch: refactor fetch refs to be more extendable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T12:48:25Z","receivedAt":"2021-09-01T12:50:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 25, 2021 at 10:19:27AM -0400, Derrick Stolee wrote:\n> On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:\n> > Refactor `fetch_refs()` code to make it more extendable by explicitly\n> > handling error cases. The refactored code should behave the same.\n[snip]\n> > -\tif (!ret)\n> > -\t\t/*\n> > -\t\t * Keep the new pack's \".keep\" file around to allow the caller\n> > -\t\t * time to update refs to reference the new objects.\n> > -\t\t */\n> > -\t\treturn 0;\n> > -\ttransport_unlock_pack(transport);\n> > -\treturn ret;\n> > +\n> > +\t/*\n> > +\t * Keep the new pack's \".keep\" file around to allow the caller\n> > +\t * time to update refs to reference the new objects.\n> > +\t */\n> > +\treturn 0;\n> \n> And it happens that 'ret' is zero here. Should we keep returning 'ret'\n> or perhaps add an \"assert(!ret);\" before the return? The assert()\n> doesn't do much, but at minimum would serve as an extra indicator to\n> anyone working in this method in the future.\n\nThe assert isn't really needed: in the subsequent patch, we always\nunlock the packfile on exit.\n\nPatrick\n"},{"id":"434429","messageId":"YS92vF16DSjwtiVc@ncase","threadId":"56331","inReplyTo":"439cb382-a4d0-18ad-5475-0cb29fba98e7@gmail.com","subject":"Re: [PATCH v2 6/7] fetch: merge fetching and consuming refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T12:49:00Z","receivedAt":"2021-09-01T12:50:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Aug 25, 2021 at 10:26:28AM -0400, Derrick Stolee wrote:\n> On 8/24/2021 6:37 AM, Patrick Steinhardt wrote:\n> > -\t\tif (ret) {\n> > -\t\t\ttransport_unlock_pack(transport);\n> > -\t\t\treturn ret;\n> > -\t\t}\n> > +\t\tif (ret)\n> > +\t\t\tgoto out;\n> \n> You were just reorganizing this method in the previous patch.\n> This \"goto out\" trick could have applied there instead, which\n> wouldn't complicate that patch and would simplify this one.\n> \n> But perhaps it would look strange to have the following ending\n> to the method, even if for only one patch:\n> \n> \treturn 0;\n> \n> out:\n> \ttransport_unlock_pack(transport);\n> \treturn res;\n> }\n> \n> So, feel free to ignore me here. Decide based on your taste.\n\nI think you've got a point, I'll change this.\n\nPatrick\n"},{"id":"434433","messageId":"cover.1630501732.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1629452412.git.ps@pks.im","subject":"[PATCH v3 0/7] Speed up mirror-fetches with many refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T13:09:36Z","receivedAt":"2021-09-01T13:09:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the third version of my patch series to speed up mirror-fetches\nwith many refs. This patch series applies on top of master with\nps/connectivity-optim merged into it.\n\nThere's only some smallish changes based on Stolee's feedback (thanks\nfor that!):\n\n    - A small typo in 1/7.\n\n    - A confict fix in 4/7 required now because it's based on master\n      instead of directly on my merged topic.\n\n    - I've adjusted patch 5/7 such that I don't have to re-touch the\n      logic in 6/7.\n\nPatrick\n\nPatrick Steinhardt (7):\n  fetch: speed up lookup of want refs via commit-graph\n  fetch: avoid unpacking headers in object existence check\n  connected: refactor iterator to return next object ID directly\n  fetch-pack: optimize loading of refs via commit graph\n  fetch: refactor fetch refs to be more extendable\n  fetch: merge fetching and consuming refs\n  fetch: avoid second connectivity check if we already have all objects\n\n builtin/clone.c        |  8 ++---\n builtin/fetch.c        | 74 +++++++++++++++++++++++-------------------\n builtin/receive-pack.c | 17 ++++------\n connected.c            | 15 +++++----\n connected.h            |  2 +-\n fetch-pack.c           | 12 ++++---\n 6 files changed, 67 insertions(+), 61 deletions(-)\n\nRange-diff against v2:\n1:  4a819a6830 ! 1:  8214f04971 fetch: speed up lookup of want refs via commit-graph\n    @@ Commit message\n         that we repeatedly need to unpack object headers for each of the\n         referenced objects.\n     \n    -    Speed this up by opportunistcally trying to resolve object IDs via the\n    +    Speed this up by opportunistically trying to resolve object IDs via the\n         commit graph. We only do so for any refs which are not in \"refs/tags\":\n         more likely than not, these are going to be a commit anyway, and this\n         lets us avoid having to unpack object headers completely in case the\n2:  81ebadabe8 = 2:  991a27cb82 fetch: avoid unpacking headers in object existence check\n3:  98e981ced9 = 3:  ba834803ab connected: refactor iterator to return next object ID directly\n4:  6311203f08 ! 4:  99d3316d48 fetch-pack: optimize loading of refs via commit graph\n    @@ fetch-pack.c: static struct commit *deref_without_lazy_fetch(const struct object\n      \n      \twhile (1) {\n      \t\tif (oid_object_info_extended(the_repository, oid, &info,\n    -@@ fetch-pack.c: static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n    - \t}\n    - \n    - \tif (type == OBJ_COMMIT) {\n    --\t\tstruct commit *commit = lookup_commit(the_repository, oid);\n    -+\t\tcommit = lookup_commit(the_repository, oid);\n    - \t\tif (!commit || repo_parse_commit(the_repository, commit))\n    - \t\t\treturn NULL;\n    - \t\treturn commit;\n5:  56a9158ac3 ! 5:  d64888e072 fetch: refactor fetch refs to be more extendable\n    @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n      \t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n      \t\tret = transport_fetch_refs(transport, ref_map);\n      \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n    -+\t\tif (ret) {\n    -+\t\t\ttransport_unlock_pack(transport);\n    -+\t\t\treturn ret;\n    -+\t\t}\n    ++\t\tif (ret)\n    ++\t\t\tgoto out;\n      \t}\n     -\tif (!ret)\n     -\t\t/*\n    @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n     -\t\t * time to update refs to reference the new objects.\n     -\t\t */\n     -\t\treturn 0;\n    --\ttransport_unlock_pack(transport);\n    --\treturn ret;\n     +\n     +\t/*\n     +\t * Keep the new pack's \".keep\" file around to allow the caller\n     +\t * time to update refs to reference the new objects.\n     +\t */\n    -+\treturn 0;\n    ++\treturn ret;\n    ++\n    ++out:\n    + \ttransport_unlock_pack(transport);\n    + \treturn ret;\n      }\n    - \n    - /* Update local refs based on the ref values fetched from a remote */\n6:  31d9f72edf ! 6:  56ecbfc9c3 fetch: merge fetching and consuming refs\n    @@ builtin/fetch.c: static int check_exist_and_connected(struct ref *ref_map)\n      \n      \t/*\n     @@ builtin/fetch.c: static int fetch_refs(struct transport *transport, struct ref *ref_map)\n    - \t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n    - \t\tret = transport_fetch_refs(transport, ref_map);\n    - \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n    --\t\tif (ret) {\n    --\t\t\ttransport_unlock_pack(transport);\n    --\t\t\treturn ret;\n    --\t\t}\n    -+\t\tif (ret)\n    -+\t\t\tgoto out;\n    + \t\t\tgoto out;\n      \t}\n      \n     -\t/*\n     -\t * Keep the new pack's \".keep\" file around to allow the caller\n     -\t * time to update refs to reference the new objects.\n     -\t */\n    --\treturn 0;\n    +-\treturn ret;\n    +-\n    +-out:\n    +-\ttransport_unlock_pack(transport);\n    +-\treturn ret;\n     -}\n     -\n     -/* Update local refs based on the ref values fetched from a remote */\n7:  84e39c847f = 7:  c342fc0c69 fetch: avoid second connectivity check if we already have all objects\n-- \n2.33.0\n\n"},{"id":"434434","messageId":"8214f04971c4ea628f23d2e9ea143d87570c29d2.1630501732.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1630501732.git.ps@pks.im","subject":"[PATCH v3 1/7] fetch: speed up lookup of want refs via commit-graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T13:09:41Z","receivedAt":"2021-09-01T13:09:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When updating our local refs based on the refs fetched from the remote,\nwe need to iterate through all requested refs and load their respective\ncommits such that we can determine whether they need to be appended to\nFETCH_HEAD or not. In cases where we're fetching from a remote with\nexceedingly many refs, resolving these refs can be quite expensive given\nthat we repeatedly need to unpack object headers for each of the\nreferenced objects.\n\nSpeed this up by opportunistically trying to resolve object IDs via the\ncommit graph. We only do so for any refs which are not in \"refs/tags\":\nmore likely than not, these are going to be a commit anyway, and this\nlets us avoid having to unpack object headers completely in case the\nobject is a commit that is part of the commit-graph. This significantly\nspeeds up mirror-fetches in a real-world repository with\n2.3M refs:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     56.482 s ±  0.384 s    [User: 53.340 s, System: 5.365 s]\n      Range (min … max):   56.050 s … 57.045 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     33.727 s ±  0.170 s    [User: 30.252 s, System: 5.194 s]\n      Range (min … max):   33.452 s … 33.871 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.67 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 24 ++++++++++++++++++------\n 1 file changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 25740c13df..bd7c0da232 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1074,7 +1074,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t      int connectivity_checked, struct ref *ref_map)\n {\n \tstruct fetch_head fetch_head;\n-\tstruct commit *commit;\n \tint url_len, i, rc = 0;\n \tstruct strbuf note = STRBUF_INIT, err = STRBUF_INIT;\n \tstruct ref_transaction *transaction = NULL;\n@@ -1122,6 +1121,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t     want_status <= FETCH_HEAD_IGNORE;\n \t     want_status++) {\n \t\tfor (rm = ref_map; rm; rm = rm->next) {\n+\t\t\tstruct commit *commit = NULL;\n \t\t\tstruct ref *ref = NULL;\n \n \t\t\tif (rm->status == REF_STATUS_REJECT_SHALLOW) {\n@@ -1131,11 +1131,23 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t\tcontinue;\n \t\t\t}\n \n-\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n-\t\t\t\t\t\t\t\t&rm->old_oid,\n-\t\t\t\t\t\t\t\t1);\n-\t\t\tif (!commit)\n-\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t\t\t/*\n+\t\t\t * References in \"refs/tags/\" are often going to point\n+\t\t\t * to annotated tags, which are not part of the\n+\t\t\t * commit-graph. We thus only try to look up refs in\n+\t\t\t * the graph which are not in that namespace to not\n+\t\t\t * regress performance in repositories with many\n+\t\t\t * annotated tags.\n+\t\t\t */\n+\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n+\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n+\t\t\tif (!commit) {\n+\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\t\t\t&rm->old_oid,\n+\t\t\t\t\t\t\t\t\t1);\n+\t\t\t\tif (!commit)\n+\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t\t\t}\n \n \t\t\tif (rm->fetch_head_status != want_status)\n \t\t\t\tcontinue;\n-- \n2.33.0\n\n"},{"id":"434435","messageId":"991a27cb8286195e8ad928807b96ed17ebe1e5a5.1630501732.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1630501732.git.ps@pks.im","subject":"[PATCH v3 2/7] fetch: avoid unpacking headers in object existence check","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T13:09:45Z","receivedAt":"2021-09-01T13:09:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When updating local refs after the fetch has transferred all objects, we\ndo an object existence test as a safety guard to avoid updating a ref to\nan object which we don't have. We do so via `oid_object_info()`: if it\nreturns an error, then we know the object does not exist.\n\nOne side effect of `oid_object_info()` is that it parses the object's\ntype, and to do so it must unpack the object header. This is completely\npointless: we don't care for the type, but only want to assert that the\nobject exists.\n\nRefactor the code to use `repo_has_object_file()`, which both makes the\ncode's intent clearer and is also faster because it does not unpack\nobject headers. In a real-world repo with 2.3M refs, this results in a\nsmall speedup when doing a mirror-fetch:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     33.686 s ±  0.176 s    [User: 30.119 s, System: 5.262 s]\n      Range (min … max):   33.512 s … 33.944 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     31.247 s ±  0.195 s    [User: 28.135 s, System: 5.066 s]\n      Range (min … max):   30.948 s … 31.472 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.08 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex bd7c0da232..0b18c47732 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -846,13 +846,11 @@ static int update_local_ref(struct ref *ref,\n \t\t\t    int summary_width)\n {\n \tstruct commit *current = NULL, *updated;\n-\tenum object_type type;\n \tstruct branch *current_branch = branch_get(NULL);\n \tconst char *pretty_ref = prettify_refname(ref->name);\n \tint fast_forward = 0;\n \n-\ttype = oid_object_info(the_repository, &ref->new_oid, NULL);\n-\tif (type < 0)\n+\tif (!repo_has_object_file(the_repository, &ref->new_oid))\n \t\tdie(_(\"object %s not found\"), oid_to_hex(&ref->new_oid));\n \n \tif (oideq(&ref->old_oid, &ref->new_oid)) {\n-- \n2.33.0\n\n"},{"id":"434436","messageId":"ba834803ab30e3cc0be9ed71da54a6f2da2e6949.1630501732.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1630501732.git.ps@pks.im","subject":"[PATCH v3 3/7] connected: refactor iterator to return next object ID directly","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T13:09:50Z","receivedAt":"2021-09-01T13:09:56Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The object ID iterator used by the connectivity checks returns the next\nobject ID via an out-parameter and then uses a return code to indicate\nwhether an item was found. This is a bit roundabout: instead of a\nseparate error code, we can just return the next object ID directly and\nuse `NULL` pointers as indicator that the iterator got no items left.\nFurthermore, this avoids a copy of the object ID.\n\nRefactor the iterator and all its implementations to return object IDs\ndirectly. This brings a tiny performance improvement when doing a mirror-fetch of a repository with about 2.3M refs:\n\n    Benchmark #1: 328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch\n      Time (mean ± σ):     30.110 s ±  0.148 s    [User: 27.161 s, System: 5.075 s]\n      Range (min … max):   29.934 s … 30.406 s    10 runs\n\n    Benchmark #2: 328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch\n      Time (mean ± σ):     29.899 s ±  0.109 s    [User: 26.916 s, System: 5.104 s]\n      Range (min … max):   29.696 s … 29.996 s    10 runs\n\n    Summary\n      '328dc58b49919c43897240f2eabfa30be2ce32a4: git-fetch' ran\n        1.01 ± 0.01 times faster than '328dc58b49919c43897240f2eabfa30be2ce32a4~: git-fetch'\n\nWhile this 1% speedup could be labelled as statistically insignificant,\nthe speedup is consistent on my machine. Furthermore, this is an end to\nend test, so it is expected that the improvement in the connectivity\ncheck itself is more significant.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/clone.c        |  8 +++-----\n builtin/fetch.c        |  7 +++----\n builtin/receive-pack.c | 17 +++++++----------\n connected.c            | 15 ++++++++-------\n connected.h            |  2 +-\n fetch-pack.c           |  7 +++----\n 6 files changed, 25 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 66fe66679c..4a1056fcc2 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -657,7 +657,7 @@ static void write_followtags(const struct ref *refs, const char *msg)\n \t}\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static const struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n@@ -668,13 +668,11 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \t */\n \twhile (ref && !ref->peer_ref)\n \t\tref = ref->next;\n-\t/* Returning -1 notes \"end of list\" to the caller. */\n \tif (!ref)\n-\t\treturn -1;\n+\t\treturn NULL;\n \n-\toidcpy(oid, &ref->old_oid);\n \t*rm = ref->next;\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n static void update_remote_refs(const struct ref *refs,\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 0b18c47732..f67fe543ad 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -962,7 +962,7 @@ static int update_local_ref(struct ref *ref,\n \t}\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static const struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n@@ -970,10 +970,9 @@ static int iterate_ref_map(void *cb_data, struct object_id *oid)\n \twhile (ref && ref->status == REF_STATUS_REJECT_SHALLOW)\n \t\tref = ref->next;\n \tif (!ref)\n-\t\treturn -1; /* end of the list */\n+\t\treturn NULL;\n \t*rm = ref->next;\n-\toidcpy(oid, &ref->old_oid);\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n struct fetch_head {\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 2d1f97e1ca..041e915454 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1306,7 +1306,7 @@ static void refuse_unconfigured_deny_delete_current(void)\n \trp_error(\"%s\", _(refuse_unconfigured_deny_delete_current_msg));\n }\n \n-static int command_singleton_iterator(void *cb_data, struct object_id *oid);\n+static const struct object_id *command_singleton_iterator(void *cb_data);\n static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n {\n \tstruct shallow_lock shallow_lock = SHALLOW_LOCK_INIT;\n@@ -1731,16 +1731,15 @@ static void check_aliased_updates(struct command *commands)\n \tstring_list_clear(&ref_list, 0);\n }\n \n-static int command_singleton_iterator(void *cb_data, struct object_id *oid)\n+static const struct object_id *command_singleton_iterator(void *cb_data)\n {\n \tstruct command **cmd_list = cb_data;\n \tstruct command *cmd = *cmd_list;\n \n \tif (!cmd || is_null_oid(&cmd->new_oid))\n-\t\treturn -1; /* end of list */\n+\t\treturn NULL;\n \t*cmd_list = NULL; /* this returns only one */\n-\toidcpy(oid, &cmd->new_oid);\n-\treturn 0;\n+\treturn &cmd->new_oid;\n }\n \n static void set_connectivity_errors(struct command *commands,\n@@ -1770,7 +1769,7 @@ struct iterate_data {\n \tstruct shallow_info *si;\n };\n \n-static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n+static const struct object_id *iterate_receive_command_list(void *cb_data)\n {\n \tstruct iterate_data *data = cb_data;\n \tstruct command **cmd_list = &data->cmds;\n@@ -1781,13 +1780,11 @@ static int iterate_receive_command_list(void *cb_data, struct object_id *oid)\n \t\t\t/* to be checked in update_shallow_ref() */\n \t\t\tcontinue;\n \t\tif (!is_null_oid(&cmd->new_oid) && !cmd->skip_update) {\n-\t\t\toidcpy(oid, &cmd->new_oid);\n \t\t\t*cmd_list = cmd->next;\n-\t\t\treturn 0;\n+\t\t\treturn &cmd->new_oid;\n \t\t}\n \t}\n-\t*cmd_list = NULL;\n-\treturn -1; /* end of list */\n+\treturn NULL;\n }\n \n static void reject_updates_to_hidden(struct command *commands)\ndiff --git a/connected.c b/connected.c\nindex b5f9523a5f..cf68e37a97 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -24,7 +24,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \tstruct child_process rev_list = CHILD_PROCESS_INIT;\n \tFILE *rev_list_in;\n \tstruct check_connected_options defaults = CHECK_CONNECTED_INIT;\n-\tstruct object_id oid;\n+\tconst struct object_id *oid;\n \tint err = 0;\n \tstruct packed_git *new_pack = NULL;\n \tstruct transport *transport;\n@@ -34,7 +34,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\topt = &defaults;\n \ttransport = opt->transport;\n \n-\tif (fn(cb_data, &oid)) {\n+\toid = fn(cb_data);\n+\tif (!oid) {\n \t\tif (opt->err_fd)\n \t\t\tclose(opt->err_fd);\n \t\treturn err;\n@@ -73,7 +74,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t\tfor (p = get_all_packs(the_repository); p; p = p->next) {\n \t\t\t\tif (!p->pack_promisor)\n \t\t\t\t\tcontinue;\n-\t\t\t\tif (find_pack_entry_one(oid.hash, p))\n+\t\t\t\tif (find_pack_entry_one(oid->hash, p))\n \t\t\t\t\tgoto promisor_pack_found;\n \t\t\t}\n \t\t\t/*\n@@ -83,7 +84,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t\tgoto no_promisor_pack_found;\n promisor_pack_found:\n \t\t\t;\n-\t\t} while (!fn(cb_data, &oid));\n+\t\t} while ((oid = fn(cb_data)) != NULL);\n \t\treturn 0;\n \t}\n \n@@ -133,12 +134,12 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\t * are sure the ref is good and not sending it to\n \t\t * rev-list for verification.\n \t\t */\n-\t\tif (new_pack && find_pack_entry_one(oid.hash, new_pack))\n+\t\tif (new_pack && find_pack_entry_one(oid->hash, new_pack))\n \t\t\tcontinue;\n \n-\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(&oid)) < 0)\n+\t\tif (fprintf(rev_list_in, \"%s\\n\", oid_to_hex(oid)) < 0)\n \t\t\tbreak;\n-\t} while (!fn(cb_data, &oid));\n+\t} while ((oid = fn(cb_data)) != NULL);\n \n \tif (ferror(rev_list_in) || fflush(rev_list_in)) {\n \t\tif (errno != EPIPE && errno != EINVAL)\ndiff --git a/connected.h b/connected.h\nindex 8d5a6b3ad6..6e59c92aa3 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -9,7 +9,7 @@ struct transport;\n  * When called after returning the name for the last object, return -1\n  * to signal EOF, otherwise return 0.\n  */\n-typedef int (*oid_iterate_fn)(void *, struct object_id *oid);\n+typedef const struct object_id *(*oid_iterate_fn)(void *);\n \n /*\n  * Named-arguments struct for check_connected. All arguments are\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b0c7be717c..7b0e69884d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1906,16 +1906,15 @@ static void update_shallow(struct fetch_pack_args *args,\n \toid_array_clear(&ref);\n }\n \n-static int iterate_ref_map(void *cb_data, struct object_id *oid)\n+static const struct object_id *iterate_ref_map(void *cb_data)\n {\n \tstruct ref **rm = cb_data;\n \tstruct ref *ref = *rm;\n \n \tif (!ref)\n-\t\treturn -1; /* end of the list */\n+\t\treturn NULL;\n \t*rm = ref->next;\n-\toidcpy(oid, &ref->old_oid);\n-\treturn 0;\n+\treturn &ref->old_oid;\n }\n \n struct ref *fetch_pack(struct fetch_pack_args *args,\n-- \n2.33.0\n\n"},{"id":"434437","messageId":"99d3316d48162a318b1ac0473eb9c837c60e3833.1630501732.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1630501732.git.ps@pks.im","subject":"[PATCH v3 4/7] fetch-pack: optimize loading of refs via commit graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T13:09:54Z","receivedAt":"2021-09-01T13:09:59Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In order to negotiate a packfile, we need to dereference refs to see\nwhich commits we have in common with the remote. To do so, we first look\nup the object's type -- if it's a tag, we peel until we hit a non-tag\nobject. If we hit a commit eventually, then we return that commit.\n\nIn case the object ID points to a commit directly, we can avoid the\ninitial lookup of the object type by opportunistically looking up the\ncommit via the commit-graph, if available, which gives us a slight speed\nbump of about 2% in a huge repository with about 2.3M refs:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     31.634 s ±  0.258 s    [User: 28.400 s, System: 5.090 s]\n      Range (min … max):   31.280 s … 31.896 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     31.129 s ±  0.543 s    [User: 27.976 s, System: 5.056 s]\n      Range (min … max):   30.172 s … 31.479 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.02 ± 0.02 times faster than 'HEAD~: git-fetch'\n\nIn case this fails, we fall back to the old code which peels the\nobjects to a commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n fetch-pack.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 7b0e69884d..da92a5e474 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -119,6 +119,11 @@ static struct commit *deref_without_lazy_fetch(const struct object_id *oid,\n {\n \tenum object_type type;\n \tstruct object_info info = { .typep = &type };\n+\tstruct commit *commit;\n+\n+\tcommit = lookup_commit_in_graph(the_repository, oid);\n+\tif (commit)\n+\t\treturn commit;\n \n \twhile (1) {\n \t\tif (oid_object_info_extended(the_repository, oid, &info,\n-- \n2.33.0\n\n"},{"id":"434438","messageId":"d64888e072c3be2d25372b9743cf1bd3baefc9f4.1630501732.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1630501732.git.ps@pks.im","subject":"[PATCH v3 5/7] fetch: refactor fetch refs to be more extendable","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T13:09:58Z","receivedAt":"2021-09-01T13:10:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Refactor `fetch_refs()` code to make it more extendable by explicitly\nhandling error cases. The refactored code should behave the same.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 24 +++++++++++++++++-------\n 1 file changed, 17 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f67fe543ad..bafeb3d44b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1293,18 +1293,28 @@ static int check_exist_and_connected(struct ref *ref_map)\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n {\n-\tint ret = check_exist_and_connected(ref_map);\n+\tint ret;\n+\n+\t/*\n+\t * We don't need to perform a fetch in case we can already satisfy all\n+\t * refs.\n+\t */\n+\tret = check_exist_and_connected(ref_map);\n \tif (ret) {\n \t\ttrace2_region_enter(\"fetch\", \"fetch_refs\", the_repository);\n \t\tret = transport_fetch_refs(transport, ref_map);\n \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n+\t\tif (ret)\n+\t\t\tgoto out;\n \t}\n-\tif (!ret)\n-\t\t/*\n-\t\t * Keep the new pack's \".keep\" file around to allow the caller\n-\t\t * time to update refs to reference the new objects.\n-\t\t */\n-\t\treturn 0;\n+\n+\t/*\n+\t * Keep the new pack's \".keep\" file around to allow the caller\n+\t * time to update refs to reference the new objects.\n+\t */\n+\treturn ret;\n+\n+out:\n \ttransport_unlock_pack(transport);\n \treturn ret;\n }\n-- \n2.33.0\n\n"},{"id":"434439","messageId":"56ecbfc9c3ac03ae751f83eed7ff0b45966f9297.1630501732.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1630501732.git.ps@pks.im","subject":"[PATCH v3 6/7] fetch: merge fetching and consuming refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T13:10:02Z","receivedAt":"2021-09-01T13:10:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The functions `fetch_refs()` and `consume_refs()` must always be called\ntogether such that we first obtain all missing objects and then update\nour local refs to match the remote refs. In a subsequent patch, we'll\nfurther require that `fetch_refs()` must always be called before\n`consume_refs()` such that it can correctly assert that we have all\nobjects after the fetch given that we're about to move the connectivity\ncheck.\n\nMake this requirement explicit by merging both functions into a single\n`fetch_and_consume_refs()` function.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 30 +++++++++---------------------\n 1 file changed, 9 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex bafeb3d44b..09f7742d0f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1291,8 +1291,9 @@ static int check_exist_and_connected(struct ref *ref_map)\n \treturn check_connected(iterate_ref_map, &rm, &opt);\n }\n \n-static int fetch_refs(struct transport *transport, struct ref *ref_map)\n+static int fetch_and_consume_refs(struct transport *transport, struct ref *ref_map)\n {\n+\tint connectivity_checked;\n \tint ret;\n \n \t/*\n@@ -1308,30 +1309,18 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n \t\t\tgoto out;\n \t}\n \n-\t/*\n-\t * Keep the new pack's \".keep\" file around to allow the caller\n-\t * time to update refs to reference the new objects.\n-\t */\n-\treturn ret;\n-\n-out:\n-\ttransport_unlock_pack(transport);\n-\treturn ret;\n-}\n-\n-/* Update local refs based on the ref values fetched from a remote */\n-static int consume_refs(struct transport *transport, struct ref *ref_map)\n-{\n-\tint connectivity_checked = transport->smart_options\n+\tconnectivity_checked = transport->smart_options\n \t\t? transport->smart_options->connectivity_checked : 0;\n-\tint ret;\n+\n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(transport->url,\n \t\t\t\t transport->remote->name,\n \t\t\t\t connectivity_checked,\n \t\t\t\t ref_map);\n-\ttransport_unlock_pack(transport);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n+\n+out:\n+\ttransport_unlock_pack(transport);\n \treturn ret;\n }\n \n@@ -1518,8 +1507,7 @@ static void backfill_tags(struct transport *transport, struct ref *ref_map)\n \ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n \ttransport_set_option(transport, TRANS_OPT_DEEPEN_RELATIVE, NULL);\n-\tif (!fetch_refs(transport, ref_map))\n-\t\tconsume_refs(transport, ref_map);\n+\tfetch_and_consume_refs(transport, ref_map);\n \n \tif (gsecondary) {\n \t\ttransport_disconnect(gsecondary);\n@@ -1610,7 +1598,7 @@ static int do_fetch(struct transport *transport,\n \t\t\t\t   transport->url);\n \t\t}\n \t}\n-\tif (fetch_refs(transport, ref_map) || consume_refs(transport, ref_map)) {\n+\tif (fetch_and_consume_refs(transport, ref_map)) {\n \t\tfree_refs(ref_map);\n \t\tretcode = 1;\n \t\tgoto cleanup;\n-- \n2.33.0\n\n"},{"id":"434440","messageId":"c342fc0c69226d4be8692f2311544a440c67a76d.1630501732.git.ps@pks.im","threadId":"56331","inReplyTo":"cover.1630501732.git.ps@pks.im","subject":"[PATCH v3 7/7] fetch: avoid second connectivity check if we already have all objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-09-01T13:10:06Z","receivedAt":"2021-09-01T13:10:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching refs, we are doing two connectivity checks:\n\n    - The first one is done such that we can skip fetching refs in the\n      case where we already have all objects referenced by the updated\n      set of refs.\n\n    - The second one verifies that we have all objects after we have\n      fetched objects.\n\nWe always execute both connectivity checks, but this is wasteful in case\nthe first connectivity check already notices that we have all objects\nlocally available.\n\nSkip the second connectivity check in case we already had all objects\navailable. This gives us a nice speedup when doing a mirror-fetch in a\nrepository with about 2.3M refs where the fetching repo already has all\nobjects:\n\n    Benchmark #1: HEAD~: git-fetch\n      Time (mean ± σ):     30.025 s ±  0.081 s    [User: 27.070 s, System: 4.933 s]\n      Range (min … max):   29.900 s … 30.111 s    5 runs\n\n    Benchmark #2: HEAD: git-fetch\n      Time (mean ± σ):     25.574 s ±  0.177 s    [User: 22.855 s, System: 4.683 s]\n      Range (min … max):   25.399 s … 25.765 s    5 runs\n\n    Summary\n      'HEAD: git-fetch' ran\n        1.17 ± 0.01 times faster than 'HEAD~: git-fetch'\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 09f7742d0f..6fdd01fb01 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1293,7 +1293,7 @@ static int check_exist_and_connected(struct ref *ref_map)\n \n static int fetch_and_consume_refs(struct transport *transport, struct ref *ref_map)\n {\n-\tint connectivity_checked;\n+\tint connectivity_checked = 1;\n \tint ret;\n \n \t/*\n@@ -1307,11 +1307,10 @@ static int fetch_and_consume_refs(struct transport *transport, struct ref *ref_m\n \t\ttrace2_region_leave(\"fetch\", \"fetch_refs\", the_repository);\n \t\tif (ret)\n \t\t\tgoto out;\n+\t\tconnectivity_checked = transport->smart_options ?\n+\t\t\ttransport->smart_options->connectivity_checked : 0;\n \t}\n \n-\tconnectivity_checked = transport->smart_options\n-\t\t? transport->smart_options->connectivity_checked : 0;\n-\n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n \tret = store_updated_refs(transport->url,\n \t\t\t\t transport->remote->name,\n-- \n2.33.0\n\n"},{"id":"434466","messageId":"xmqqwno0oyp0.fsf@gitster.g","threadId":"56331","inReplyTo":"cover.1630501732.git.ps@pks.im","subject":"Re: [PATCH v3 0/7] Speed up mirror-fetches with many refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T19:58:19Z","receivedAt":"2021-09-01T19:58:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> There's only some smallish changes based on Stolee's feedback (thanks\n> for that!):\n>\n>     - A small typo in 1/7.\n>\n>     - A confict fix in 4/7 required now because it's based on master\n>       instead of directly on my merged topic.\n>\n>     - I've adjusted patch 5/7 such that I don't have to re-touch the\n>       logic in 6/7.\n\nOK, as a sanity check on my end, I've tried to apply this on master\nand then to merge the result with 'ps/connectivity-optim' topic\n(i.e. expected endgame for this and the other topic if both of them\ngraduated today without any other topics), and then tried a merge on\nmaster to the previous round (i.e. has the other topic already in),\nand did not see any difference in the end result.\n\nWill replace.\n"},{"id":"434969","messageId":"xmqqo8947wth.fsf@gitster.g","threadId":"56331","inReplyTo":"xmqqwno0oyp0.fsf@gitster.g","subject":"Re: [PATCH v3 0/7] Speed up mirror-fetches with many refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-08T00:08:58Z","receivedAt":"2021-09-08T00:09:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Patrick Steinhardt <ps@pks.im> writes:\n>\n>> There's only some smallish changes based on Stolee's feedback (thanks\n>> for that!):\n>>\n>>     - A small typo in 1/7.\n>>\n>>     - A confict fix in 4/7 required now because it's based on master\n>>       instead of directly on my merged topic.\n>>\n>>     - I've adjusted patch 5/7 such that I don't have to re-touch the\n>>       logic in 6/7.\n>\n> OK, as a sanity check on my end, I've tried to apply this on master\n> and then to merge the result with 'ps/connectivity-optim' topic\n> (i.e. expected endgame for this and the other topic if both of them\n> graduated today without any other topics), and then tried a merge on\n> master to the previous round (i.e. has the other topic already in),\n> and did not see any difference in the end result.\n>\n> Will replace.\n\nOK.  Is everybody happy with this version?  I am about to mark it\nfor 'next' in the next issue of \"What's cooking\" report, so please\nholler if I should wait.\n"}]}