{"thread":{"id":"52291","subject":"Push a ref to a remote with many refs","startedAt":"2019-11-19T13:20:08Z","lastAt":"2019-12-10T20:27:35Z","messageCount":12,"participants":["Patrick Marlier (pamarlie)","Jeff King","Junio C Hamano","Jonathan Nieder","Jonathan Tan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"386514","messageId":"CH2PR11MB429411CA1288526D21C7AF26CF4C0@CH2PR11MB4294.namprd11.prod.outlook.com","threadId":"52291","inReplyTo":null,"subject":"Push a ref to a remote with many refs","fromName":"Patrick Marlier (pamarlie)","fromEmail":"pamarlie@cisco.com","sentAt":"2019-11-19T13:12:51Z","receivedAt":"2019-11-19T13:20:08Z","isPatch":false,"sender":{"key":"pamarlie@cisco.com","avatar":null},"body":"Dear git community,\n\nI am hitting a performance issue with \"git push <remote> <refspec>\".\nThe local repository has only few refs and the remote repository has a lot of refs (1000+) with objects unknown to the local repository.\n\n\"git push\" of only one refspec takes minutes to complete. A quick analysis shows that a lot of time is spent in the client side.\nA deeper analysis shows that the client receives the entire list of refs on the remote, then the client is checking in its local repository if the objects exist for all remote refs.\nSince the local repository has a only few refs, most of the objects are unknown.\nThis issue is particularly amplified because the local repository is using many alternates. Indeed for each unknown object, git will try to find in all alternates too.\n\nTo workaround this issue, I ended up skipping the refs that are not part of the push. See patch at the end of the message.\nIs this safe to do? Is there a better way to do this?\n\nThanks.\nHave a nice day,\n\nAuthor: Patrick Marlier <pamarlie@cisco.com>\nDate:   Tue Aug 6 21:40:48 2019 +0200\n\n    send-pack skip if not part of refspec\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 34c77cbb1a..0fe899f753 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -94,7 +94,7 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc\n                feed_object(&extra->oid[i], po_in, 1);\n \n        while (refs) {\n-               if (!is_null_oid(&refs->old_oid))\n+               if (!is_null_oid(&refs->old_oid) && !is_null_oid(&refs->new_oid))\n                        feed_object(&refs->old_oid, po_in, 1);\n                if (!is_null_oid(&refs->new_oid))\n                        feed_object(&refs->new_oid, po_in, 0);\n"},{"id":"387007","messageId":"CH2PR11MB4294168406728D1510A92704CF4A0@CH2PR11MB4294.namprd11.prod.outlook.com","threadId":"52291","inReplyTo":"CH2PR11MB429411CA1288526D21C7AF26CF4C0@CH2PR11MB4294.namprd11.prod.outlook.com","subject":"Re: Push a ref to a remote with many refs","fromName":"Patrick Marlier (pamarlie)","fromEmail":"pamarlie@cisco.com","sentAt":"2019-11-25T16:22:48Z","receivedAt":"2019-11-25T16:30:20Z","isPatch":false,"sender":{"key":"pamarlie@cisco.com","avatar":null},"body":"> To workaround this issue, I ended up skipping the refs that are not part of the push. See patch at the end of the message.\n> Is this safe to do? Is there a better way to do this?\n\nSome help on this?\n\nThanks!\n--\nPat"},{"id":"387147","messageId":"20191127123211.GG22221@sigill.intra.peff.net","threadId":"52291","inReplyTo":"CH2PR11MB429411CA1288526D21C7AF26CF4C0@CH2PR11MB4294.namprd11.prod.outlook.com","subject":"[PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-11-27T12:32:11Z","receivedAt":"2019-11-27T12:32:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 19, 2019 at 01:12:51PM +0000, Patrick Marlier (pamarlie) wrote:\n\n> I am hitting a performance issue with \"git push <remote> <refspec>\".\n> The local repository has only few refs and the remote repository has a\n> lot of refs (1000+) with objects unknown to the local repository.\n> \n> \"git push\" of only one refspec takes minutes to complete. A quick\n> analysis shows that a lot of time is spent in the client side.\n> A deeper analysis shows that the client receives the entire list of\n> refs on the remote, then the client is checking in its local\n> repository if the objects exist for all remote refs.\n\nRight, this is expected. The client send-pack feeds the list of remote\nobjects (that it has) to pack-objects, which can then limit the size of\nthe packfile it sends based on what the other side has.\n\nSo the patch you showed (to skip refs that aren't part of the push)\nwould miss many opportunities for a smaller push. E.g., imagine I create\na new branch \"topic\" from the remote branch \"master\", and then try to\npush it up. If I do not look at which objects are in \"master\", I'll end\nup sending the whole history again, when I really just need to send the\ndifferences between the two.\n\n> Since the local repository has a only few refs, most of the objects\n> are unknown.\n>\n> This issue is particularly amplified because the local repository is\n> using many alternates. Indeed for each unknown object, git will try to\n> find in all alternates too.\n\nI think the patch below would help you.\n\n-- >8 --\nSubject: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects\n\nWhen pushing, we feed pack-objects a list of both positive and negative\nobjects. The positive objects are what we want to send, and the negative\nobjects are what the other side told us they have, which we can use to\nlimit the size of the push.\n\nBefore passing along a negative object, send_pack() will make sure we\nactually have it (since we only know about it because the remote\nmentioned it, not because it's one of our refs). So it's expected that\nsome of these objects will be missing on the local side. But looking for\na missing object is more expensive than one that we have: it triggers\nreprepare_packed_git() to handle a racy repack, plus it has to explore\nevery alternate's loose object tree (which can be slow if you have a lot\nof them, or have a high-latency filesystem).\n\nThis isn't usually a big problem, since repositories you're pushing to\ndon't generally have a large number of refs that are unrelated to what\nthe client has. But there's no reason such a setup is wrong, and it\ncurrently performs poorly.\n\nWe can fix this by using OBJECT_INFO_QUICK, which tells the lookup\ncode that we expect objects to be missing. Notably, it will not re-scan\nthe packs, and it will use the loose cache from 61c7711cfe (sha1-file:\nuse loose object cache for quick existence check, 2018-11-12).\n\nThe downside is that in the rare case that we race with a local repack,\nwe might fail to feed some objects to pack-objects, making the resulting\npush larger. But we'd never produce an invalid or incorrect push, just a\nless optimal one. That seems like a reasonable tradeoff, and we already\ndo similar things on the fetch side (e.g., when marking COMPLETE\ncommits).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nInterestingly, upload-pack does not use OBJECT_INFO_QUICK when it's\ngetting oids from the other side. But I think it could possibly benefit\nin the same way. Nobody seems to have noticed. Perhaps it simply comes\nup less, as servers would tend to have more objects than their clients?\n\n send-pack.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 34c77cbb1a..16d6584439 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -41,7 +41,9 @@ int option_parse_push_signed(const struct option *opt,\n static void feed_object(const struct object_id *oid, FILE *fh, int negative)\n {\n \tif (negative &&\n-\t    !has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n+\t    !has_object_file_with_flags(oid,\n+\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT |\n+\t\t\t\t\tOBJECT_INFO_QUICK))\n \t\treturn;\n \n \tif (negative)\n-- \n2.24.0.716.g722aff65ed\n\n"},{"id":"387299","messageId":"CH2PR11MB4294A6C1AA71DF1A1721C94CCF460@CH2PR11MB4294.namprd11.prod.outlook.com","threadId":"52291","inReplyTo":"20191127123211.GG22221@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Patrick Marlier (pamarlie)","fromEmail":"pamarlie@cisco.com","sentAt":"2019-11-29T09:22:22Z","receivedAt":"2019-11-29T09:22:30Z","isPatch":true,"sender":{"key":"pamarlie@cisco.com","avatar":null},"body":"> On Tue, Nov 19, 2019 at 01:12:51PM +0000, Patrick Marlier (pamarlie) wrote:\n>> I am hitting a performance issue with \"git push <remote> <refspec>\".\n>> The local repository has only few refs and the remote repository has a\n>> lot of refs (1000+) with objects unknown to the local repository.\n>> \n>> \"git push\" of only one refspec takes minutes to complete. A quick\n>> analysis shows that a lot of time is spent in the client side.\n>> A deeper analysis shows that the client receives the entire list of\n>> refs on the remote, then the client is checking in its local\n>> repository if the objects exist for all remote refs.\n\n>Right, this is expected. The client send-pack feeds the list of remote\n>objects (that it has) to pack-objects, which can then limit the size of\n>the packfile it sends based on what the other side has.\n\n>So the patch you showed (to skip refs that aren't part of the push)\n>would miss many opportunities for a smaller push. E.g., imagine I create\n>a new branch \"topic\" from the remote branch \"master\", and then try to\n>push it up. If I do not look at which objects are in \"master\", I'll end\n>up sending the whole history again, when I really just need to send the\n>differences between the two.\n\nThanks Jeff. Indeed it makes sense.\n(My very particular use-case would have a limited impact in this case)\n\n\n>> Since the local repository has a only few refs, most of the objects\n>> are unknown.\n>>\n>> This issue is particularly amplified because the local repository is\n>> using many alternates. Indeed for each unknown object, git will try to\n>> find in all alternates too.\n\n> I think the patch below would help you.\n\nI will give a try next week and I keep you posted on how it goes.\n\nThanks a lot for your input and the patch!\n\nHave a nice day,\n--\nPat"},{"id":"387337","messageId":"xmqqr21p2sxb.fsf@gitster-ct.c.googlers.com","threadId":"52291","inReplyTo":"20191127123211.GG22221@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-30T17:08:32Z","receivedAt":"2019-11-30T17:08:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think the patch below would help you.\n>\n> -- >8 --\n> Subject: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects\n>\n> When pushing, we feed pack-objects a list of both positive and negative\n> objects. The positive objects are what we want to send, and the negative\n> objects are what the other side told us they have, which we can use to\n> limit the size of the push.\n>\n> Before passing along a negative object, send_pack() will make sure we\n> actually have it (since we only know about it because the remote\n> mentioned it, not because it's one of our refs). So it's expected that\n> some of these objects will be missing on the local side. But looking for\n> a missing object is more expensive than one that we have: it triggers\n> reprepare_packed_git() to handle a racy repack, plus it has to explore\n> every alternate's loose object tree (which can be slow if you have a lot\n> of them, or have a high-latency filesystem).\n>\n> This isn't usually a big problem, since repositories you're pushing to\n> don't generally have a large number of refs that are unrelated to what\n> the client has. But there's no reason such a setup is wrong, and it\n> currently performs poorly.\n>\n> We can fix this by using OBJECT_INFO_QUICK, which tells the lookup\n> code that we expect objects to be missing. Notably, it will not re-scan\n> the packs, and it will use the loose cache from 61c7711cfe (sha1-file:\n> use loose object cache for quick existence check, 2018-11-12).\n>\n> The downside is that in the rare case that we race with a local repack,\n> we might fail to feed some objects to pack-objects, making the resulting\n> push larger. But we'd never produce an invalid or incorrect push, just a\n> less optimal one. That seems like a reasonable tradeoff, and we already\n> do similar things on the fetch side (e.g., when marking COMPLETE\n> commits).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> Interestingly, upload-pack does not use OBJECT_INFO_QUICK when it's\n> getting oids from the other side. But I think it could possibly benefit\n> in the same way. Nobody seems to have noticed. Perhaps it simply comes\n> up less, as servers would tend to have more objects than their clients?\n\nMakes me wonder how many times we wre bitten by the fact that\nINFO_SKIP_FETCH_OBJECT does not imply INFO_QUICK.  Perhaps most of\nthe users of INFO_SKIP_FETCH_OBJECT wants to use INFO_FOR_PREFETCH?\n\ncf. 31f5256c (\"sha1-file: split OBJECT_INFO_FOR_PREFETCH\", 2019-05-28)\n\n>  send-pack.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/send-pack.c b/send-pack.c\n> index 34c77cbb1a..16d6584439 100644\n> --- a/send-pack.c\n> +++ b/send-pack.c\n> @@ -41,7 +41,9 @@ int option_parse_push_signed(const struct option *opt,\n>  static void feed_object(const struct object_id *oid, FILE *fh, int negative)\n>  {\n>  \tif (negative &&\n> -\t    !has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n> +\t    !has_object_file_with_flags(oid,\n> +\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT |\n> +\t\t\t\t\tOBJECT_INFO_QUICK))\n>  \t\treturn;\n>  \n>  \tif (negative)\n"},{"id":"387470","messageId":"20191203232007.GA30535@sigill.intra.peff.net","threadId":"52291","inReplyTo":"xmqqr21p2sxb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-03T23:20:07Z","receivedAt":"2019-12-03T23:20:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 30, 2019 at 09:08:32AM -0800, Junio C Hamano wrote:\n\n> > Interestingly, upload-pack does not use OBJECT_INFO_QUICK when it's\n> > getting oids from the other side. But I think it could possibly benefit\n> > in the same way. Nobody seems to have noticed. Perhaps it simply comes\n> > up less, as servers would tend to have more objects than their clients?\n> \n> Makes me wonder how many times we wre bitten by the fact that\n> INFO_SKIP_FETCH_OBJECT does not imply INFO_QUICK.  Perhaps most of\n> the users of INFO_SKIP_FETCH_OBJECT wants to use INFO_FOR_PREFETCH?\n\nWe seem to be discussing this about once a month lately. :)\n\nThere's some good digging by Jonathan in:\n\n  https://public-inbox.org/git/20191011220822.154063-1-jonathantanmy@google.com/\n\nThat thread is also about this exact same spot, which is why I cc'd him\noriginally.\n\nI do tend to think that QUICK ought to imply SKIP_FETCH. I'm slightly\nnegative on sprinkling FOR_PREFETCH everywhere, because the name implies\nto me that we are telling object_info() that we are pre-fetching. Which\nyes, has the effect we want, but I think is misleading. So we'd want to\nchange the name of the combined flag, I think, or just let QUICK imply\nSKIP_FETCH and use that more thoroughly.\n\n-Peff\n"},{"id":"387471","messageId":"20191204035522.GC214771@google.com","threadId":"52291","inReplyTo":"20191127123211.GG22221@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-12-04T03:55:22Z","receivedAt":"2019-12-04T03:55:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> Subject: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects\n>\n> When pushing, we feed pack-objects a list of both positive and negative\n> objects. The positive objects are what we want to send, and the negative\n> objects are what the other side told us they have, which we can use to\n> limit the size of the push.\n>\n> Before passing along a negative object, send_pack() will make sure we\n> actually have it (since we only know about it because the remote\n> mentioned it, not because it's one of our refs). So it's expected that\n> some of these objects will be missing on the local side. But looking for\n> a missing object is more expensive than one that we have: it triggers\n> reprepare_packed_git() to handle a racy repack, plus it has to explore\n> every alternate's loose object tree (which can be slow if you have a lot\n> of them, or have a high-latency filesystem).\n\nNice analysis.\n\n> This isn't usually a big problem, since repositories you're pushing to\n> don't generally have a large number of refs that are unrelated to what\n> the client has. But there's no reason such a setup is wrong, and it\n> currently performs poorly.\n>\n> We can fix this by using OBJECT_INFO_QUICK, which tells the lookup\n> code that we expect objects to be missing. Notably, it will not re-scan\n> the packs, and it will use the loose cache from 61c7711cfe (sha1-file:\n> use loose object cache for quick existence check, 2018-11-12).\n\nOn first reading, I wondered how this would interact with alternates,\nsince you had mentioned that checking alternates can be expensive.  Does\nthis go too far in that direction by treating an object as missing\nwhenever it's not in the local object store, even if it's available from\nan alternate?\n\nBut I believe that was a misreading.  With this patch, we still do pay\nthe cost of checking alternates for the missing object.  The savings\nis instead about having to *double* check.\n\nAm I understanding correctly?\n\n[...]\n> Signed-off-by: Jeff King <peff@peff.net>\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n> ---\n> Interestingly, upload-pack does not use OBJECT_INFO_QUICK when it's\n> getting oids from the other side. But I think it could possibly benefit\n> in the same way. Nobody seems to have noticed. Perhaps it simply comes\n> up less, as servers would tend to have more objects than their clients?\n\nI like to imagine that servers are also more likely to keep a tidy set\nof packs and to avoid alternates.  But using INFO_QUICK when checking\nthe fetcher's \"have\"s does sound like a sensible change to me.\n\nThanks,\nJonathan\n"},{"id":"387472","messageId":"20191204040544.GA11502@sigill.intra.peff.net","threadId":"52291","inReplyTo":"20191204035522.GC214771@google.com","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-04T04:05:44Z","receivedAt":"2019-12-04T04:05:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 03, 2019 at 07:55:22PM -0800, Jonathan Nieder wrote:\n\n> > We can fix this by using OBJECT_INFO_QUICK, which tells the lookup\n> > code that we expect objects to be missing. Notably, it will not re-scan\n> > the packs, and it will use the loose cache from 61c7711cfe (sha1-file:\n> > use loose object cache for quick existence check, 2018-11-12).\n> \n> On first reading, I wondered how this would interact with alternates,\n> since you had mentioned that checking alternates can be expensive.  Does\n> this go too far in that direction by treating an object as missing\n> whenever it's not in the local object store, even if it's available from\n> an alternate?\n> \n> But I believe that was a misreading.  With this patch, we still do pay\n> the cost of checking alternates for the missing object.  The savings\n> is instead about having to *double* check.\n> \n> Am I understanding correctly?\n\nYes, we'd still look in alternates for each object before giving up. The\nreason alternates are relevant is that normally if you have (say) 5\nalternates, then you have to do 5 syscalls to find out whether each\nalternate has an object. And alternates are more likely to be on\nhigh-latency filesystems like NFS, which exacerbates the cost. But with\nOBJECT_INFO_QUICK, we'll build an in-memory cache for each alternate\ndirectory (as well as the main object store, of course), rather than\nmaking one request per object.\n\n> > Interestingly, upload-pack does not use OBJECT_INFO_QUICK when it's\n> > getting oids from the other side. But I think it could possibly benefit\n> > in the same way. Nobody seems to have noticed. Perhaps it simply comes\n> > up less, as servers would tend to have more objects than their clients?\n> \n> I like to imagine that servers are also more likely to keep a tidy set\n> of packs and to avoid alternates.  But using INFO_QUICK when checking\n> the fetcher's \"have\"s does sound like a sensible change to me.\n\nAt GitHub we do use alternates (but only one, and on the same local\ndisk). And our packing situation does sometimes get unwieldy. I think it\nmight be worth looking into, but it would be nice to have real numbers\nbefore proceeding (likewise we've known about this spot in send-pack,\nbut it hadn't been expensive enough for anybody to notice; I'll be\ncurious to see real-world numbers from Patrick's case).\n\n-Peff\n"},{"id":"387510","messageId":"20191204205353.118168-1-jonathantanmy@google.com","threadId":"52291","inReplyTo":"20191203232007.GA30535@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-12-04T20:53:53Z","receivedAt":"2019-12-04T20:54:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> On Sat, Nov 30, 2019 at 09:08:32AM -0800, Junio C Hamano wrote:\n> \n> > > Interestingly, upload-pack does not use OBJECT_INFO_QUICK when it's\n> > > getting oids from the other side. But I think it could possibly benefit\n> > > in the same way. Nobody seems to have noticed. Perhaps it simply comes\n> > > up less, as servers would tend to have more objects than their clients?\n> > \n> > Makes me wonder how many times we wre bitten by the fact that\n> > INFO_SKIP_FETCH_OBJECT does not imply INFO_QUICK.  Perhaps most of\n> > the users of INFO_SKIP_FETCH_OBJECT wants to use INFO_FOR_PREFETCH?\n> \n> We seem to be discussing this about once a month lately. :)\n> \n> There's some good digging by Jonathan in:\n> \n>   https://public-inbox.org/git/20191011220822.154063-1-jonathantanmy@google.com/\n> \n> That thread is also about this exact same spot, which is why I cc'd him\n> originally.\n> \n> I do tend to think that QUICK ought to imply SKIP_FETCH. I'm slightly\n> negative on sprinkling FOR_PREFETCH everywhere, because the name implies\n> to me that we are telling object_info() that we are pre-fetching. Which\n> yes, has the effect we want, but I think is misleading. So we'd want to\n> change the name of the combined flag, I think, or just let QUICK imply\n> SKIP_FETCH and use that more thoroughly.\n\nIf we want to make QUICK imply SKIP_FETCH but not the other way around, then\nnote that we'll have to make sure that we do not repeat the error that\n31f5256c82 (\"sha1-file: split OBJECT_INFO_FOR_PREFETCH\", 2019-05-28) fixes.\n\nAs for whether we should do it in the first place, I think that having 2\northogonal flags is clearer than having 1 that controls fetch\n(SKIP_FETCH) and 1 that does both (QUICK) without any that recheck\npacked, but I'm quite familiar with this part of the code, so perhaps\nI'm the wrong person to ask - we should optimize for the typical Git\ndeveloper who just wants to access the object store. Judging from the\nfrequency in which we encounter this issue (as Peff says), maybe we\nshould go ahead and make QUICK imply SKIP_FETCH but not the other way\naround. (It is also possible just to make both imply the other and unify\nQUICK and SKIP_FETCH into one flag - I am not opposed to that - although\nread my email that Peff linked to see why bidirectional implication is\ncorrect in one sense but incorrect in another.)\n"},{"id":"387519","messageId":"xmqqr21j3h7e.fsf@gitster-ct.c.googlers.com","threadId":"52291","inReplyTo":"20191204205353.118168-1-jonathantanmy@google.com","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-04T21:37:41Z","receivedAt":"2019-12-04T21:37:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> As for whether we should do it in the first place, I think that having 2\n> orthogonal flags is clearer than having 1 that controls fetch\n> (SKIP_FETCH) and 1 that does both (QUICK) without any that recheck\n> packed, but I'm quite familiar with this part of the code, so perhaps\n> I'm the wrong person to ask - we should optimize for the typical Git\n> developer who just wants to access the object store. Judging from the\n> frequency in which we encounter this issue (as Peff says), maybe we\n> should go ahead and make QUICK imply SKIP_FETCH but not the other way\n> around.\n\nThanks for thinking this through.  I am OK with that direction.\n\n> (It is also possible just to make both imply the other and unify\n> QUICK and SKIP_FETCH into one flag - I am not opposed to that -\n> although read my email that Peff linked to see why bidirectional\n> implication is correct in one sense but incorrect in another.)\n\nYup, also from the old message:\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 agree that it makes sense to treat promisor-remote.c as the\nlow-level implementation detail that is allowed to be intimately\nfamiliar with how its behaviour would interact with QUICK and\nSKIP_FETCH options.\n\nThanks.\n"},{"id":"387899","messageId":"CH2PR11MB4294C33B1CC956D5BF6AAB81CF5B0@CH2PR11MB4294.namprd11.prod.outlook.com","threadId":"52291","inReplyTo":"20191204040544.GA11502@sigill.intra.peff.net","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Patrick Marlier (pamarlie)","fromEmail":"pamarlie@cisco.com","sentAt":"2019-12-10T16:16:40Z","receivedAt":"2019-12-10T16:16:47Z","isPatch":true,"sender":{"key":"pamarlie@cisco.com","avatar":null},"body":"> From: Jeff King <peff@peff.net>\n> I'll be curious to see real-world numbers from Patrick's case).\n\nHere the real-world case (40 pushes)\n\nreleased\naverage 00:08:35.54\nmedian 00:07:37.88\nmax 00:15:30.26\n\npatched\naverage 00:06:45.67\nmedian 00:05:39.57\nmax 00:16:15.46\n\nNote our production is using 2.19 so I back-ported the patch (need to include 2 other patches).\nIt helped to improve the push duration. I know one big part is git lfs but I need to check what are the other bottlenecks.\n\nThanks a lot for the help.\n--\nPat"},{"id":"387916","messageId":"20191210202732.GA11733@coredump.intra.peff.net","threadId":"52291","inReplyTo":"CH2PR11MB4294C33B1CC956D5BF6AAB81CF5B0@CH2PR11MB4294.namprd11.prod.outlook.com","subject":"Re: [PATCH] send-pack: use OBJECT_INFO_QUICK to check negative objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-12-10T20:27:32Z","receivedAt":"2019-12-10T20:27:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 10, 2019 at 04:16:40PM +0000, Patrick Marlier (pamarlie) wrote:\n\n> > From: Jeff King <peff@peff.net>\n> > I'll be curious to see real-world numbers from Patrick's case).\n> \n> Here the real-world case (40 pushes)\n> \n> released\n> average 00:08:35.54\n> median 00:07:37.88\n> max 00:15:30.26\n> \n> patched\n> average 00:06:45.67\n> median 00:05:39.57\n> max 00:16:15.46\n> \n> Note our production is using 2.19 so I back-ported the patch (need to\n> include 2 other patches).\n\nYou may see much better performance if you move to 2.21 or higher, or\nbackport the topic from 3b2f8a02fa (Merge branch 'jk/loose-object-cache',\n2019-01-04). That would cause most of your negative lookups to not touch\nthe filesystem at all.\n\n-Peff\n"}]}