{"thread":{"id":"52617","subject":"[PATCH 0/2] Skip a connectivity check during fetch --filter","startedAt":"2020-01-12T04:15:34Z","lastAt":"2020-01-30T18:57:35Z","messageCount":6,"participants":["Jonathan Tan","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"389622","messageId":"cover.1578802317.git.jonathantanmy@google.com","threadId":"52617","inReplyTo":null,"subject":"[PATCH 0/2] Skip a connectivity check during fetch --filter","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-12T04:15:23Z","receivedAt":"2020-01-12T04:15:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The same optimization in dfa33a298d (\"clone: do faster object check for\npartial clones\", 2019-04-21) can be applied to fetch as well, so this\npatch set does so. Patch 1 makes the check more robust, and patch 2\napplies it to one of two connectivity checks performed during the fetch.\n\nAs mentioned in patch 2, when fetching from a local repo, I got a\nspeedup of 6.63s to 3.39s. \n\nJonathan Tan (2):\n  connected: verify promisor-ness of partial clone\n  fetch: forgo full connectivity check if --filter\n\n builtin/clone.c |  5 +++--\n builtin/fetch.c | 11 ++++++++++-\n connected.c     | 19 ++++++++++++++-----\n connected.h     | 11 ++++++-----\n 4 files changed, 33 insertions(+), 13 deletions(-)\n\n-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"389623","messageId":"6a4f704e475fe1669e63731333fce9ed09d17d0c.1578802317.git.jonathantanmy@google.com","threadId":"52617","inReplyTo":"cover.1578802317.git.jonathantanmy@google.com","subject":"[PATCH 1/2] connected: verify promisor-ness of partial clone","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-12T04:15:24Z","receivedAt":"2020-01-12T04:15:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit dfa33a298d (\"clone: do faster object check for partial clones\",\n2019-04-21) optimized the connectivity check done when cloning with\n--filter to check only the existence of objects directly pointed to by\nrefs. But this is not sufficient: they also need to be promisor objects.\nMake this check more robust by instead checking that these objects are\npromisor objects, that is, they appear in a promisor pack.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/clone.c |  5 +++--\n connected.c     | 19 ++++++++++++++-----\n connected.h     | 11 ++++++-----\n 3 files changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 0fc89ae2b9..0516181052 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -673,7 +673,7 @@ static void update_remote_refs(const struct ref *refs,\n \t\t\t       const char *msg,\n \t\t\t       struct transport *transport,\n \t\t\t       int check_connectivity,\n-\t\t\t       int check_refs_only)\n+\t\t\t       int check_refs_are_promisor_objects_only)\n {\n \tconst struct ref *rm = mapped_refs;\n \n@@ -682,7 +682,8 @@ static void update_remote_refs(const struct ref *refs,\n \n \t\topt.transport = transport;\n \t\topt.progress = transport->progress;\n-\t\topt.check_refs_only = !!check_refs_only;\n+\t\topt.check_refs_are_promisor_objects_only =\n+\t\t\t!!check_refs_are_promisor_objects_only;\n \n \t\tif (check_connected(iterate_ref_map, &rm, &opt))\n \t\t\tdie(_(\"remote did not send all necessary objects\"));\ndiff --git a/connected.c b/connected.c\nindex c337f5f7f4..7e9bd1bc62 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -52,19 +52,28 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\tstrbuf_release(&idx_file);\n \t}\n \n-\tif (opt->check_refs_only) {\n+\tif (opt->check_refs_are_promisor_objects_only) {\n \t\t/*\n \t\t * For partial clones, we don't want to have to do a regular\n \t\t * connectivity check because we have to enumerate and exclude\n \t\t * all promisor objects (slow), and then the connectivity check\n \t\t * itself becomes a no-op because in a partial clone every\n \t\t * object is a promisor object. Instead, just make sure we\n-\t\t * received the objects pointed to by each wanted ref.\n+\t\t * received, in a promisor packfile, the objects pointed to by\n+\t\t * each wanted ref.\n \t\t */\n \t\tdo {\n-\t\t\tif (!repo_has_object_file_with_flags(the_repository, &oid,\n-\t\t\t\t\t\t\t     OBJECT_INFO_SKIP_FETCH_OBJECT))\n-\t\t\t\treturn 1;\n+\t\t\tstruct packed_git *p;\n+\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\t\tgoto promisor_pack_found;\n+\t\t\t}\n+\t\t\treturn 1;\n+promisor_pack_found:\n+\t\t\t;\n \t\t} while (!fn(cb_data, &oid));\n \t\treturn 0;\n \t}\ndiff --git a/connected.h b/connected.h\nindex ce2e7d8f2e..eba5c261ba 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -48,12 +48,13 @@ struct check_connected_options {\n \tunsigned is_deepening_fetch : 1;\n \n \t/*\n-\t * If non-zero, only check the top-level objects referenced by the\n-\t * wanted refs (passed in as cb_data). This is useful for partial\n-\t * clones, where enumerating and excluding all promisor objects is very\n-\t * slow and the commit-walk itself becomes a no-op.\n+\t * If non-zero, only check that the top-level objects referenced by the\n+\t * wanted refs (passed in as cb_data) are promisor objects. This is\n+\t * useful for partial clones, where enumerating and excluding all\n+\t * promisor objects is very slow and the commit-walk itself becomes a\n+\t * no-op.\n \t */\n-\tunsigned check_refs_only : 1;\n+\tunsigned check_refs_are_promisor_objects_only : 1;\n };\n \n #define CHECK_CONNECTED_INIT { 0 }\n-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"389624","messageId":"be1d6aa4c4fd8868f3682b73c01a92d3830534ad.1578802317.git.jonathantanmy@google.com","threadId":"52617","inReplyTo":"cover.1578802317.git.jonathantanmy@google.com","subject":"[PATCH 2/2] fetch: forgo full connectivity check if --filter","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-01-12T04:15:25Z","receivedAt":"2020-01-12T04:15:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"If a filter is specified, we do not need a full connectivity check on\nthe contents of the packfile we just fetched; we only need to check that\nthe objects referenced are promisor objects.\n\nThis significantly speeds up fetches into repositories that have many\npromisor objects, because during the connectivity check, all promisor\nobjects are enumerated (to mark them UNINTERESTING), and that takes a\nsignificant amount of time.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nFor example, a local fetch was sped up from 6.63s to 3.39s. The bulk of\nthe remaining time is spent in yet another connectivity check\n(fetch_refs -> check_exist_and_connected) prior to the fetch - that will\nhopefully be done in a subsequent patch.\n---\n builtin/fetch.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex b4c6d921d0..6fb50320eb 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -906,8 +906,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\turl = xstrdup(\"foreign\");\n \n \tif (!connectivity_checked) {\n+\t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n+\n+\t\tif (filter_options.choice)\n+\t\t\t/*\n+\t\t\t * Since a filter is specified, objects indirectly\n+\t\t\t * referenced by refs are allowed to be absent.\n+\t\t\t */\n+\t\t\topt.check_refs_are_promisor_objects_only = 1;\n+\n \t\trm = ref_map;\n-\t\tif (check_connected(iterate_ref_map, &rm, NULL)) {\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-- \n2.25.0.rc1.283.g88dfdc4193-goog\n\n"},{"id":"390744","messageId":"20200129204152.GA17350@google.com","threadId":"52617","inReplyTo":"6a4f704e475fe1669e63731333fce9ed09d17d0c.1578802317.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] connected: verify promisor-ness of partial clone","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-29T20:41:52Z","receivedAt":"2020-01-29T20:41:56Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> Commit dfa33a298d (\"clone: do faster object check for partial clones\",\n> 2019-04-21) optimized the connectivity check done when cloning with\n> --filter to check only the existence of objects directly pointed to by\n> refs. But this is not sufficient: they also need to be promisor objects.\n> Make this check more robust by instead checking that these objects are\n> promisor objects, that is, they appear in a promisor pack.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  builtin/clone.c |  5 +++--\n>  connected.c     | 19 ++++++++++++++-----\n>  connected.h     | 11 ++++++-----\n>  3 files changed, 23 insertions(+), 12 deletions(-)\n\nGood call.  Sorry for the slow review.\n\n[...]\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -52,19 +52,28 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \t\tstrbuf_release(&idx_file);\n>  \t}\n>  \n> -\tif (opt->check_refs_only) {\n> +\tif (opt->check_refs_are_promisor_objects_only) {\n>  \t\t/*\n>  \t\t * For partial clones, we don't want to have to do a regular\n>  \t\t * connectivity check because we have to enumerate and exclude\n>  \t\t * all promisor objects (slow), and then the connectivity check\n>  \t\t * itself becomes a no-op because in a partial clone every\n>  \t\t * object is a promisor object. Instead, just make sure we\n> -\t\t * received the objects pointed to by each wanted ref.\n> +\t\t * received, in a promisor packfile, the objects pointed to by\n> +\t\t * each wanted ref.\n>  \t\t */\n>  \t\tdo {\n> -\t\t\tif (!repo_has_object_file_with_flags(the_repository, &oid,\n> -\t\t\t\t\t\t\t     OBJECT_INFO_SKIP_FETCH_OBJECT))\n> -\t\t\t\treturn 1;\n> +\t\t\tstruct packed_git *p;\n> +\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\t\tgoto promisor_pack_found;\n> +\t\t\t}\n> +\t\t\treturn 1;\n> +promisor_pack_found:\n> +\t\t\t;\n>  \t\t} while (!fn(cb_data, &oid));\n>  \t\treturn 0;\n\nYep, does what it says on the tin.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"390746","messageId":"20200129204326.GB17350@google.com","threadId":"52617","inReplyTo":"be1d6aa4c4fd8868f3682b73c01a92d3830534ad.1578802317.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch: forgo full connectivity check if --filter","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-01-29T20:43:26Z","receivedAt":"2020-01-29T20:43:30Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Tan wrote:\n\n> If a filter is specified, we do not need a full connectivity check on\n> the contents of the packfile we just fetched; we only need to check that\n> the objects referenced are promisor objects.\n>\n> This significantly speeds up fetches into repositories that have many\n> promisor objects, because during the connectivity check, all promisor\n> objects are enumerated (to mark them UNINTERESTING), and that takes a\n> significant amount of time.\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n> For example, a local fetch was sped up from 6.63s to 3.39s. The bulk of\n> the remaining time is spent in yet another connectivity check\n> (fetch_refs -> check_exist_and_connected) prior to the fetch - that will\n> hopefully be done in a subsequent patch.\n\nCan this information (at least the speedup) be included in the comment\nmessage?\n\nOr even better, can we demonstrate the impact using a perf test?\n\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -906,8 +906,17 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n>  \t\turl = xstrdup(\"foreign\");\n>  \n>  \tif (!connectivity_checked) {\n> +\t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n> +\n> +\t\tif (filter_options.choice)\n> +\t\t\t/*\n> +\t\t\t * Since a filter is specified, objects indirectly\n> +\t\t\t * referenced by refs are allowed to be absent.\n> +\t\t\t */\n> +\t\t\topt.check_refs_are_promisor_objects_only = 1;\n> +\n>  \t\trm = ref_map;\n> -\t\tif (check_connected(iterate_ref_map, &rm, NULL)) {\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\nSimple and sensible.  With or without a change like described above,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n"},{"id":"390828","messageId":"xmqqimksdb6w.fsf@gitster-ct.c.googlers.com","threadId":"52617","inReplyTo":"20200129204326.GB17350@google.com","subject":"Re: [PATCH 2/2] fetch: forgo full connectivity check if --filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-30T18:57:27Z","receivedAt":"2020-01-30T18:57:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Jonathan Tan wrote:\n>\n>> If a filter is specified, we do not need a full connectivity check on\n>> the contents of the packfile we just fetched; we only need to check that\n>> the objects referenced are promisor objects.\n>>\n>> This significantly speeds up fetches into repositories that have many\n>> promisor objects, because during the connectivity check, all promisor\n>> objects are enumerated (to mark them UNINTERESTING), and that takes a\n>> significant amount of time.\n>>\n>> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n>> ---\n>> For example, a local fetch was sped up from 6.63s to 3.39s. The bulk of\n>> the remaining time is spent in yet another connectivity check\n>> (fetch_refs -> check_exist_and_connected) prior to the fetch - that will\n>> hopefully be done in a subsequent patch.\n>\n> Can this information (at least the speedup) be included in the comment\n> message?\n>\n> Or even better, can we demonstrate the impact using a perf test?\n\nIt does make sense, but let's queue these two first and then add it\nas a follow-up patch on top.\n\nThanks for writing and reviewing.\n"}]}