{"thread":{"id":"51989","subject":"[PATCH] send-pack: never fetch when checking exclusions","startedAt":"2019-10-07T21:38:53Z","lastAt":"2019-10-17T06:10:42Z","messageCount":9,"participants":["Jonathan Tan","Junio C Hamano","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"383627","messageId":"20191007213839.233451-1-jonathantanmy@google.com","threadId":"51989","inReplyTo":null,"subject":"[PATCH] send-pack: never fetch when checking exclusions","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-07T21:38:39Z","receivedAt":"2019-10-07T21:38:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When building the packfile to be sent, send_pack() is given a list of\nremote refs to be used as exclusions. For each ref, it first checks if\nthe ref exists locally, and if it does, passes it with a \"^\" prefix to\npack-objects. However, in a partial clone, the check may trigger a lazy\nfetch. Ensure that this lazy fetch does not occur.\n\nSigned-off-by: Jonathan Tan \n---\n send-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 6dc16c3211..34c77cbb1a 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -40,7 +40,8 @@ int option_parse_push_signed(const struct option *opt,\n \n static void feed_object(const struct object_id *oid, FILE *fh, int negative)\n {\n-\tif (negative && !has_object_file(oid))\n+\tif (negative &&\n+\t !has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\treturn;\n \n \tif (negative)\n-- \n2.23.0.581.g78d2f28ef7-goog\n\n"},{"id":"383650","messageId":"xmqqzhibnahi.fsf@gitster-ct.c.googlers.com","threadId":"51989","inReplyTo":"20191007213839.233451-1-jonathantanmy@google.com","subject":"Re: [PATCH] send-pack: never fetch when checking exclusions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-08T04:10:17Z","receivedAt":"2019-10-08T04:10:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> When building the packfile to be sent, send_pack() is given a list of\n> remote refs to be used as exclusions. For each ref, it first checks if\n> the ref exists locally, and if it does, passes it with a \"^\" prefix to\n> pack-objects. However, in a partial clone, the check may trigger a lazy\n> fetch. Ensure that this lazy fetch does not occur.\n\nIs there any effect worth describing here, other than the obvious\n\"we do not lazily fetch from within the has_object_file() function\"?\n\nFor example, would this change mean that the resulting pack may\ninclude stuff that are reachable from the (missing) negative objects\nthat would not otherwise have to be sent if these objects were\navailable (or made available by the lazy fetching), and we are\nmaking a trade-off to send possibly more in order for not fetching?\nHave we laid enough on the table to help readers if such a trade-off\n(if we are making one, that is) strikes the right balance?\n\nThanks.\n\n> Signed-off-by: Jonathan Tan \n> ---\n> send-pack.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/send-pack.c b/send-pack.c\n> index 6dc16c3211..34c77cbb1a 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -40,7 +40,8 @@ int option_parse_push_signed(const struct option *opt,\n> \n> static void feed_object(const struct object_id *oid, FILE *fh, int negative)\n> {\n> -\tif (negative && !has_object_file(oid))\n> +\tif (negative &&\n> +\t !has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n> \t\treturn;\n> \n> \tif (negative)\n"},{"id":"383697","messageId":"20191008183739.194714-1-jonathantanmy@google.com","threadId":"51989","inReplyTo":"xmqqzhibnahi.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] send-pack: never fetch when checking exclusions","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-08T18:37:39Z","receivedAt":"2019-10-08T18:37:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When building the packfile to be sent, send_pack() is given a list of\nremote refs to be used as exclusions. For each ref, it first checks if\nthe ref exists locally, and if it does, passes it with a \"^\" prefix to\npack-objects. However, in a partial clone, the check may trigger a lazy\nfetch.\n\nThe additional commit ancestry information obtained during such fetches\nmay show that certain objects that would have been sent are already\nknown to the server, resulting in a smaller pack being sent. But this is\nat the cost of fetching from many possibly unrelated refs, and the lazy\nfetches do not help at all in the typical case where the client is\nup-to-date with the upstream of the branch being pushed.\n\nEnsure that these lazy fetches do not occur.\n\nSigned-off-by: Jonathan Tan \n---\n> For example, would this change mean that the resulting pack may\n> include stuff that are reachable from the (missing) negative objects\n> that would not otherwise have to be sent if these objects were\n> available (or made available by the lazy fetching), and we are\n> making a trade-off to send possibly more in order for not fetching?\n> Have we laid enough on the table to help readers if such a trade-off\n> (if we are making one, that is) strikes the right balance?\n\nThanks for your comments. I've expanded the commit message.\n---\n send-pack.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 6dc16c3211..34c77cbb1a 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -40,7 +40,8 @@ int option_parse_push_signed(const struct option *opt,\n \n static void feed_object(const struct object_id *oid, FILE *fh, int negative)\n {\n-\tif (negative && !has_object_file(oid))\n+\tif (negative &&\n+\t !has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\treturn;\n \n \tif (negative)\n-- \n2.23.0.581.g78d2f28ef7-goog\n\n"},{"id":"383877","messageId":"20191011061257.GD20094@sigill.intra.peff.net","threadId":"51989","inReplyTo":"20191008183739.194714-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] send-pack: never fetch when checking exclusions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-11T06:12:57Z","receivedAt":"2019-10-11T06:12:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 08, 2019 at 11:37:39AM -0700, Jonathan Tan wrote:\n\n> When building the packfile to be sent, send_pack() is given a list of\n> remote refs to be used as exclusions. For each ref, it first checks if\n> the ref exists locally, and if it does, passes it with a \"^\" prefix to\n> pack-objects. However, in a partial clone, the check may trigger a lazy\n> fetch.\n> \n> The additional commit ancestry information obtained during such fetches\n> may show that certain objects that would have been sent are already\n> known to the server, resulting in a smaller pack being sent. But this is\n> at the cost of fetching from many possibly unrelated refs, and the lazy\n> fetches do not help at all in the typical case where the client is\n> up-to-date with the upstream of the branch being pushed.\n> \n> Ensure that these lazy fetches do not occur.\n\nThat makes sense. For similar reasons, should we be using\nOBJECT_INFO_QUICK here? If the other side has a bunch of ref tips that\nwe don't have, we'll end up re-scanning the pack directory over and over\n(which is _usually_ pretty quick, but can be slow if you have a lot of\npacks locally). And it's OK if we racily miss out on an exclusion due to\nsomebody else repacking simultaneously.\n\n-Peff\n"},{"id":"383887","messageId":"a87cf3ce-fbff-ef4e-941e-bd2da0bf182f@gmail.com","threadId":"51989","inReplyTo":"20191011061257.GD20094@sigill.intra.peff.net","subject":"Re: [PATCH v2] send-pack: never fetch when checking exclusions","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-10-11T12:31:30Z","receivedAt":"2019-10-11T12:31:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/11/2019 2:12 AM, Jeff King wrote:\n> On Tue, Oct 08, 2019 at 11:37:39AM -0700, Jonathan Tan wrote:\n> \n>> When building the packfile to be sent, send_pack() is given a list of\n>> remote refs to be used as exclusions. For each ref, it first checks if\n>> the ref exists locally, and if it does, passes it with a \"^\" prefix to\n>> pack-objects. However, in a partial clone, the check may trigger a lazy\n>> fetch.\n>>\n>> The additional commit ancestry information obtained during such fetches\n>> may show that certain objects that would have been sent are already\n>> known to the server, resulting in a smaller pack being sent. But this is\n>> at the cost of fetching from many possibly unrelated refs, and the lazy\n>> fetches do not help at all in the typical case where the client is\n>> up-to-date with the upstream of the branch being pushed.\n>>\n>> Ensure that these lazy fetches do not occur.\n> \n> That makes sense. For similar reasons, should we be using\n> OBJECT_INFO_QUICK here? If the other side has a bunch of ref tips that\n> we don't have, we'll end up re-scanning the pack directory over and over\n> (which is _usually_ pretty quick, but can be slow if you have a lot of\n> packs locally). And it's OK if we racily miss out on an exclusion due to\n> somebody else repacking simultaneously.\n\nThat's a good idea. We can hint to the object store that we don't expect\nmisses to be due to a concurrent repack, so we don't want to reprepare\npack-files.\n\n-Stolee\n\n"},{"id":"383901","messageId":"20191011161504.GB19741@sigill.intra.peff.net","threadId":"51989","inReplyTo":"a87cf3ce-fbff-ef4e-941e-bd2da0bf182f@gmail.com","subject":"Re: [PATCH v2] send-pack: never fetch when checking exclusions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-11T16:15:05Z","receivedAt":"2019-10-11T16:15:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 11, 2019 at 08:31:30AM -0400, Derrick Stolee wrote:\n\n> >> Ensure that these lazy fetches do not occur.\n> > \n> > That makes sense. For similar reasons, should we be using\n> > OBJECT_INFO_QUICK here? If the other side has a bunch of ref tips that\n> > we don't have, we'll end up re-scanning the pack directory over and over\n> > (which is _usually_ pretty quick, but can be slow if you have a lot of\n> > packs locally). And it's OK if we racily miss out on an exclusion due to\n> > somebody else repacking simultaneously.\n> \n> That's a good idea. We can hint to the object store that we don't expect\n> misses to be due to a concurrent repack, so we don't want to reprepare\n> pack-files.\n\nAs a general rule (and why I'm raising this issue in reply to Jonathan's\npatch), I think most or all sites that want OBJECT_INFO_QUICK will want\nSKIP_FETCH_OBJECT as well, and vice versa. The reasoning is generally\nthe same:\n\n - it's OK to racily have a false negative (we'll still be correct, but\n possibly a little less optimal)\n\n - it's expected and normal to be missing the object, so spending time\n double-checking the pack store wastes measurable time in real-world\n cases\n\n-Peff\n"},{"id":"383939","messageId":"20191011220822.154063-1-jonathantanmy@google.com","threadId":"51989","inReplyTo":"20191011161504.GB19741@sigill.intra.peff.net","subject":"Re: [PATCH v2] send-pack: never fetch when checking exclusions","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-10-11T22:08:22Z","receivedAt":"2019-10-11T22:08:27Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> As a general rule (and why I'm raising this issue in reply to Jonathan's\n> patch), I think most or all sites that want OBJECT_INFO_QUICK will want\n> SKIP_FETCH_OBJECT as well, and vice versa. The reasoning is generally\n> the same:\n> \n> - it's OK to racily have a false negative (we'll still be correct, but\n> possibly a little less optimal)\n> \n> - it's expected and normal to be missing the object, so spending time\n> double-checking the pack store wastes measurable time in real-world\n> cases\n\nI took a look on \"next\" and it's true for these reasons in most cases\nbut not all.\n\nQUICK implies SKIP_FETCH_OBJECT:\n\n fetch-pack.c: Run with fetch_if_missing=0 (from builtin/fetch.c,\n builtin/fetch-pack.c, or through a lazy fetch) so OK.\n \n builtin/index-pack.c: Run with fetch_if_missing=0, so OK.\n \n builtin/fetch.c: Run with fetch_if_missing=0, so OK.\n \n object-store.h, sha1-file.c: Definition and implementation of this\n flag.\n\nEverything is OK here. Now, SKIP_FETCH_OBJECT implies QUICK:\n\n cache-tree.c: I added this recently in f981ec18cf (\"cache-tree: do not\n lazy-fetch tentative tree\", 2019-09-09). No problem with a false\n negative, since we know how to reconstruct the tree. OK.\n \n object-store.h, sha1-file.c: Definition and implementation of this\n flag.\n \n send-pack.c: This patch (which is already in \"next\"). If we have a\n false negative, we might accidentally send more than we need. But that\n is not too bad.\n \n promisor-remote.c: This is the slightly tricky one. We use this\n information to determine if we got our lazily-fetched object from the\n most recent lazy fetch, or if we should continue attempting to fetch the\n given object from other promisor remotes; so this information is\n important. However, adding QUICK doesn't lose us anything because the\n lack of QUICK only helps us when there is another process packing\n loose objects: if we got our object, our object will be in a pack\n (because of the way the fetch is implemented - in particular, we need\n a pack because we need the \".promisor\" file).\n\nSo everything is OK except for promisor-remote.c, but even that is OK\nfor another reason.\n\nHaving said that, perhaps we should consider promisor-remote.c as\nlow-level code and expect it to know that objects are fetched into a\npackfile (as opposed to loose objects), so it can safely use QUICK\n(which is documented as checking packed after packed and loose). If no\none disagrees, I can make such a patch after jt/push-avoid-lazy-fetch is\nmerged to master (as is the plan, according to What's Cooking [1]).\n\n[1] https://public-inbox.org/git/xmqq8sprhgzc.fsf@gitster-ct.c.googlers.com/\n"},{"id":"383955","messageId":"xmqqftjyg57r.fsf@gitster-ct.c.googlers.com","threadId":"51989","inReplyTo":"20191011161504.GB19741@sigill.intra.peff.net","subject":"Re: [PATCH v2] send-pack: never fetch when checking exclusions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-12T00:47:20Z","receivedAt":"2019-10-12T00:47:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> As a general rule (and why I'm raising this issue in reply to Jonathan's\n> patch), I think most or all sites that want OBJECT_INFO_QUICK will want\n> SKIP_FETCH_OBJECT as well, and vice versa. The reasoning is generally\n> the same:\n>\n> - it's OK to racily have a false negative (we'll still be correct, but\n> possibly a little less optimal)\n>\n> - it's expected and normal to be missing the object, so spending time\n> double-checking the pack store wastes measurable time in real-world\n> cases\n\n31f5256c (\"sha1-file: split OBJECT_INFO_FOR_PREFETCH\", 2019-05-28)\nseparated SKIP_FETCH_OBJECT out of FOR_PREFETCH, the latter of which\nwas and is SKIP_FETCH and QUICK combined. Use SKIP_FETCH_OBJECT\nalone may need to be re-examined and discouraged?\n\n"},{"id":"384233","messageId":"20191017061038.GA10543@sigill.intra.peff.net","threadId":"51989","inReplyTo":"20191011220822.154063-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] send-pack: never fetch when checking exclusions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-10-17T06:10:39Z","receivedAt":"2019-10-17T06:10:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 11, 2019 at 03:08:22PM -0700, Jonathan Tan wrote:\n\n> > As a general rule (and why I'm raising this issue in reply to Jonathan's\n> > patch), I think most or all sites that want OBJECT_INFO_QUICK will want\n> > SKIP_FETCH_OBJECT as well, and vice versa. The reasoning is generally\n> > the same:\n> > \n> > - it's OK to racily have a false negative (we'll still be correct, but\n> > possibly a little less optimal)\n> > \n> > - it's expected and normal to be missing the object, so spending time\n> > double-checking the pack store wastes measurable time in real-world\n> > cases\n> \n> I took a look on \"next\" and it's true for these reasons in most cases\n> but not all.\n\nThanks for digging into this.\n\n> QUICK implies SKIP_FETCH_OBJECT:\n> \n> fetch-pack.c: Run with fetch_if_missing=0 (from builtin/fetch.c,\n> builtin/fetch-pack.c, or through a lazy fetch) so OK.\n> \n> builtin/index-pack.c: Run with fetch_if_missing=0, so OK.\n> \n> builtin/fetch.c: Run with fetch_if_missing=0, so OK.\n> \n> object-store.h, sha1-file.c: Definition and implementation of this\n> flag.\n\nRight, I think going in this direction is pretty simple. Having been\nmarked with QUICK, they hit both of my points from above. And if we want\nto avoid re-scanning the pack directory because of cost, we _definitely_\nwant to avoid making an expensive network call.\n\n> Everything is OK here. Now, SKIP_FETCH_OBJECT implies QUICK:\n> \n> cache-tree.c: I added this recently in f981ec18cf (\"cache-tree: do not\n> lazy-fetch tentative tree\", 2019-09-09). No problem with a false\n> negative, since we know how to reconstruct the tree. OK.\n> [...]\n> send-pack.c: This patch (which is already in \"next\"). If we have a\n> false negative, we might accidentally send more than we need. But that\n> is not too bad.\n\nYeah, I think both of these could be QUICK.\n\n> promisor-remote.c: This is the slightly tricky one. We use this\n> information to determine if we got our lazily-fetched object from the\n> most recent lazy fetch, or if we should continue attempting to fetch the\n> given object from other promisor remotes; so this information is\n> important. However, adding QUICK doesn't lose us anything because the\n> lack of QUICK only helps us when there is another process packing\n> loose objects: if we got our object, our object will be in a pack\n> (because of the way the fetch is implemented - in particular, we need\n> a pack because we need the \".promisor\" file).\n> \n> So everything is OK except for promisor-remote.c, but even that is OK\n> for another reason.\n\nYeah, though I wouldn't be sad to see that use a separate flag, since it\nreally is about promisor logic.\n\nThat implies to me maybe we should be using QUICK more aggressively, and\nQUICK should auto-imply SKIP_FETCH_OBJECT.\n\n> Having said that, perhaps we should consider promisor-remote.c as\n> low-level code and expect it to know that objects are fetched into a\n> packfile (as opposed to loose objects), so it can safely use QUICK\n> (which is documented as checking packed after packed and loose). If no\n> one disagrees, I can make such a patch after jt/push-avoid-lazy-fetch is\n> merged to master (as is the plan, according to What's Cooking [1]).\n\nI think it's OK to continue leaving out QUICK there if it's not causing\nproblems. It really is a bit different than the other cases.\n\n-Peff\n"}]}