{"thread":{"id":"57461","subject":"[PATCH 2/5] fetch: avoid lookup of commits when not appending to FETCH_HEAD","startedAt":"2022-02-23T12:35:39Z","lastAt":"2022-03-02T18:54:40Z","messageCount":20,"participants":["Patrick Steinhardt","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"449241","messageId":"80f993dddd521133154a751aeaab86adee409eea.1645619224.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1645619224.git.ps@pks.im","subject":"[PATCH 2/5] fetch: avoid lookup of commits when not appending to FETCH_HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-23T12:35:31Z","receivedAt":"2022-02-23T12:35:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching from a remote repository we will by default write what has\nbeen fetched into the special FETCH_HEAD reference. The order in which\nreferences are written depends on whether the reference is for merge or\nnot, which, despite some other conditions, is also determined based on\nwhether the old object ID the reference is being updated from actually\nexists in the repository.\n\nTo write FETCH_HEAD we thus loop through all references thrice: once for\nthe references that are about to be merged, once for the references that\nare not for merge, and finally for all references that are ignored. For\nevery iteration, we then look up the old object ID to determine whether\nthe referenced object exists so that we can label it as \"not-for-merge\"\nif it doesn't exist. It goes without saying that this can be expensive\nin case where we are fetching a lot of references.\n\nWhile this is hard to avoid in the case where we're writing FETCH_HEAD,\nusers can in fact ask us to skip this work via `--no-write-fetch-head`.\nIn that case, we do not care for the result of those lookups at all\nbecause we don't have to order writes to FETCH_HEAD in the first place.\n\nSkip this busywork in case we're not writing to FETCH_HEAD. The\nfollowing benchmark performs a mirror-fetch in a repository with about\ntwo million references:\n\n Benchmark 1: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)\n Time (mean ± σ): 75.388 s ± 1.942 s [User: 71.103 s, System: 8.953 s]\n Range (min … max): 73.184 s … 76.845 s 3 runs\n\n Benchmark 2: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)\n Time (mean ± σ): 69.486 s ± 1.016 s [User: 65.941 s, System: 8.806 s]\n Range (min … max): 68.864 s … 70.659 s 3 runs\n\n Summary\n 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)' ran\n 1.08 ± 0.03 times faster than 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)'\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 42 +++++++++++++++++++++++++++---------------\n 1 file changed, 27 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e8305b6662..4d12c2fd4d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1146,7 +1146,6 @@ 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@@ -1157,21 +1156,34 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t}\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 * When writing FETCH_HEAD we need to determine whether\n+\t\t\t * we already have the commit or not. If not, then the\n+\t\t\t * reference is not for merge and needs to be written\n+\t\t\t * to the reflog after other commits which we already\n+\t\t\t * have. We're not interested in this property though\n+\t\t\t * in case FETCH_HEAD is not to be updated, so we can\n+\t\t\t * skip the classification in that case.\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\tif (fetch_head->fp) {\n+\t\t\t\tstruct commit *commit = NULL;\n+\n+\t\t\t\t/*\n+\t\t\t\t * References in \"refs/tags/\" are often going to point\n+\t\t\t\t * to annotated tags, which are not part of the\n+\t\t\t\t * commit-graph. We thus only try to look up refs in\n+\t\t\t\t * the graph which are not in that namespace to not\n+\t\t\t\t * regress performance in repositories with many\n+\t\t\t\t * annotated tags.\n+\t\t\t\t */\n+\t\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n+\t\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n+\t\t\t\tif (!commit) {\n+\t\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\t\t\t\t&rm->old_oid,\n+\t\t\t\t\t\t\t\t\t\t1);\n+\t\t\t\t\tif (!commit)\n+\t\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tif (rm->fetch_head_status != want_status)\n-- \n2.35.1\n\n"},{"id":"449242","messageId":"ca5e136cca495c7d927e99f5ae8a672d93823eea.1645619224.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1645619224.git.ps@pks.im","subject":"[PATCH 1/5] upload-pack: look up \"want\" lines via commit-graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-23T12:35:27Z","receivedAt":"2022-02-23T12:35:40Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"During packfile negotiation the client will send \"want\" and \"want-ref\"\nlines to the server to tell it which objects it is interested in. The\nserver-side parses each of those and looks them up to see whether it\nactually has requested objects. This lookup is performed by calling\n`parse_object()` directly, which thus hits the object database. In the\ngeneral case though most of the objects the client requests will be\ncommits. We can thus try to look up the object via the commit-graph\nopportunistically, which is much faster than doing the same via the\nobject database.\n\nRefactor parsing of both \"want\" and \"want-ref\" lines to do so.\n\nThe following benchmark is executed in a repository with a huge number\nof references. It uses cached request from git-fetch(1) as input and\ncontains about 876,000 \"want\" lines:\n\n Benchmark 1: git-upload-pack (HEAD~)\n Time (mean ± σ): 7.113 s ± 0.028 s [User: 6.900 s, System: 0.662 s]\n Range (min … max): 7.072 s … 7.168 s 10 runs\n\n Benchmark 2: git-upload-pack (HEAD)\n Time (mean ± σ): 6.622 s ± 0.061 s [User: 6.452 s, System: 0.650 s]\n Range (min … max): 6.535 s … 6.727 s 10 runs\n\n Summary\n 'git-upload-pack (HEAD)' ran\n 1.07 ± 0.01 times faster than 'git-upload-pack (HEAD~)'\n\nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 8acc98741b..3a851b3606 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1400,13 +1400,19 @@ static int parse_want(struct packet_writer *writer, const char *line,\n \tconst char *arg;\n \tif (skip_prefix(line, \"want \", &arg)) {\n \t\tstruct object_id oid;\n+\t\tstruct commit *commit;\n \t\tstruct object *o;\n \n \t\tif (get_oid_hex(arg, &oid))\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get oid, not '%s'\", line);\n \n-\t\to = parse_object(the_repository, &oid);\n+\t\tcommit = lookup_commit_in_graph(the_repository, &oid);\n+\t\tif (commit)\n+\t\t\to = &commit->object;\n+\t\telse\n+\t\t\to = parse_object(the_repository, &oid);\n+\n \t\tif (!o) {\n \t\t\tpacket_writer_error(writer,\n \t\t\t\t\t \"upload-pack: not our ref %s\",\n@@ -1434,7 +1440,7 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n \tif (skip_prefix(line, \"want-ref \", &refname_nons)) {\n \t\tstruct object_id oid;\n \t\tstruct string_list_item *item;\n-\t\tstruct object *o;\n+\t\tstruct object *o = NULL;\n \t\tstruct strbuf refname = STRBUF_INIT;\n \n \t\tstrbuf_addf(&refname, \"%s%s\", get_git_namespace(), refname_nons);\n@@ -1448,7 +1454,15 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n \t\titem = string_list_append(wanted_refs, refname_nons);\n \t\titem->util = oiddup(&oid);\n \n-\t\to = parse_object_or_die(&oid, refname_nons);\n+\t\tif (!starts_with(refname_nons, \"refs/tags/\")) {\n+\t\t\tstruct commit *commit = lookup_commit_in_graph(the_repository, &oid);\n+\t\t\tif (commit)\n+\t\t\t\to = &commit->object;\n+\t\t}\n+\n+\t\tif (!o)\n+\t\t\to = parse_object_or_die(&oid, refname_nons);\n+\n \t\tif (!(o->flags & WANTED)) {\n \t\t\to->flags |= WANTED;\n \t\t\tadd_object_array(o, NULL, want_obj);\n-- \n2.35.1\n\n"},{"id":"449243","messageId":"cover.1645619224.git.ps@pks.im","threadId":"57461","inReplyTo":null,"subject":"[PATCH 0/5] fetch: more optimizations for mirror fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-23T12:35:23Z","receivedAt":"2022-02-23T12:35:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is another patch series with the aim to speed up mirror fetches. It\napplies on top of e6ebfd0e8c (The sixth batch, 2022-02-18) with\n3824153b23 (Merge branch 'ps/fetch-atomic' into next, 2022-02-18) merged\ninto it to fix a conflict.\n\nThe patch series is structured as follows:\n\n - Patch 1: Another conversion to use the commit-graph, this time to\n look up \"want\" and \"want-ref\" lines in git-upload-pack(1).\n\n - Patch 2: This skips useless commit lookups when we don't write\n FETCH_HEAD. These were only used to compute the correct order in\n which we append to FETCH_HEAD.\n\n - Patch 3-5: This optimizes the way we look up symbolic refs in the\n files backend. Currently we always search the packed-refs file,\n which cannot ever contain symbolic references.\n\nIn total, this patch series brings down the time it takes to replicate\none of our biggest repos in numbers of references from 75 seconds to 60\nseconds.\n\nPatrick\n\nPatrick Steinhardt (5):\n upload-pack: look up \"want\" lines via commit-graph\n fetch: avoid lookup of commits when not appending to FETCH_HEAD\n refs: add ability for backends to special-case reading of symbolic\n refs\n remote: read symbolic refs via `refs_read_symbolic_ref()`\n refs/files-backend: optimize reading of symbolic refs\n\n builtin/fetch.c | 42 +++++++++++++++++++++++++++---------------\n builtin/remote.c | 8 +++++---\n refs.c | 17 +++++++++++++++++\n refs.h | 3 +++\n refs/debug.c | 1 +\n refs/files-backend.c | 33 ++++++++++++++++++++++++++++-----\n refs/packed-backend.c | 1 +\n refs/refs-internal.h | 16 ++++++++++++++++\n remote.c | 14 +++++++-------\n upload-pack.c | 20 +++++++++++++++++---\n 10 files changed, 122 insertions(+), 33 deletions(-)\n\n-- \n2.35.1\n\n"},{"id":"449244","messageId":"28cacbdbe2e7dd38f96ea2d37aa438adb9e5a270.1645619224.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1645619224.git.ps@pks.im","subject":"[PATCH 3/5] refs: add ability for backends to special-case reading of symbolic refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-23T12:35:35Z","receivedAt":"2022-02-23T12:35:45Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Reading of symbolic and non-symbolic references is currently treated the\nsame in reference backends: we always call `refs_read_raw_ref()` and\nthen decide based on the returned flags what type it is. This has one\ndownside though: symbolic references may be treated different from\nnormal references in a backend from normal references. The packed-refs\nbackend for example doesn't even know about symbolic references, and as\na result it is pointless to even ask it for one.\n\nThere are cases where we really only care about whether a reference is\nsymbolic or not, but don't care about whether it exists at all or may be\na non-symbolic reference. But it is not possible to optimize for this\ncase right now, and as a consequence we will always first check for a\nloose reference to exist, and if it doesn't, we'll query the packed-refs\nbackend for a known-to-not-be-symbolic reference. This is inefficient\nand requires us to search all packed references even though we know to\nnot care for the result at all.\n\nIntroduce a new function `refs_read_symbolic_ref()` which allows us to\nfix this case. This function will only ever return symbolic references\nand can thus optimize for the scenario layed out above. By default, if\nthe backend doesn't provide an implementation for it, we just use the\nold code path and fall back to `read_raw_ref()`. But in case the backend\nprovides its own, more efficient implementation, we will use that one\ninstead.\n\nNote that this function is explicitly designed to not distinguish\nbetween missing references and non-symbolic references. If it did, we'd\nbe forced to always search the packed-refs backend to see whether the\nsymbolic reference the user asked for really doesn't exist, or if it\nexists as a non-symbolic reference.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 17 +++++++++++++++++\n refs.h | 3 +++\n refs/debug.c | 1 +\n refs/files-backend.c | 1 +\n refs/packed-backend.c | 1 +\n refs/refs-internal.h | 16 ++++++++++++++++\n 6 files changed, 39 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 1598fb13e2..0b79bdd7c3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1673,6 +1673,23 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t\t\t\t type, failure_errno);\n }\n \n+int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,\n+\t\t\t struct strbuf *referent)\n+{\n+\tstruct object_id oid;\n+\tint ret, failure_errno = 0;\n+\tunsigned int type = 0;\n+\n+\tif (ref_store->be->read_symbolic_ref)\n+\t\treturn ref_store->be->read_symbolic_ref(ref_store, refname, referent);\n+\n+\tret = refs_read_raw_ref(ref_store, refname, &oid, referent, &type, &failure_errno);\n+\tif (ret || !(type & REF_ISSYMREF))\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\ndiff --git a/refs.h b/refs.h\nindex 1ae12c410a..23479c7ee0 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -82,6 +82,9 @@ int read_ref_full(const char *refname, int resolve_flags,\n \t\t struct object_id *oid, int *flags);\n int read_ref(const char *refname, struct object_id *oid);\n \n+int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,\n+\t\t\t struct strbuf *referent);\n+\n /*\n * Return 0 if a reference named refname could be created without\n * conflicting with the name of an existing reference. Otherwise,\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 2b0771ca53..c590d37720 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -435,6 +435,7 @@ struct ref_storage_be refs_be_debug = {\n \n \tdebug_ref_iterator_begin,\n \tdebug_read_raw_ref,\n+\tNULL,\n \n \tdebug_reflog_iterator_begin,\n \tdebug_for_each_reflog_ent,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f59589d6cc..f3428a9f12 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3286,6 +3286,7 @@ struct ref_storage_be refs_be_files = {\n \n \tfiles_ref_iterator_begin,\n \tfiles_read_raw_ref,\n+\tNULL,\n \n \tfiles_reflog_iterator_begin,\n \tfiles_for_each_reflog_ent,\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 27dd8c3922..f56e2cc635 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1684,6 +1684,7 @@ struct ref_storage_be refs_be_packed = {\n \n \tpacked_ref_iterator_begin,\n \tpacked_read_raw_ref,\n+\tNULL,\n \n \tpacked_reflog_iterator_begin,\n \tpacked_for_each_reflog_ent,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 6e15db3ca4..001ef15835 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -649,6 +649,21 @@ typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname,\n \t\t\t struct object_id *oid, struct strbuf *referent,\n \t\t\t unsigned int *type, int *failure_errno);\n \n+/*\n+ * Read a symbolic reference from the specified reference store. This function\n+ * is optional: if not implemented by a backend, then `read_raw_ref_fn` is used\n+ * to read the symbolcic reference instead. It is intended to be implemented\n+ * only in case the backend can optimize the reading of symbolic references.\n+ *\n+ * Return 0 on success, or -1 on failure. `referent` will be set to the target\n+ * of the symbolic reference on success. This function explicitly does not\n+ * distinguish between error cases and the reference not being a symbolic\n+ * reference to allow backends to optimize this operation in case symbolic and\n+ * non-symbolic references are treated differently.\n+ */\n+typedef int read_symbolic_ref_fn(struct ref_store *ref_store, const char *refname,\n+\t\t\t\t struct strbuf *referent);\n+\n struct ref_storage_be {\n \tstruct ref_storage_be *next;\n \tconst char *name;\n@@ -668,6 +683,7 @@ struct ref_storage_be {\n \n \tref_iterator_begin_fn *iterator_begin;\n \tread_raw_ref_fn *read_raw_ref;\n+\tread_symbolic_ref_fn *read_symbolic_ref;\n \n \treflog_iterator_begin_fn *reflog_iterator_begin;\n \tfor_each_reflog_ent_fn *for_each_reflog_ent;\n-- \n2.35.1\n\n"},{"id":"449245","messageId":"1d24101fe4fb296257110eb82da67c95b5e03189.1645619224.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1645619224.git.ps@pks.im","subject":"[PATCH 4/5] remote: read symbolic refs via `refs_read_symbolic_ref()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-23T12:35:39Z","receivedAt":"2022-02-23T12:35:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have two cases in the remote code where we check whether a reference\nis symbolic or not, but don't mind in case it doesn't exist or in case\nit exists but is a non-symbolic reference. Convert these two callsites\nto use the new `refs_read_symbolic_ref()` function, whose intent is to\nimplement exactly that usecase.\n\nNo change in behaviour is expected from this change.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/remote.c | 8 +++++---\n remote.c | 14 +++++++-------\n 2 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 6f27ddc47b..0142ed09b5 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -766,13 +766,15 @@ static int mv(int argc, const char **argv)\n \tfor_each_ref(read_remote_branches, &rename);\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n-\t\tint flag = 0;\n+\t\tstruct strbuf referent = STRBUF_INIT;\n \n-\t\tread_ref_full(item->string, RESOLVE_REF_READING, NULL, &flag);\n-\t\tif (!(flag & REF_ISSYMREF))\n+\t\tif (refs_read_symbolic_ref(get_main_ref_store(the_repository), item->string,\n+\t\t\t\t\t &referent))\n \t\t\tcontinue;\n \t\tif (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))\n \t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n+\n+\t\tstrbuf_release(&referent);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\ndiff --git a/remote.c b/remote.c\nindex c97c626eaa..42a4e7106e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1945,13 +1945,9 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err)\n \treturn branch->push_tracking_ref;\n }\n \n-static int ignore_symref_update(const char *refname)\n+static int ignore_symref_update(const char *refname, struct strbuf *scratch)\n {\n-\tint flag;\n-\n-\tif (!resolve_ref_unsafe(refname, 0, NULL, &flag))\n-\t\treturn 0; /* non-existing refs are OK */\n-\treturn (flag & REF_ISSYMREF);\n+\treturn !refs_read_symbolic_ref(get_main_ref_store(the_repository), refname, scratch);\n }\n \n /*\n@@ -1964,6 +1960,7 @@ static int ignore_symref_update(const char *refname)\n static struct ref *get_expanded_map(const struct ref *remote_refs,\n \t\t\t\t const struct refspec_item *refspec)\n {\n+\tstruct strbuf scratch = STRBUF_INIT;\n \tconst struct ref *ref;\n \tstruct ref *ret = NULL;\n \tstruct ref **tail = &ret;\n@@ -1971,11 +1968,13 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tchar *expn_name = NULL;\n \n+\t\tstrbuf_reset(&scratch);\n+\n \t\tif (strchr(ref->name, '^'))\n \t\t\tcontinue; /* a dereference item */\n \t\tif (match_name_with_pattern(refspec->src, ref->name,\n \t\t\t\t\t refspec->dst, &expn_name) &&\n-\t\t !ignore_symref_update(expn_name)) {\n+\t\t !ignore_symref_update(expn_name, &scratch)) {\n \t\t\tstruct ref *cpy = copy_ref(ref);\n \n \t\t\tcpy->peer_ref = alloc_ref(expn_name);\n@@ -1987,6 +1986,7 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n \t\tfree(expn_name);\n \t}\n \n+\tstrbuf_release(&scratch);\n \treturn ret;\n }\n \n-- \n2.35.1\n\n"},{"id":"449246","messageId":"7213ffdbddc7b14c8665974e41bda9449f66de4a.1645619224.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1645619224.git.ps@pks.im","subject":"[PATCH 5/5] refs/files-backend: optimize reading of symbolic refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-02-23T12:35:43Z","receivedAt":"2022-02-23T12:35:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When reading references via `files_read_raw_ref()` we always consult\nboth the loose reference, and if that wasn't found, we also consult the\npacked-refs file. While this makes sense to read a generic reference, it\nis wasteful in the case where we only care about symbolic references:\nthe packed-refs backend does not support them, and thus it cannot ever\nreturn one for us.\n\nSpecial-case reading of symbolic references for the files backend such\nthat we always skip asking the packed-refs backend.\n\nWe use `refs_read_symbolic_ref()` extensively to determine whether we\nneed to skip updating local symbolic references during a fetch, which is\nwhy the change results in a significant speedup when doing fetches in\nrepositories with huge numbers of references. The following benchmark\nexecutes a mirror-fetch in a repository with about 2 million references:\n\n Benchmark 1: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)\n Time (mean ± σ): 68.372 s ± 2.344 s [User: 65.629 s, System: 8.786 s]\n Range (min … max): 65.745 s … 70.246 s 3 runs\n\n Benchmark 2: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)\n Time (mean ± σ): 60.259 s ± 0.343 s [User: 61.019 s, System: 7.245 s]\n Range (min … max): 60.003 s … 60.649 s 3 runs\n\n Summary\n 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)' ran\n 1.13 ± 0.04 times faster than 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)'\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 34 ++++++++++++++++++++++++++++------\n 1 file changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f3428a9f12..0457ecdb42 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -338,9 +338,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)\n \treturn refs->loose;\n }\n \n-static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n-\t\t\t struct object_id *oid, struct strbuf *referent,\n-\t\t\t unsigned int *type, int *failure_errno)\n+static int read_ref_internal(struct ref_store *ref_store, const char *refname,\n+\t\t\t struct object_id *oid, struct strbuf *referent,\n+\t\t\t unsigned int *type, int *failure_errno, int skip_packed_refs)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n@@ -381,7 +381,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \tif (lstat(path, &st) < 0) {\n \t\tint ignore_errno;\n \t\tmyerr = errno;\n-\t\tif (myerr != ENOENT)\n+\t\tif (myerr != ENOENT || skip_packed_refs)\n \t\t\tgoto out;\n \t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n \t\t\t\t referent, type, &ignore_errno)) {\n@@ -425,7 +425,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t * ref is supposed to be, there could still be a\n \t\t * packed ref:\n \t\t */\n-\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n+\t\tif (skip_packed_refs ||\n+\t\t refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n \t\t\t\t referent, type, &ignore_errno)) {\n \t\t\tmyerr = EISDIR;\n \t\t\tgoto out;\n@@ -470,6 +471,27 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \treturn ret;\n }\n \n+static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n+\t\t\t struct object_id *oid, struct strbuf *referent,\n+\t\t\t unsigned int *type, int *failure_errno)\n+{\n+\treturn read_ref_internal(ref_store, refname, oid, referent, type, failure_errno, 0);\n+}\n+\n+static int files_read_symbolic_ref(struct ref_store *ref_store, const char *refname,\n+\t\t\t\t struct strbuf *referent)\n+{\n+\tstruct object_id oid;\n+\tint failure_errno, ret;\n+\tunsigned int type;\n+\n+\tret = read_ref_internal(ref_store, refname, &oid, referent, &type, &failure_errno, 1);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\treturn !(type & REF_ISSYMREF);\n+}\n+\n int parse_loose_ref_contents(const char *buf, struct object_id *oid,\n \t\t\t struct strbuf *referent, unsigned int *type,\n \t\t\t int *failure_errno)\n@@ -3286,7 +3308,7 @@ struct ref_storage_be refs_be_files = {\n \n \tfiles_ref_iterator_begin,\n \tfiles_read_raw_ref,\n-\tNULL,\n+\tfiles_read_symbolic_ref,\n \n \tfiles_reflog_iterator_begin,\n \tfiles_for_each_reflog_ent,\n-- \n2.35.1\n\n"},{"id":"449250","messageId":"ddb91a1d-6ddf-ba25-f1af-ba3f4c18726e@github.com","threadId":"57461","inReplyTo":"ca5e136cca495c7d927e99f5ae8a672d93823eea.1645619224.git.ps@pks.im","subject":"Re: [PATCH 1/5] upload-pack: look up \"want\" lines via commit-graph","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-02-23T14:13:53Z","receivedAt":"2022-02-23T14:13:59Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/23/2022 7:35 AM, Patrick Steinhardt wrote:\n\n> The following benchmark is executed in a repository with a huge number\n> of references. It uses cached request from git-fetch(1) as input and\n> contains about 876,000 \"want\" lines:\n> \n> Benchmark 1: git-upload-pack (HEAD~)\n> Time (mean ± σ): 7.113 s ± 0.028 s [User: 6.900 s, System: 0.662 s]\n> Range (min … max): 7.072 s … 7.168 s 10 runs\n> \n> Benchmark 2: git-upload-pack (HEAD)\n> Time (mean ± σ): 6.622 s ± 0.061 s [User: 6.452 s, System: 0.650 s]\n> Range (min … max): 6.535 s … 6.727 s 10 runs\n> \n> Summary\n> 'git-upload-pack (HEAD)' ran\n> 1.07 ± 0.01 times faster than 'git-upload-pack (HEAD~)'\n\nNice!\n\n> -\t\to = parse_object(the_repository, &oid);\n> +\t\tcommit = lookup_commit_in_graph(the_repository, &oid);\n> +\t\tif (commit)\n> +\t\t\to = &commit->object;\n> +\t\telse\n> +\t\t\to = parse_object(the_repository, &oid);\n> +\n\nThis is a neat trick. I see that we've also done this trick in\nrevision.c:get_reference(). Perhaps it is worth creating a helper,\nmaybe named parse_probably_commit()?\n\n> \t\tif (!o) {\n> \t\t\tpacket_writer_error(writer,\n> \t\t\t\t\t \"upload-pack: not our ref %s\",\n> @@ -1434,7 +1440,7 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> \tif (skip_prefix(line, \"want-ref \", &refname_nons)) {\n> \t\tstruct object_id oid;\n> \t\tstruct string_list_item *item;\n> -\t\tstruct object *o;\n> +\t\tstruct object *o = NULL;\n> \t\tstruct strbuf refname = STRBUF_INIT;\n> \n> \t\tstrbuf_addf(&refname, \"%s%s\", get_git_namespace(), refname_nons);\n> @@ -1448,7 +1454,15 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> \t\titem = string_list_append(wanted_refs, refname_nons);\n> \t\titem->util = oiddup(&oid);\n> \n> -\t\to = parse_object_or_die(&oid, refname_nons);\n> +\t\tif (!starts_with(refname_nons, \"refs/tags/\")) {\n> +\t\t\tstruct commit *commit = lookup_commit_in_graph(the_repository, &oid);\n> +\t\t\tif (commit)\n> +\t\t\t\to = &commit->object;\n> +\t\t}\n> +\n> +\t\tif (!o)\n> +\t\t\to = parse_object_or_die(&oid, refname_nons);\n> +\n\nEven here, we _could_ use a parse_probably_commit() helper\ninside the if (!starts_with(...)) block, even though we would\nstill need the if (!o) check later.\n\nThanks,\n-Stolee\n"},{"id":"449251","messageId":"33154b3c-1316-aab3-46a1-a0fbd87293de@github.com","threadId":"57461","inReplyTo":"80f993dddd521133154a751aeaab86adee409eea.1645619224.git.ps@pks.im","subject":"Re: [PATCH 2/5] fetch: avoid lookup of commits when not appending to FETCH_HEAD","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-02-23T14:18:05Z","receivedAt":"2022-02-23T14:18:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 2/23/2022 7:35 AM, Patrick Steinhardt wrote:\n> When fetching from a remote repository we will by default write what has\n> been fetched into the special FETCH_HEAD reference. The order in which\n> references are written depends on whether the reference is for merge or\n> not, which, despite some other conditions, is also determined based on\n> whether the old object ID the reference is being updated from actually\n> exists in the repository.\n> \n> To write FETCH_HEAD we thus loop through all references thrice: once for\n> the references that are about to be merged, once for the references that\n> are not for merge, and finally for all references that are ignored. For\n> every iteration, we then look up the old object ID to determine whether\n> the referenced object exists so that we can label it as \"not-for-merge\"\n> if it doesn't exist. It goes without saying that this can be expensive\n> in case where we are fetching a lot of references.\n> \n> While this is hard to avoid in the case where we're writing FETCH_HEAD,\n> users can in fact ask us to skip this work via `--no-write-fetch-head`.\n> In that case, we do not care for the result of those lookups at all\n> because we don't have to order writes to FETCH_HEAD in the first place.\n> \n> Skip this busywork in case we're not writing to FETCH_HEAD. The\n> following benchmark performs a mirror-fetch in a repository with about\n> two million references:\n> \n> Benchmark 1: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)\n> Time (mean ± σ): 75.388 s ± 1.942 s [User: 71.103 s, System: 8.953 s]\n> Range (min … max): 73.184 s … 76.845 s 3 runs\n> \n> Benchmark 2: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)\n> Time (mean ± σ): 69.486 s ± 1.016 s [User: 65.941 s, System: 8.806 s]\n> Range (min … max): 68.864 s … 70.659 s 3 runs\n> \n> Summary\n> 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)' ran\n> 1.08 ± 0.03 times faster than 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)'\n\nI have a super-small nitpick here.\n\nI see that you are using '-n' to name your experiments. These names\nare a bit long, especially since they are the same Git command but\nbuilt at different commits. It would be enough to say the command\nyou are testing before the stats and leave the names as \"HEAD\" and\n\"HEAD~\" (or, I typically use \"new\" and \"old\", respectively).\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 * When writing FETCH_HEAD we need to determine whether\n> +\t\t\t * we already have the commit or not. If not, then the\n> +\t\t\t * reference is not for merge and needs to be written\n> +\t\t\t * to the reflog after other commits which we already\n> +\t\t\t * have. We're not interested in this property though\n> +\t\t\t * in case FETCH_HEAD is not to be updated, so we can\n> +\t\t\t * skip the classification in that case.\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\tif (fetch_head->fp) {\n> +\t\t\t\tstruct commit *commit = NULL;\n> +\n> +\t\t\t\t/*\n> +\t\t\t\t * References in \"refs/tags/\" are often going to point\n> +\t\t\t\t * to annotated tags, which are not part of the\n> +\t\t\t\t * commit-graph. We thus only try to look up refs in\n> +\t\t\t\t * the graph which are not in that namespace to not\n> +\t\t\t\t * regress performance in repositories with many\n> +\t\t\t\t * annotated tags.\n> +\t\t\t\t */\n> +\t\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n> +\t\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n> +\t\t\t\tif (!commit) {\n> +\t\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n> +\t\t\t\t\t\t\t\t\t\t&rm->old_oid,\n> +\t\t\t\t\t\t\t\t\t\t1);\n> +\t\t\t\t\tif (!commit)\n> +\t\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n> +\t\t\t\t}\n> \t\t\t}\nLooks good. Most of the diff is whitespace.\n\nThanks,\n-Stolee\n"},{"id":"449885","messageId":"Yh3cprbfc/UQ01fo@ncase","threadId":"57461","inReplyTo":"ddb91a1d-6ddf-ba25-f1af-ba3f4c18726e@github.com","subject":"Re: [PATCH 1/5] upload-pack: look up \"want\" lines via commit-graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T08:43:18Z","receivedAt":"2022-03-01T08:43:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 23, 2022 at 09:13:53AM -0500, Derrick Stolee wrote:\n> On 2/23/2022 7:35 AM, Patrick Steinhardt wrote:\n> \n> > The following benchmark is executed in a repository with a huge number\n> > of references. It uses cached request from git-fetch(1) as input and\n> > contains about 876,000 \"want\" lines:\n> > \n> > Benchmark 1: git-upload-pack (HEAD~)\n> > Time (mean ± σ): 7.113 s ± 0.028 s [User: 6.900 s, System: 0.662 s]\n> > Range (min … max): 7.072 s … 7.168 s 10 runs\n> > \n> > Benchmark 2: git-upload-pack (HEAD)\n> > Time (mean ± σ): 6.622 s ± 0.061 s [User: 6.452 s, System: 0.650 s]\n> > Range (min … max): 6.535 s … 6.727 s 10 runs\n> > \n> > Summary\n> > 'git-upload-pack (HEAD)' ran\n> > 1.07 ± 0.01 times faster than 'git-upload-pack (HEAD~)'\n> \n> Nice!\n> \n> > -\t\to = parse_object(the_repository, &oid);\n> > +\t\tcommit = lookup_commit_in_graph(the_repository, &oid);\n> > +\t\tif (commit)\n> > +\t\t\to = &commit->object;\n> > +\t\telse\n> > +\t\t\to = parse_object(the_repository, &oid);\n> > +\n> \n> This is a neat trick. I see that we've also done this trick in\n> revision.c:get_reference(). Perhaps it is worth creating a helper,\n> maybe named parse_probably_commit()?\n\nThat might be a good idea, thanks. I'll have a look at what the end\nresult would look like.\n\nPatrick\n\n> > \t\tif (!o) {\n> > \t\t\tpacket_writer_error(writer,\n> > \t\t\t\t\t \"upload-pack: not our ref %s\",\n> > @@ -1434,7 +1440,7 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> > \tif (skip_prefix(line, \"want-ref \", &refname_nons)) {\n> > \t\tstruct object_id oid;\n> > \t\tstruct string_list_item *item;\n> > -\t\tstruct object *o;\n> > +\t\tstruct object *o = NULL;\n> > \t\tstruct strbuf refname = STRBUF_INIT;\n> > \n> > \t\tstrbuf_addf(&refname, \"%s%s\", get_git_namespace(), refname_nons);\n> > @@ -1448,7 +1454,15 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> > \t\titem = string_list_append(wanted_refs, refname_nons);\n> > \t\titem->util = oiddup(&oid);\n> > \n> > -\t\to = parse_object_or_die(&oid, refname_nons);\n> > +\t\tif (!starts_with(refname_nons, \"refs/tags/\")) {\n> > +\t\t\tstruct commit *commit = lookup_commit_in_graph(the_repository, &oid);\n> > +\t\t\tif (commit)\n> > +\t\t\t\to = &commit->object;\n> > +\t\t}\n> > +\n> > +\t\tif (!o)\n> > +\t\t\to = parse_object_or_die(&oid, refname_nons);\n> > +\n> \n> Even here, we _could_ use a parse_probably_commit() helper\n> inside the if (!starts_with(...)) block, even though we would\n> still need the if (!o) check later.\n> \n> Thanks,\n> -Stolee\n"},{"id":"449886","messageId":"Yh3c0PvV1f3HEsOI@ncase","threadId":"57461","inReplyTo":"33154b3c-1316-aab3-46a1-a0fbd87293de@github.com","subject":"Re: [PATCH 2/5] fetch: avoid lookup of commits when not appending to FETCH_HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T08:44:00Z","receivedAt":"2022-03-01T08:44:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Feb 23, 2022 at 09:18:05AM -0500, Derrick Stolee wrote:\n> On 2/23/2022 7:35 AM, Patrick Steinhardt wrote:\n> > When fetching from a remote repository we will by default write what has\n> > been fetched into the special FETCH_HEAD reference. The order in which\n> > references are written depends on whether the reference is for merge or\n> > not, which, despite some other conditions, is also determined based on\n> > whether the old object ID the reference is being updated from actually\n> > exists in the repository.\n> > \n> > To write FETCH_HEAD we thus loop through all references thrice: once for\n> > the references that are about to be merged, once for the references that\n> > are not for merge, and finally for all references that are ignored. For\n> > every iteration, we then look up the old object ID to determine whether\n> > the referenced object exists so that we can label it as \"not-for-merge\"\n> > if it doesn't exist. It goes without saying that this can be expensive\n> > in case where we are fetching a lot of references.\n> > \n> > While this is hard to avoid in the case where we're writing FETCH_HEAD,\n> > users can in fact ask us to skip this work via `--no-write-fetch-head`.\n> > In that case, we do not care for the result of those lookups at all\n> > because we don't have to order writes to FETCH_HEAD in the first place.\n> > \n> > Skip this busywork in case we're not writing to FETCH_HEAD. The\n> > following benchmark performs a mirror-fetch in a repository with about\n> > two million references:\n> > \n> > Benchmark 1: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)\n> > Time (mean ± σ): 75.388 s ± 1.942 s [User: 71.103 s, System: 8.953 s]\n> > Range (min … max): 73.184 s … 76.845 s 3 runs\n> > \n> > Benchmark 2: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)\n> > Time (mean ± σ): 69.486 s ± 1.016 s [User: 65.941 s, System: 8.806 s]\n> > Range (min … max): 68.864 s … 70.659 s 3 runs\n> > \n> > Summary\n> > 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)' ran\n> > 1.08 ± 0.03 times faster than 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)'\n> \n> I have a super-small nitpick here.\n> \n> I see that you are using '-n' to name your experiments. These names\n> are a bit long, especially since they are the same Git command but\n> built at different commits. It would be enough to say the command\n> you are testing before the stats and leave the names as \"HEAD\" and\n> \"HEAD~\" (or, I typically use \"new\" and \"old\", respectively).\n\nFair enough, will change.\n\nPatrick\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 * When writing FETCH_HEAD we need to determine whether\n> > +\t\t\t * we already have the commit or not. If not, then the\n> > +\t\t\t * reference is not for merge and needs to be written\n> > +\t\t\t * to the reflog after other commits which we already\n> > +\t\t\t * have. We're not interested in this property though\n> > +\t\t\t * in case FETCH_HEAD is not to be updated, so we can\n> > +\t\t\t * skip the classification in that case.\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\tif (fetch_head->fp) {\n> > +\t\t\t\tstruct commit *commit = NULL;\n> > +\n> > +\t\t\t\t/*\n> > +\t\t\t\t * References in \"refs/tags/\" are often going to point\n> > +\t\t\t\t * to annotated tags, which are not part of the\n> > +\t\t\t\t * commit-graph. We thus only try to look up refs in\n> > +\t\t\t\t * the graph which are not in that namespace to not\n> > +\t\t\t\t * regress performance in repositories with many\n> > +\t\t\t\t * annotated tags.\n> > +\t\t\t\t */\n> > +\t\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n> > +\t\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n> > +\t\t\t\tif (!commit) {\n> > +\t\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n> > +\t\t\t\t\t\t\t\t\t\t&rm->old_oid,\n> > +\t\t\t\t\t\t\t\t\t\t1);\n> > +\t\t\t\t\tif (!commit)\n> > +\t\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n> > +\t\t\t\t}\n> > \t\t\t}\n> Looks good. Most of the diff is whitespace.\n> \n> Thanks,\n> -Stolee\n"},{"id":"449887","messageId":"Yh3mR3/h/+nFyF6V@ncase","threadId":"57461","inReplyTo":"Yh3cprbfc/UQ01fo@ncase","subject":"Re: [PATCH 1/5] upload-pack: look up \"want\" lines via commit-graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T09:24:23Z","receivedAt":"2022-03-01T09:24:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 01, 2022 at 09:43:19AM +0100, Patrick Steinhardt wrote:\n> On Wed, Feb 23, 2022 at 09:13:53AM -0500, Derrick Stolee wrote:\n> > On 2/23/2022 7:35 AM, Patrick Steinhardt wrote:\n> > \n> > > The following benchmark is executed in a repository with a huge number\n> > > of references. It uses cached request from git-fetch(1) as input and\n> > > contains about 876,000 \"want\" lines:\n> > > \n> > > Benchmark 1: git-upload-pack (HEAD~)\n> > > Time (mean ± σ): 7.113 s ± 0.028 s [User: 6.900 s, System: 0.662 s]\n> > > Range (min … max): 7.072 s … 7.168 s 10 runs\n> > > \n> > > Benchmark 2: git-upload-pack (HEAD)\n> > > Time (mean ± σ): 6.622 s ± 0.061 s [User: 6.452 s, System: 0.650 s]\n> > > Range (min … max): 6.535 s … 6.727 s 10 runs\n> > > \n> > > Summary\n> > > 'git-upload-pack (HEAD)' ran\n> > > 1.07 ± 0.01 times faster than 'git-upload-pack (HEAD~)'\n> > \n> > Nice!\n> > \n> > > -\t\to = parse_object(the_repository, &oid);\n> > > +\t\tcommit = lookup_commit_in_graph(the_repository, &oid);\n> > > +\t\tif (commit)\n> > > +\t\t\to = &commit->object;\n> > > +\t\telse\n> > > +\t\t\to = parse_object(the_repository, &oid);\n> > > +\n> > \n> > This is a neat trick. I see that we've also done this trick in\n> > revision.c:get_reference(). Perhaps it is worth creating a helper,\n> > maybe named parse_probably_commit()?\n> \n> That might be a good idea, thanks. I'll have a look at what the end\n> result would look like.\n> \n> Patrick\n\nI had a look at existing callsites which use `lookup_commit_in_graph()`,\nbut I found that it wasn't easily possible to convert them all to use a\nnew helper like you propose. Most of them have some custom logic like\nskipping `parse_object()` if it's part of a promisor pack, so I really\nonly found two locations where such a new helper could be used without\nalso adding and supporting flags. I don't really think that's worth it\nfor now.\n\nPatrick\n\n> > > \t\tif (!o) {\n> > > \t\t\tpacket_writer_error(writer,\n> > > \t\t\t\t\t \"upload-pack: not our ref %s\",\n> > > @@ -1434,7 +1440,7 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> > > \tif (skip_prefix(line, \"want-ref \", &refname_nons)) {\n> > > \t\tstruct object_id oid;\n> > > \t\tstruct string_list_item *item;\n> > > -\t\tstruct object *o;\n> > > +\t\tstruct object *o = NULL;\n> > > \t\tstruct strbuf refname = STRBUF_INIT;\n> > > \n> > > \t\tstrbuf_addf(&refname, \"%s%s\", get_git_namespace(), refname_nons);\n> > > @@ -1448,7 +1454,15 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n> > > \t\titem = string_list_append(wanted_refs, refname_nons);\n> > > \t\titem->util = oiddup(&oid);\n> > > \n> > > -\t\to = parse_object_or_die(&oid, refname_nons);\n> > > +\t\tif (!starts_with(refname_nons, \"refs/tags/\")) {\n> > > +\t\t\tstruct commit *commit = lookup_commit_in_graph(the_repository, &oid);\n> > > +\t\t\tif (commit)\n> > > +\t\t\t\to = &commit->object;\n> > > +\t\t}\n> > > +\n> > > +\t\tif (!o)\n> > > +\t\t\to = parse_object_or_die(&oid, refname_nons);\n> > > +\n> > \n> > Even here, we _could_ use a parse_probably_commit() helper\n> > inside the if (!starts_with(...)) block, even though we would\n> > still need the if (!o) check later.\n> > \n> > Thanks,\n> > -Stolee\n\n\n"},{"id":"449890","messageId":"cover.1646127015.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1645619224.git.ps@pks.im","subject":"[PATCH v2 0/5] fetch: more optimizations for mirror fetches","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T09:33:33Z","receivedAt":"2022-03-01T09:34:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is another patch series with the aim to speed up mirror fetches. It\napplies on top of e6ebfd0e8c (The sixth batch, 2022-02-18) with\n3824153b23 (Merge branch 'ps/fetch-atomic' into next, 2022-02-18) merged\ninto it to fix a conflict.\n\nThe only change compared to v2 is an update to the benchmarks so that\nthey're less verbose, as proposed by Derrick. I also had a look at\nintroducing a new helper `parse_object_probably_commit()`, but I didn't\nfind the end result to be much of an improvement compared to the ad-hoc\n`lookup_commit_in_graph() || parse_object()` dance we do right now.\n\nThanks!\n\nPatrick\n\nPatrick Steinhardt (5):\n upload-pack: look up \"want\" lines via commit-graph\n fetch: avoid lookup of commits when not appending to FETCH_HEAD\n refs: add ability for backends to special-case reading of symbolic\n refs\n remote: read symbolic refs via `refs_read_symbolic_ref()`\n refs/files-backend: optimize reading of symbolic refs\n\n builtin/fetch.c | 42 +++++++++++++++++++++++++++---------------\n builtin/remote.c | 8 +++++---\n refs.c | 17 +++++++++++++++++\n refs.h | 3 +++\n refs/debug.c | 1 +\n refs/files-backend.c | 33 ++++++++++++++++++++++++++++-----\n refs/packed-backend.c | 1 +\n refs/refs-internal.h | 16 ++++++++++++++++\n remote.c | 14 +++++++-------\n upload-pack.c | 20 +++++++++++++++++---\n 10 files changed, 122 insertions(+), 33 deletions(-)\n\nRange-diff against v1:\n1: ca5e136cca ! 1: b5c696bd8e upload-pack: look up \"want\" lines via commit-graph\n @@ Commit message\n Refactor parsing of both \"want\" and \"want-ref\" lines to do so.\n \n The following benchmark is executed in a repository with a huge number\n - of references. It uses cached request from git-fetch(1) as input and\n - contains about 876,000 \"want\" lines:\n + of references. It uses cached request from git-fetch(1) as input to\n + git-upload-pack(1) that contains about 876,000 \"want\" lines:\n \n - Benchmark 1: git-upload-pack (HEAD~)\n + Benchmark 1: HEAD~\n Time (mean ± σ): 7.113 s ± 0.028 s [User: 6.900 s, System: 0.662 s]\n Range (min … max): 7.072 s … 7.168 s 10 runs\n \n - Benchmark 2: git-upload-pack (HEAD)\n + Benchmark 2: HEAD\n Time (mean ± σ): 6.622 s ± 0.061 s [User: 6.452 s, System: 0.650 s]\n Range (min … max): 6.535 s … 6.727 s 10 runs\n \n Summary\n - 'git-upload-pack (HEAD)' ran\n - 1.07 ± 0.01 times faster than 'git-upload-pack (HEAD~)'\n + 'HEAD' ran\n + 1.07 ± 0.01 times faster than 'HEAD~'\n \n Signed-off-by: Patrick Steinhardt \n \n2: 80f993dddd ! 2: fbe76b78c3 fetch: avoid lookup of commits when not appending to FETCH_HEAD\n @@ Commit message\n \n Skip this busywork in case we're not writing to FETCH_HEAD. The\n following benchmark performs a mirror-fetch in a repository with about\n - two million references:\n + two million references via `git fetch --prune --no-write-fetch-head\n + +refs/*:refs/*`:\n \n - Benchmark 1: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)\n + Benchmark 1: HEAD~\n Time (mean ± σ): 75.388 s ± 1.942 s [User: 71.103 s, System: 8.953 s]\n Range (min … max): 73.184 s … 76.845 s 3 runs\n \n - Benchmark 2: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)\n + Benchmark 2: HEAD\n Time (mean ± σ): 69.486 s ± 1.016 s [User: 65.941 s, System: 8.806 s]\n Range (min … max): 68.864 s … 70.659 s 3 runs\n \n Summary\n - 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)' ran\n - 1.08 ± 0.03 times faster than 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)'\n + 'HEAD' ran\n + 1.08 ± 0.03 times faster than 'HEAD~'\n \n Signed-off-by: Patrick Steinhardt \n \n3: 28cacbdbe2 = 3: 29eb81d37c refs: add ability for backends to special-case reading of symbolic refs\n4: 1d24101fe4 = 4: 0489380e00 remote: read symbolic refs via `refs_read_symbolic_ref()`\n5: 7213ffdbdd ! 5: b6eca63d3b refs/files-backend: optimize reading of symbolic refs\n @@ Commit message\n need to skip updating local symbolic references during a fetch, which is\n why the change results in a significant speedup when doing fetches in\n repositories with huge numbers of references. The following benchmark\n - executes a mirror-fetch in a repository with about 2 million references:\n + executes a mirror-fetch in a repository with about 2 million references\n + via `git fetch --prune --no-write-fetch-head +refs/*:refs/*`:\n \n - Benchmark 1: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)\n + Benchmark 1: HEAD~\n Time (mean ± σ): 68.372 s ± 2.344 s [User: 65.629 s, System: 8.786 s]\n Range (min … max): 65.745 s … 70.246 s 3 runs\n \n - Benchmark 2: git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)\n + Benchmark 2: HEAD\n Time (mean ± σ): 60.259 s ± 0.343 s [User: 61.019 s, System: 7.245 s]\n Range (min … max): 60.003 s … 60.649 s 3 runs\n \n Summary\n - 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD)' ran\n - 1.13 ± 0.04 times faster than 'git fetch --prune --no-write-fetch-head +refs/*:refs/* (HEAD~)'\n + 'HEAD' ran\n + 1.13 ± 0.04 times faster than 'HEAD~'\n \n Signed-off-by: Patrick Steinhardt \n \n-- \n2.35.1\n\n"},{"id":"449891","messageId":"b5c696bd8e4b33e782e4837ad5051268e35f9828.1646127015.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1646127015.git.ps@pks.im","subject":"[PATCH v2 1/5] upload-pack: look up \"want\" lines via commit-graph","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T09:33:37Z","receivedAt":"2022-03-01T09:34:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"During packfile negotiation the client will send \"want\" and \"want-ref\"\nlines to the server to tell it which objects it is interested in. The\nserver-side parses each of those and looks them up to see whether it\nactually has requested objects. This lookup is performed by calling\n`parse_object()` directly, which thus hits the object database. In the\ngeneral case though most of the objects the client requests will be\ncommits. We can thus try to look up the object via the commit-graph\nopportunistically, which is much faster than doing the same via the\nobject database.\n\nRefactor parsing of both \"want\" and \"want-ref\" lines to do so.\n\nThe following benchmark is executed in a repository with a huge number\nof references. It uses cached request from git-fetch(1) as input to\ngit-upload-pack(1) that contains about 876,000 \"want\" lines:\n\n Benchmark 1: HEAD~\n Time (mean ± σ): 7.113 s ± 0.028 s [User: 6.900 s, System: 0.662 s]\n Range (min … max): 7.072 s … 7.168 s 10 runs\n\n Benchmark 2: HEAD\n Time (mean ± σ): 6.622 s ± 0.061 s [User: 6.452 s, System: 0.650 s]\n Range (min … max): 6.535 s … 6.727 s 10 runs\n\n Summary\n 'HEAD' ran\n 1.07 ± 0.01 times faster than 'HEAD~'\n\nSigned-off-by: Patrick Steinhardt \n---\n upload-pack.c | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 8acc98741b..3a851b3606 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -1400,13 +1400,19 @@ static int parse_want(struct packet_writer *writer, const char *line,\n \tconst char *arg;\n \tif (skip_prefix(line, \"want \", &arg)) {\n \t\tstruct object_id oid;\n+\t\tstruct commit *commit;\n \t\tstruct object *o;\n \n \t\tif (get_oid_hex(arg, &oid))\n \t\t\tdie(\"git upload-pack: protocol error, \"\n \t\t\t \"expected to get oid, not '%s'\", line);\n \n-\t\to = parse_object(the_repository, &oid);\n+\t\tcommit = lookup_commit_in_graph(the_repository, &oid);\n+\t\tif (commit)\n+\t\t\to = &commit->object;\n+\t\telse\n+\t\t\to = parse_object(the_repository, &oid);\n+\n \t\tif (!o) {\n \t\t\tpacket_writer_error(writer,\n \t\t\t\t\t \"upload-pack: not our ref %s\",\n@@ -1434,7 +1440,7 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n \tif (skip_prefix(line, \"want-ref \", &refname_nons)) {\n \t\tstruct object_id oid;\n \t\tstruct string_list_item *item;\n-\t\tstruct object *o;\n+\t\tstruct object *o = NULL;\n \t\tstruct strbuf refname = STRBUF_INIT;\n \n \t\tstrbuf_addf(&refname, \"%s%s\", get_git_namespace(), refname_nons);\n@@ -1448,7 +1454,15 @@ static int parse_want_ref(struct packet_writer *writer, const char *line,\n \t\titem = string_list_append(wanted_refs, refname_nons);\n \t\titem->util = oiddup(&oid);\n \n-\t\to = parse_object_or_die(&oid, refname_nons);\n+\t\tif (!starts_with(refname_nons, \"refs/tags/\")) {\n+\t\t\tstruct commit *commit = lookup_commit_in_graph(the_repository, &oid);\n+\t\t\tif (commit)\n+\t\t\t\to = &commit->object;\n+\t\t}\n+\n+\t\tif (!o)\n+\t\t\to = parse_object_or_die(&oid, refname_nons);\n+\n \t\tif (!(o->flags & WANTED)) {\n \t\t\to->flags |= WANTED;\n \t\t\tadd_object_array(o, NULL, want_obj);\n-- \n2.35.1\n\n"},{"id":"449892","messageId":"fbe76b78c30d98f1b10c474b1e0ddf6fa4db44fc.1646127015.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1646127015.git.ps@pks.im","subject":"[PATCH v2 2/5] fetch: avoid lookup of commits when not appending to FETCH_HEAD","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T09:33:41Z","receivedAt":"2022-03-01T09:34:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When fetching from a remote repository we will by default write what has\nbeen fetched into the special FETCH_HEAD reference. The order in which\nreferences are written depends on whether the reference is for merge or\nnot, which, despite some other conditions, is also determined based on\nwhether the old object ID the reference is being updated from actually\nexists in the repository.\n\nTo write FETCH_HEAD we thus loop through all references thrice: once for\nthe references that are about to be merged, once for the references that\nare not for merge, and finally for all references that are ignored. For\nevery iteration, we then look up the old object ID to determine whether\nthe referenced object exists so that we can label it as \"not-for-merge\"\nif it doesn't exist. It goes without saying that this can be expensive\nin case where we are fetching a lot of references.\n\nWhile this is hard to avoid in the case where we're writing FETCH_HEAD,\nusers can in fact ask us to skip this work via `--no-write-fetch-head`.\nIn that case, we do not care for the result of those lookups at all\nbecause we don't have to order writes to FETCH_HEAD in the first place.\n\nSkip this busywork in case we're not writing to FETCH_HEAD. The\nfollowing benchmark performs a mirror-fetch in a repository with about\ntwo million references via `git fetch --prune --no-write-fetch-head\n+refs/*:refs/*`:\n\n Benchmark 1: HEAD~\n Time (mean ± σ): 75.388 s ± 1.942 s [User: 71.103 s, System: 8.953 s]\n Range (min … max): 73.184 s … 76.845 s 3 runs\n\n Benchmark 2: HEAD\n Time (mean ± σ): 69.486 s ± 1.016 s [User: 65.941 s, System: 8.806 s]\n Range (min … max): 68.864 s … 70.659 s 3 runs\n\n Summary\n 'HEAD' ran\n 1.08 ± 0.03 times faster than 'HEAD~'\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fetch.c | 42 +++++++++++++++++++++++++++---------------\n 1 file changed, 27 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e8305b6662..4d12c2fd4d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1146,7 +1146,6 @@ 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@@ -1157,21 +1156,34 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t\t}\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 * When writing FETCH_HEAD we need to determine whether\n+\t\t\t * we already have the commit or not. If not, then the\n+\t\t\t * reference is not for merge and needs to be written\n+\t\t\t * to the reflog after other commits which we already\n+\t\t\t * have. We're not interested in this property though\n+\t\t\t * in case FETCH_HEAD is not to be updated, so we can\n+\t\t\t * skip the classification in that case.\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\tif (fetch_head->fp) {\n+\t\t\t\tstruct commit *commit = NULL;\n+\n+\t\t\t\t/*\n+\t\t\t\t * References in \"refs/tags/\" are often going to point\n+\t\t\t\t * to annotated tags, which are not part of the\n+\t\t\t\t * commit-graph. We thus only try to look up refs in\n+\t\t\t\t * the graph which are not in that namespace to not\n+\t\t\t\t * regress performance in repositories with many\n+\t\t\t\t * annotated tags.\n+\t\t\t\t */\n+\t\t\t\tif (!starts_with(rm->name, \"refs/tags/\"))\n+\t\t\t\t\tcommit = lookup_commit_in_graph(the_repository, &rm->old_oid);\n+\t\t\t\tif (!commit) {\n+\t\t\t\t\tcommit = lookup_commit_reference_gently(the_repository,\n+\t\t\t\t\t\t\t\t\t\t&rm->old_oid,\n+\t\t\t\t\t\t\t\t\t\t1);\n+\t\t\t\t\tif (!commit)\n+\t\t\t\t\t\trm->fetch_head_status = FETCH_HEAD_NOT_FOR_MERGE;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tif (rm->fetch_head_status != want_status)\n-- \n2.35.1\n\n"},{"id":"449893","messageId":"29eb81d37c8fef63858d8e066795e308692d219c.1646127015.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1646127015.git.ps@pks.im","subject":"[PATCH v2 3/5] refs: add ability for backends to special-case reading of symbolic refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T09:33:46Z","receivedAt":"2022-03-01T09:34:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Reading of symbolic and non-symbolic references is currently treated the\nsame in reference backends: we always call `refs_read_raw_ref()` and\nthen decide based on the returned flags what type it is. This has one\ndownside though: symbolic references may be treated different from\nnormal references in a backend from normal references. The packed-refs\nbackend for example doesn't even know about symbolic references, and as\na result it is pointless to even ask it for one.\n\nThere are cases where we really only care about whether a reference is\nsymbolic or not, but don't care about whether it exists at all or may be\na non-symbolic reference. But it is not possible to optimize for this\ncase right now, and as a consequence we will always first check for a\nloose reference to exist, and if it doesn't, we'll query the packed-refs\nbackend for a known-to-not-be-symbolic reference. This is inefficient\nand requires us to search all packed references even though we know to\nnot care for the result at all.\n\nIntroduce a new function `refs_read_symbolic_ref()` which allows us to\nfix this case. This function will only ever return symbolic references\nand can thus optimize for the scenario layed out above. By default, if\nthe backend doesn't provide an implementation for it, we just use the\nold code path and fall back to `read_raw_ref()`. But in case the backend\nprovides its own, more efficient implementation, we will use that one\ninstead.\n\nNote that this function is explicitly designed to not distinguish\nbetween missing references and non-symbolic references. If it did, we'd\nbe forced to always search the packed-refs backend to see whether the\nsymbolic reference the user asked for really doesn't exist, or if it\nexists as a non-symbolic reference.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 17 +++++++++++++++++\n refs.h | 3 +++\n refs/debug.c | 1 +\n refs/files-backend.c | 1 +\n refs/packed-backend.c | 1 +\n refs/refs-internal.h | 16 ++++++++++++++++\n 6 files changed, 39 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 1598fb13e2..0b79bdd7c3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1673,6 +1673,23 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t\t\t\t type, failure_errno);\n }\n \n+int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,\n+\t\t\t struct strbuf *referent)\n+{\n+\tstruct object_id oid;\n+\tint ret, failure_errno = 0;\n+\tunsigned int type = 0;\n+\n+\tif (ref_store->be->read_symbolic_ref)\n+\t\treturn ref_store->be->read_symbolic_ref(ref_store, refname, referent);\n+\n+\tret = refs_read_raw_ref(ref_store, refname, &oid, referent, &type, &failure_errno);\n+\tif (ret || !(type & REF_ISSYMREF))\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n const char *refs_resolve_ref_unsafe(struct ref_store *refs,\n \t\t\t\t const char *refname,\n \t\t\t\t int resolve_flags,\ndiff --git a/refs.h b/refs.h\nindex 1ae12c410a..23479c7ee0 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -82,6 +82,9 @@ int read_ref_full(const char *refname, int resolve_flags,\n \t\t struct object_id *oid, int *flags);\n int read_ref(const char *refname, struct object_id *oid);\n \n+int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,\n+\t\t\t struct strbuf *referent);\n+\n /*\n * Return 0 if a reference named refname could be created without\n * conflicting with the name of an existing reference. Otherwise,\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 2b0771ca53..c590d37720 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -435,6 +435,7 @@ struct ref_storage_be refs_be_debug = {\n \n \tdebug_ref_iterator_begin,\n \tdebug_read_raw_ref,\n+\tNULL,\n \n \tdebug_reflog_iterator_begin,\n \tdebug_for_each_reflog_ent,\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f59589d6cc..f3428a9f12 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3286,6 +3286,7 @@ struct ref_storage_be refs_be_files = {\n \n \tfiles_ref_iterator_begin,\n \tfiles_read_raw_ref,\n+\tNULL,\n \n \tfiles_reflog_iterator_begin,\n \tfiles_for_each_reflog_ent,\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 27dd8c3922..f56e2cc635 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1684,6 +1684,7 @@ struct ref_storage_be refs_be_packed = {\n \n \tpacked_ref_iterator_begin,\n \tpacked_read_raw_ref,\n+\tNULL,\n \n \tpacked_reflog_iterator_begin,\n \tpacked_for_each_reflog_ent,\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 6e15db3ca4..001ef15835 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -649,6 +649,21 @@ typedef int read_raw_ref_fn(struct ref_store *ref_store, const char *refname,\n \t\t\t struct object_id *oid, struct strbuf *referent,\n \t\t\t unsigned int *type, int *failure_errno);\n \n+/*\n+ * Read a symbolic reference from the specified reference store. This function\n+ * is optional: if not implemented by a backend, then `read_raw_ref_fn` is used\n+ * to read the symbolcic reference instead. It is intended to be implemented\n+ * only in case the backend can optimize the reading of symbolic references.\n+ *\n+ * Return 0 on success, or -1 on failure. `referent` will be set to the target\n+ * of the symbolic reference on success. This function explicitly does not\n+ * distinguish between error cases and the reference not being a symbolic\n+ * reference to allow backends to optimize this operation in case symbolic and\n+ * non-symbolic references are treated differently.\n+ */\n+typedef int read_symbolic_ref_fn(struct ref_store *ref_store, const char *refname,\n+\t\t\t\t struct strbuf *referent);\n+\n struct ref_storage_be {\n \tstruct ref_storage_be *next;\n \tconst char *name;\n@@ -668,6 +683,7 @@ struct ref_storage_be {\n \n \tref_iterator_begin_fn *iterator_begin;\n \tread_raw_ref_fn *read_raw_ref;\n+\tread_symbolic_ref_fn *read_symbolic_ref;\n \n \treflog_iterator_begin_fn *reflog_iterator_begin;\n \tfor_each_reflog_ent_fn *for_each_reflog_ent;\n-- \n2.35.1\n\n"},{"id":"449894","messageId":"0489380e00357dcdd14e399a88d420eab66b19bb.1646127015.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1646127015.git.ps@pks.im","subject":"[PATCH v2 4/5] remote: read symbolic refs via `refs_read_symbolic_ref()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T09:33:50Z","receivedAt":"2022-03-01T09:34:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"We have two cases in the remote code where we check whether a reference\nis symbolic or not, but don't mind in case it doesn't exist or in case\nit exists but is a non-symbolic reference. Convert these two callsites\nto use the new `refs_read_symbolic_ref()` function, whose intent is to\nimplement exactly that usecase.\n\nNo change in behaviour is expected from this change.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/remote.c | 8 +++++---\n remote.c | 14 +++++++-------\n 2 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 6f27ddc47b..0142ed09b5 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -766,13 +766,15 @@ static int mv(int argc, const char **argv)\n \tfor_each_ref(read_remote_branches, &rename);\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n-\t\tint flag = 0;\n+\t\tstruct strbuf referent = STRBUF_INIT;\n \n-\t\tread_ref_full(item->string, RESOLVE_REF_READING, NULL, &flag);\n-\t\tif (!(flag & REF_ISSYMREF))\n+\t\tif (refs_read_symbolic_ref(get_main_ref_store(the_repository), item->string,\n+\t\t\t\t\t &referent))\n \t\t\tcontinue;\n \t\tif (delete_ref(NULL, item->string, NULL, REF_NO_DEREF))\n \t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n+\n+\t\tstrbuf_release(&referent);\n \t}\n \tfor (i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\ndiff --git a/remote.c b/remote.c\nindex c97c626eaa..42a4e7106e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1945,13 +1945,9 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err)\n \treturn branch->push_tracking_ref;\n }\n \n-static int ignore_symref_update(const char *refname)\n+static int ignore_symref_update(const char *refname, struct strbuf *scratch)\n {\n-\tint flag;\n-\n-\tif (!resolve_ref_unsafe(refname, 0, NULL, &flag))\n-\t\treturn 0; /* non-existing refs are OK */\n-\treturn (flag & REF_ISSYMREF);\n+\treturn !refs_read_symbolic_ref(get_main_ref_store(the_repository), refname, scratch);\n }\n \n /*\n@@ -1964,6 +1960,7 @@ static int ignore_symref_update(const char *refname)\n static struct ref *get_expanded_map(const struct ref *remote_refs,\n \t\t\t\t const struct refspec_item *refspec)\n {\n+\tstruct strbuf scratch = STRBUF_INIT;\n \tconst struct ref *ref;\n \tstruct ref *ret = NULL;\n \tstruct ref **tail = &ret;\n@@ -1971,11 +1968,13 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n \tfor (ref = remote_refs; ref; ref = ref->next) {\n \t\tchar *expn_name = NULL;\n \n+\t\tstrbuf_reset(&scratch);\n+\n \t\tif (strchr(ref->name, '^'))\n \t\t\tcontinue; /* a dereference item */\n \t\tif (match_name_with_pattern(refspec->src, ref->name,\n \t\t\t\t\t refspec->dst, &expn_name) &&\n-\t\t !ignore_symref_update(expn_name)) {\n+\t\t !ignore_symref_update(expn_name, &scratch)) {\n \t\t\tstruct ref *cpy = copy_ref(ref);\n \n \t\t\tcpy->peer_ref = alloc_ref(expn_name);\n@@ -1987,6 +1986,7 @@ static struct ref *get_expanded_map(const struct ref *remote_refs,\n \t\tfree(expn_name);\n \t}\n \n+\tstrbuf_release(&scratch);\n \treturn ret;\n }\n \n-- \n2.35.1\n\n"},{"id":"449895","messageId":"b6eca63d3b9d9ff60778d6dabe13e4d087f2ff63.1646127015.git.ps@pks.im","threadId":"57461","inReplyTo":"cover.1646127015.git.ps@pks.im","subject":"[PATCH v2 5/5] refs/files-backend: optimize reading of symbolic refs","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2022-03-01T09:33:54Z","receivedAt":"2022-03-01T09:34:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When reading references via `files_read_raw_ref()` we always consult\nboth the loose reference, and if that wasn't found, we also consult the\npacked-refs file. While this makes sense to read a generic reference, it\nis wasteful in the case where we only care about symbolic references:\nthe packed-refs backend does not support them, and thus it cannot ever\nreturn one for us.\n\nSpecial-case reading of symbolic references for the files backend such\nthat we always skip asking the packed-refs backend.\n\nWe use `refs_read_symbolic_ref()` extensively to determine whether we\nneed to skip updating local symbolic references during a fetch, which is\nwhy the change results in a significant speedup when doing fetches in\nrepositories with huge numbers of references. The following benchmark\nexecutes a mirror-fetch in a repository with about 2 million references\nvia `git fetch --prune --no-write-fetch-head +refs/*:refs/*`:\n\n Benchmark 1: HEAD~\n Time (mean ± σ): 68.372 s ± 2.344 s [User: 65.629 s, System: 8.786 s]\n Range (min … max): 65.745 s … 70.246 s 3 runs\n\n Benchmark 2: HEAD\n Time (mean ± σ): 60.259 s ± 0.343 s [User: 61.019 s, System: 7.245 s]\n Range (min … max): 60.003 s … 60.649 s 3 runs\n\n Summary\n 'HEAD' ran\n 1.13 ± 0.04 times faster than 'HEAD~'\n\nSigned-off-by: Patrick Steinhardt \n---\n refs/files-backend.c | 34 ++++++++++++++++++++++++++++------\n 1 file changed, 28 insertions(+), 6 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f3428a9f12..0457ecdb42 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -338,9 +338,9 @@ static struct ref_cache *get_loose_ref_cache(struct files_ref_store *refs)\n \treturn refs->loose;\n }\n \n-static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n-\t\t\t struct object_id *oid, struct strbuf *referent,\n-\t\t\t unsigned int *type, int *failure_errno)\n+static int read_ref_internal(struct ref_store *ref_store, const char *refname,\n+\t\t\t struct object_id *oid, struct strbuf *referent,\n+\t\t\t unsigned int *type, int *failure_errno, int skip_packed_refs)\n {\n \tstruct files_ref_store *refs =\n \t\tfiles_downcast(ref_store, REF_STORE_READ, \"read_raw_ref\");\n@@ -381,7 +381,7 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \tif (lstat(path, &st) < 0) {\n \t\tint ignore_errno;\n \t\tmyerr = errno;\n-\t\tif (myerr != ENOENT)\n+\t\tif (myerr != ENOENT || skip_packed_refs)\n \t\t\tgoto out;\n \t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n \t\t\t\t referent, type, &ignore_errno)) {\n@@ -425,7 +425,8 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t * ref is supposed to be, there could still be a\n \t\t * packed ref:\n \t\t */\n-\t\tif (refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n+\t\tif (skip_packed_refs ||\n+\t\t refs_read_raw_ref(refs->packed_ref_store, refname, oid,\n \t\t\t\t referent, type, &ignore_errno)) {\n \t\t\tmyerr = EISDIR;\n \t\t\tgoto out;\n@@ -470,6 +471,27 @@ static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \treturn ret;\n }\n \n+static int files_read_raw_ref(struct ref_store *ref_store, const char *refname,\n+\t\t\t struct object_id *oid, struct strbuf *referent,\n+\t\t\t unsigned int *type, int *failure_errno)\n+{\n+\treturn read_ref_internal(ref_store, refname, oid, referent, type, failure_errno, 0);\n+}\n+\n+static int files_read_symbolic_ref(struct ref_store *ref_store, const char *refname,\n+\t\t\t\t struct strbuf *referent)\n+{\n+\tstruct object_id oid;\n+\tint failure_errno, ret;\n+\tunsigned int type;\n+\n+\tret = read_ref_internal(ref_store, refname, &oid, referent, &type, &failure_errno, 1);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\treturn !(type & REF_ISSYMREF);\n+}\n+\n int parse_loose_ref_contents(const char *buf, struct object_id *oid,\n \t\t\t struct strbuf *referent, unsigned int *type,\n \t\t\t int *failure_errno)\n@@ -3286,7 +3308,7 @@ struct ref_storage_be refs_be_files = {\n \n \tfiles_ref_iterator_begin,\n \tfiles_read_raw_ref,\n-\tNULL,\n+\tfiles_read_symbolic_ref,\n \n \tfiles_reflog_iterator_begin,\n \tfiles_for_each_reflog_ent,\n-- \n2.35.1\n\n"},{"id":"450022","messageId":"xmqqv8wxnxf6.fsf@gitster.g","threadId":"57461","inReplyTo":"cover.1646127015.git.ps@pks.im","subject":"Re: [PATCH v2 0/5] fetch: more optimizations for mirror fetches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-01T22:02:53Z","receivedAt":"2022-03-01T22:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> this is another patch series with the aim to speed up mirror fetches. It\n> applies on top of e6ebfd0e8c (The sixth batch, 2022-02-18) with\n> 3824153b23 (Merge branch 'ps/fetch-atomic' into next, 2022-02-18) merged\n> into it to fix a conflict.\n\nThanks for clearly describing a base. Except that a merge on 'next'\n(e.g. 3824153b23) is not what we want a new topic to depend on; use\nthe tip(s) of individual topic(s), i.e. 583bc419 (fetch: make\n`--atomic` flag cover pruning of refs, 2022-02-17), instead.\n\n"},{"id":"450147","messageId":"9a3b8e63-6b61-5e7a-ec71-3777f5bc45f7@github.com","threadId":"57461","inReplyTo":"Yh3mR3/h/+nFyF6V@ncase","subject":"Re: [PATCH 1/5] upload-pack: look up \"want\" lines via commit-graph","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-02T18:53:10Z","receivedAt":"2022-03-02T18:53:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/1/2022 4:24 AM, Patrick Steinhardt wrote:\n> On Tue, Mar 01, 2022 at 09:43:19AM +0100, Patrick Steinhardt wrote:\n>> On Wed, Feb 23, 2022 at 09:13:53AM -0500, Derrick Stolee wrote:\n>>> On 2/23/2022 7:35 AM, Patrick Steinhardt wrote:\n\n>>>> -\t\to = parse_object(the_repository, &oid);\n>>>> +\t\tcommit = lookup_commit_in_graph(the_repository, &oid);\n>>>> +\t\tif (commit)\n>>>> +\t\t\to = &commit->object;\n>>>> +\t\telse\n>>>> +\t\t\to = parse_object(the_repository, &oid);\n>>>> +\n>>>\n>>> This is a neat trick. I see that we've also done this trick in\n>>> revision.c:get_reference(). Perhaps it is worth creating a helper,\n>>> maybe named parse_probably_commit()?\n>>\n>> That might be a good idea, thanks. I'll have a look at what the end\n>> result would look like.\n>>\n>> Patrick\n> \n> I had a look at existing callsites which use `lookup_commit_in_graph()`,\n> but I found that it wasn't easily possible to convert them all to use a\n> new helper like you propose. Most of them have some custom logic like\n> skipping `parse_object()` if it's part of a promisor pack, so I really\n> only found two locations where such a new helper could be used without\n> also adding and supporting flags. I don't really think that's worth it\n> for now.\n\nThanks for taking the time to look into it.\n\n-Stolee\n\n"},{"id":"450149","messageId":"49c21bac-0543-7c3e-33f2-d36cdac87362@github.com","threadId":"57461","inReplyTo":"cover.1646127015.git.ps@pks.im","subject":"Re: [PATCH v2 0/5] fetch: more optimizations for mirror fetches","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-03-02T18:54:34Z","receivedAt":"2022-03-02T18:54:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/1/2022 4:33 AM, Patrick Steinhardt wrote:\n> Hi,\n> \n> this is another patch series with the aim to speed up mirror fetches. It\n> applies on top of e6ebfd0e8c (The sixth batch, 2022-02-18) with\n> 3824153b23 (Merge branch 'ps/fetch-atomic' into next, 2022-02-18) merged\n> into it to fix a conflict.\n> \n> The only change compared to v2 is an update to the benchmarks so that\n> they're less verbose, as proposed by Derrick. I also had a look at\n> introducing a new helper `parse_object_probably_commit()`, but I didn't\n> find the end result to be much of an improvement compared to the ad-hoc\n> `lookup_commit_in_graph() || parse_object()` dance we do right now.\n\nI'm satisfied that you tried the helper idea. This version\nlooks good to me.\n\nThanks,\n-Stolee\n"}]}