{"thread":{"id":"63538","subject":"[PATCH] promisor-remote: remove the promisor object check for failed fetch","startedAt":"2025-05-28T09:58:49Z","lastAt":"2025-05-30T13:26:22Z","messageCount":3,"participants":["Han Young","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"519069","messageId":"20250528095830.30306-1-hanyang.tony@bytedance.com","threadId":"63538","inReplyTo":null,"subject":"[PATCH] promisor-remote: remove the promisor object check for failed fetch","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-05-28T09:58:30Z","receivedAt":"2025-05-28T09:58:49Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"If the promisor objects fail to fetch, we check the remaining objects\nto see if they are indeed promisor objects. Then, we die on the first\nremaining promisor object. However, this promisor object check is \nunnecessary because callers of promisor_remote_get_direct already filter\nout local objects. All objects passed to promisor_remote_get_direct are\npromisor objects.\n\nThe is_promisor_object check essentially iterates through every object\nin the local packfiles and adds them to an oid set. This process is\nagonizingly slow for large repositories.\nRemove the check so that we fail immediately.\n\nSigned-off-by: Han Young <hanyang.tony@bytedance.com>\n---\n promisor-remote.c | 8 +++-----\n 1 file changed, 3 insertions(+), 5 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9d058586df..f42ea4ce78 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -275,7 +275,6 @@ void promisor_remote_get_direct(struct repository *repo,\n \tstruct object_id *remaining_oids = (struct object_id *)oids;\n \tint remaining_nr = oid_nr;\n \tint to_free = 0;\n-\tint i;\n \n \tif (oid_nr == 0)\n \t\treturn;\n@@ -296,10 +295,9 @@ void promisor_remote_get_direct(struct repository *repo,\n \t\tgoto all_fetched;\n \t}\n \n-\tfor (i = 0; i < remaining_nr; i++) {\n-\t\tif (is_promisor_object(repo, &remaining_oids[i]))\n-\t\t\tdie(_(\"could not fetch %s from promisor remote\"),\n-\t\t\t    oid_to_hex(&remaining_oids[i]));\n+\tif (remaining_nr) {\n+\t\tdie(_(\"could not fetch %s from promisor remote\"),\n+\t\t\toid_to_hex(&remaining_oids[0]));\n \t}\n \n all_fetched:\n-- \n2.48.1\n\n"},{"id":"519144","messageId":"xmqqplfrmoey.fsf@gitster.g","threadId":"63538","inReplyTo":"20250528095830.30306-1-hanyang.tony@bytedance.com","subject":"Re: [PATCH] promisor-remote: remove the promisor object check for failed fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-29T15:23:17Z","receivedAt":"2025-05-29T15:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Han Young <hanyang.tony@bytedance.com> writes:\n\n> If the promisor objects fail to fetch, we check the remaining objects\n> to see if they are indeed promisor objects. Then, we die on the first\n> remaining promisor object. However, this promisor object check is \n> unnecessary because callers of promisor_remote_get_direct already filter\n> out local objects. All objects passed to promisor_remote_get_direct are\n> promisor objects.\n>\n> The is_promisor_object check essentially iterates through every object\n> in the local packfiles and adds them to an oid set. This process is\n> agonizingly slow for large repositories.\n> Remove the check so that we fail immediately.\n\nThanks for CC'ing Christian and Jonathan Tan, who may indeed be good\nreviewers for this change.\n\nlet me think aloud a bit, but because I haven't looked at this part\nof the system for quite some time, what I mumble may not make much\nsense.  Please bear with me.\n\nSo the incoming list remaining_oids[] is what was already filtered\nby the caller and everything in it should be promisor?  If so ...\n\n> -\tfor (i = 0; i < remaining_nr; i++) {\n> -\t\tif (is_promisor_object(repo, &remaining_oids[i]))\n> -\t\t\tdie(_(\"could not fetch %s from promisor remote\"),\n> -\t\t\t    oid_to_hex(&remaining_oids[i]));\n\n... the first iteration of this loop, after is_promisor_object()\nsays the object is a promisor object (which is guaranteed if\neverybody in that remaining_oids[] array is promisor object), will\ndie immediately.  So your \"agonizingly slow\" comes from just one\nsingle call to is_promisor_object() function is ultra slow?\n\nIf that is the case, the patch, including the above explanation,\nmakes perfect sense (note that I didn't verify the \"everybody in the\nremaining_oids[] array is guaranteed to be a promisor object\" claim\nmyself, though).\n\nBut at the same time, it sounds like is_promisor_object() seriously\nis wrong.  Perhaps we need to tell pack-objects to pre-compute the\npackfile.c:add_promisor_object() stuff and cache the result in an\non-disk file, just like reverse index is stored in an auxiliary\nfile?\n\nWhat do the callers of the function use to \"filter out local\nobjects\" to ensure that \"all objects passed ... are promisor\nobjects\" do?  Have they already spent agonizingly large amount of\ntime to do so?  I sampled a few existing callers but they do not\nexplicitly restrict what they throw at the function to promisor\nobjects---they prepare an oid array, throw anything they do not see\nlocally in the array and call this function.  So objects in the\narray are either promisor objects or missing due to repository\ncorruption---we simply cannot tell.  I suspect that the claim\n\"everything the caller calls the function with is a promisor object\"\nis not exactly correct.\n\nThe end-result when remaining_nr is not zero (i.e., some objects\nthat the caller wanted us to be fetched) would not exactly be the\nsame.  The function used to die only when the object we failed to\nobtain was what a promisor remote promised to give us.  With this\nchange, we also die when an object the caller asked us to fetch is\nnot promised by any promisor.  I do not know what the implication\nof this behaviour change would be.  Reviewers, comments?\n\n> +\tif (remaining_nr) {\n> +\t\tdie(_(\"could not fetch %s from promisor remote\"),\n> +\t\t\toid_to_hex(&remaining_oids[0]));\n>  \t}\n\n"},{"id":"519221","messageId":"CAG1j3zGhSQGii86Ysj3Wsuua2iwCUgyzg362NHutFT__F9dwdQ@mail.gmail.com","threadId":"63538","inReplyTo":"xmqqplfrmoey.fsf@gitster.g","subject":"Re: [External] Re: [PATCH] promisor-remote: remove the promisor object check for failed fetch","fromName":"Han Young","fromEmail":"hanyang.tony@bytedance.com","sentAt":"2025-05-30T13:26:10Z","receivedAt":"2025-05-30T13:26:22Z","isPatch":true,"sender":{"key":"hanyang.tony@bytedance.com","avatar":"https://avatars.githubusercontent.com/u/108711387?v=4"},"body":"On Thu, May 29, 2025 at 11:23 PM Junio C Hamano <gitster@pobox.com> wrote:\n> So your \"agonizingly slow\" comes from just one\n> single call to is_promisor_object() function is ultra slow?\n\nThat's correct, is_promisor_object() function constructs the promisor\nobjects oid set by iterating through every object in the promisor\npackfiles and calling add_promisor_object() function for each object.\nadd_promisor_object() function parses the object, adds itself and the\nobjects it refers to to the oid set. For a very large partial clone\nrepository, the promisor packfiles will contain millions of objects.\nParsing each one takes a considerable amount of time.\n\n> But at the same time, it sounds like is_promisor_object() seriously\n> is wrong.  Perhaps we need to tell pack-objects to pre-compute the\n> packfile.c:add_promisor_object() stuff and cache the result in an\n> on-disk file, just like reverse index is stored in an auxiliary\n> file?\n\nAs Calvin summarized in this thread [1], creating a promisor object set\nis very expensive. The fact that a local object can become a\n\"promisor object\" makes disk caching infeasible.\nis_promisor_object() function is not inherently wrong, it's the definition\nof \"promisor object\" that makes implementation difficult.\n\n> What do the callers of the function use to \"filter out local\n> objects\" to ensure that \"all objects passed ... are promisor\n> objects\" do?  Have they already spent agonizingly large amount of\n> time to do so?\n\nThey call oid_object_info_extended() function to check whether the object\nexists in the local storage. Only objects that do not exist locally are\nsent to promisor_remote_get_direct().\n\n> So objects in the\n> array are either promisor objects or missing due to repository\n> corruption---we simply cannot tell.  I suspect that the claim\n> \"everything the caller calls the function with is a promisor object\"\n> is not exactly correct.\n\nYou are right that objects that are not present in the local repository\ndo not equal promisor objects. The array could contain missing objects\nthat are not promisor objects.\n\n> The end-result when remaining_nr is not zero (i.e., some objects\n> that the caller wanted us to be fetched) would not exactly be the\n> same.  The function used to die only when the object we failed to\n> obtain was what a promisor remote promised to give us.  With this\n> change, we also die when an object the caller asked us to fetch is\n> not promised by any promisor.  I do not know what the implication\n> of this behaviour change would be.\n\nI tested this patch with git-cat-file and git-diff on a repository\nmissing a normal object. The commands fail regardless of the patch,\nbut the error message is different. The message changed from\nfatal: Not a valid object name\nto\nfatal: could not fetch ... from promisor remote\n\n[1] https://lore.kernel.org/git/CAFySSZCyoaKCGycYgJjCJGJ2mV1yfg+gVFb7RytGKmkjupkNkQ@mail.gmail.com/\n"}]}