{"thread":{"id":"44136","subject":"[PATCH] do not reset in_vain on non-novel acks","startedAt":"2016-09-22T18:37:12Z","lastAt":"2016-09-23T19:40:41Z","messageCount":6,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"302384","messageId":"cover.1474568670.git.jonathantanmy@google.com","threadId":"44136","inReplyTo":null,"subject":"[PATCH] do not reset in_vain on non-novel acks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-22T18:36:54Z","receivedAt":"2016-09-22T18:37:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is regarding the packfile negotiation in fetch-pack. If there is a\nconcern that MAX_IN_VAIN would be hit too early (as a consequence of the\npatch below), I'm currently investigating the possibility of improving\nthe negotiation ability of the client side further (for example, by\nprioritizing refs or heads instead of merely prioritizing by date in the\npriority queue of objects), but I thought I'd send the patch out first\nanyway to see what others think.\n\nJonathan Tan (1):\n fetch-pack: do not reset in_vain on non-novel acks\n\n fetch-pack.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302385","messageId":"5a258c5dbed0683760e2ffb1bd6a1749ea66b2d5.1474568670.git.jonathantanmy@google.com","threadId":"44136","inReplyTo":"cover.1474568670.git.jonathantanmy@google.com","subject":"[PATCH] fetch-pack: do not reset in_vain on non-novel acks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-22T18:36:55Z","receivedAt":"2016-09-22T18:37:19Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The MAX_IN_VAIN mechanism was introduced in commit f061e5f (\"fetch-pack:\ngive up after getting too many \"ack continue\"\", 2006-05-24) to stop ref\nnegotiation if a number of consecutive \"have\"s have been sent with no\ncorresponding new acks. A use case (as described in that commit) is the\nscenario in which the local repository has more roots than the remote\nrepository.\n\nHowever, during a negotiation in which stateless RPCs are used,\nMAX_IN_VAIN will (almost) never trigger (in the more-roots scenario\nabove and others) because in each new request, the client has to inform\nthe server of objects it already has and knows the server has (to remind\nthe server of the state), which the server then acks.\n\nMake fetch-pack only consider novel acks (acks for objects for which the\nclient has never received an ack before in this session) as new acks for\nthe purpose of MAX_IN_VAIN.\n\nSigned-off-by: Jonathan Tan \n---\n fetch-pack.c | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 85e77af..1141e3c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -428,10 +428,18 @@ static int find_common(struct fetch_pack_args *args,\n \t\t\t\t\t\tconst char *hex = sha1_to_hex(result_sha1);\n \t\t\t\t\t\tpacket_buf_write(&req_buf, \"have %s\\n\", hex);\n \t\t\t\t\t\tstate_len = req_buf.len;\n-\t\t\t\t\t}\n+\t\t\t\t\t\t/*\n+\t\t\t\t\t\t * Reset in_vain because this\n+\t\t\t\t\t\t * ack is a novel ack (that is,\n+\t\t\t\t\t\t * an ack for this commit has\n+\t\t\t\t\t\t * not been seen).\n+\t\t\t\t\t\t */\n+\t\t\t\t\t\tin_vain = 0;\n+\t\t\t\t\t} else if (!args->stateless_rpc\n+\t\t\t\t\t\t || ack != ACK_common)\n+\t\t\t\t\t\tin_vain = 0;\n \t\t\t\t\tmark_common(commit, 0, 1);\n \t\t\t\t\tretval = 0;\n-\t\t\t\t\tin_vain = 0;\n \t\t\t\t\tgot_continue = 1;\n \t\t\t\t\tif (ack == ACK_ready) {\n \t\t\t\t\t\tclear_prio_queue(&rev_list);\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302399","messageId":"xmqqmviz4u8r.fsf@gitster.mtv.corp.google.com","threadId":"44136","inReplyTo":"cover.1474568670.git.jonathantanmy@google.com","subject":"Re: [PATCH] do not reset in_vain on non-novel acks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-22T19:20:04Z","receivedAt":"2016-09-22T19:21:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> This is regarding the packfile negotiation in fetch-pack. If there is a\n> concern that MAX_IN_VAIN would be hit too early (as a consequence of the\n> patch below), I'm currently investigating the possibility of improving\n> the negotiation ability of the client side further (for example, by\n> prioritizing refs or heads instead of merely prioritizing by date in the\n> priority queue of objects), but I thought I'd send the patch out first\n> anyway to see what others think.\n>\n> Jonathan Tan (1):\n> fetch-pack: do not reset in_vain on non-novel acks\n>\n> fetch-pack.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n\nJust a hint, because you are relatively new to the project. It\nusually is not very productive to have a cover letter to a single\npatch. Your cover letter either ends up being useless, or ends up\ncosting you time by having to repeat what you write for the patch\nanyway (and making others to read it twice).\n\nBelow the \"---\" line of the single patch is often a better place to\ntell a backstory of the patch if you need to.\n"},{"id":"302402","messageId":"xmqqfuor4s4z.fsf@gitster.mtv.corp.google.com","threadId":"44136","inReplyTo":"5a258c5dbed0683760e2ffb1bd6a1749ea66b2d5.1474568670.git.jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: do not reset in_vain on non-novel acks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-22T20:05:32Z","receivedAt":"2016-09-22T20:05:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> The MAX_IN_VAIN mechanism was introduced in commit f061e5f (\"fetch-pack:\n> give up after getting too many \"ack continue\"\", 2006-05-24) to stop ref\n> negotiation if a number of consecutive \"have\"s have been sent with no\n> corresponding new acks. A use case (as described in that commit) is the\n> scenario in which the local repository has more roots than the remote\n> repository.\n\nTo those who know what the mechanism is about, the above is\nsufficient to refresh their memory, but to others, a brief\nexplanation of _why_ it is a good idea to stop is needed to\nunderstand what you are trying to achieve with this change.\n\nIt may help to add something like \"This will stop the client to dig\ntoo deep in an irrelevant side branch in vain without ever finding a\ncommon ancestor.\" before \"A use case is ...\", perhaps?\n\nBy the way, you made me run \"git show -W f061e5f\" and then compare\nit with \"less fetch-pack.c\"; I am kind of surprised to see that\nfind_common() has grown quite a bit over the years.\n\n> However, during a negotiation in which stateless RPCs are used,\n> MAX_IN_VAIN will (almost) never trigger (in the more-roots scenario\n> above and others) because in each new request, the client has to inform\n> the server of objects it already has and knows the server has (to remind\n> the server of the state), which the server then acks.\n\nHmph. So the problem you are trying to solve is that the current\ncode sees that the other side said 'yeah, that is a common commit'\nby giving us ACK common, and resets the in_vain counter, when in\nfact we haven't made _any_ progress at that point.\n\n> Make fetch-pack only consider novel acks (acks for objects for which the\n> client has never received an ack before in this session) as new acks for\n> the purpose of MAX_IN_VAIN.\n\nMakes sense.\n\nJust a hint, because you are relatively new to the project.\nWhenever you are tempted to say \"In other words...\", \"That\nmeans...\", or further elaborte in parentheses, it pays to stop and\nthink if you can do without whatever you said before that. In the\nabove paragraph and in the comment in the patch, a newly invented\nterm \"novel ack\" is used exactly once, and because it is a newly\ninvented word, you need to explain what you want it to mean, but\nthere is no need to do so. \"Make fetch-pack only consider acks for\nobjects for which no earlier acks have been seen ...\" is equally\nreadable and does not burden the readers with \"Ah, the author\nintroduced a new term 'novel ack', so I need to remember that this\nis the definition of the word when I see it mentioned next time\".\n\n> Signed-off-by: Jonathan Tan \n> ---\n> fetch-pack.c | 12 ++++++++++--\n> 1 file changed, 10 insertions(+), 2 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 85e77af..1141e3c 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -428,10 +428,18 @@ static int find_common(struct fetch_pack_args *args,\n> \t\t\t\t\t\tconst char *hex = sha1_to_hex(result_sha1);\n> \t\t\t\t\t\tpacket_buf_write(&req_buf, \"have %s\\n\", hex);\n> \t\t\t\t\t\tstate_len = req_buf.len;\n> -\t\t\t\t\t}\n> +\t\t\t\t\t\t/*\n> +\t\t\t\t\t\t * Reset in_vain because this\n> +\t\t\t\t\t\t * ack is a novel ack (that is,\n> +\t\t\t\t\t\t * an ack for this commit has\n> +\t\t\t\t\t\t * not been seen).\n> +\t\t\t\t\t\t */\n\nSide note. Having to wrap the multi-line comment like this is a\nsign that the loop got a bit too big to fit in brain. We may want\nto see if there is way to reduce the complexity by introducing a\nhelper function or something.\n\n> +\t\t\t\t\t\tin_vain = 0;\n> +\t\t\t\t\t} else if (!args->stateless_rpc\n> +\t\t\t\t\t\t || ack != ACK_common)\n> +\t\t\t\t\t\tin_vain = 0;\n\nIt is a bit hard to read this hunk without pre-context. The\noriginal reads like so:\n\n\n\t...\n\tcase ACK_common:\n\tcase ACK_ready:\n\tcase ACK_continue: {\n\t\tstruct commit *commit =\n\t\t\tlookup_commit(result_sha1);\n\t\tif (!commit)\n\t\t\tdie(\"invalid commit %s\", sha1_to_hex(result_sha1));\n\t\tif (args->stateless_rpc\n\t\t && ack == ACK_common\n\t\t && !(commit->object.flags & COMMON)) {\n\nHere, they told us that this is a common ancestor by giving us \"ACK\ncommon\", and this is not a response to our attempt to prime a new\nincarnation of stateless server. It is curious that only ACK_common\nis checked, but it is OK because --stateless requires multi-ack and\nACK_continue is not used.\n\n\t\t\t/* We need to replay the have for this object\n\t\t\t * on the next RPC request so the peer knows\n\t\t\t * it is in common with us.\n\t\t\t */\n\t\t\tconst char *hex = sha1_to_hex(result_sha1);\n\t\t\tpacket_buf_write(&req_buf, \"have %s\\n\", hex);\n\t\t\tstate_len = req_buf.len;\n\nAnd we store it away so that the next found will start with these\nobjects as \"have\" to remind the other side where we were.\n\n> +\t\t\tin_vain = 0;\n\nAnd at this point, you reset in_vain counter with your change.\nWhich makes sense. This is a newly discovered common one, i.e. we\nare making progress.\n\n-\t\t}\n> +\t\t} else if (!args->stateless_rpc\n> +\t\t\t || ack != ACK_common)\n> +\t\t\tin_vain = 0;\n\nAnd you add an else clause here to reset in_vain counter, which we\nused to unconditionally do, when stateless is not in use, or when we\nare doing stateless and got something other than \"ACK common\". The\nlatter is to make sure that \"ACK common\" for commits we have already\nknown are common do not count as making progress.\n\nMakes perfect sense to me.\n\n\t\tmark_common(commit, 0, 1);\n\t\tretval = 0;\n-\t\tin_vain = 0;\n\nAnd you remove the unconditional reset.\n\n\t\tgot_continue = 1;\n\t\tif (ack == ACK_ready) {\n\t\t\tclear_prio_queue(&rev_list);\n\t\t\tgot_ready = 1;\n\t\t}\n\t\tbreak;\n\t\t}\n\t}\n\nEverything looks good to me and well thought-out.\n\nThanks.\n"},{"id":"302453","messageId":"1474652495-26051-1-git-send-email-jonathantanmy@google.com","threadId":"44136","inReplyTo":"xmqqfuor4s4z.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] fetch-pack: do not reset in_vain on non-novel acks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2016-09-23T17:41:35Z","receivedAt":"2016-09-23T17:41:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The MAX_IN_VAIN mechanism was introduced in commit f061e5f (\"fetch-pack:\ngive up after getting too many \"ack continue\"\", 2006-05-24) to stop ref\nnegotiation if a number of consecutive \"have\"s have been sent with no\ncorresponding new acks. This is to stop the client from digging too deep\nin an irrelevant side branch in vain without ever finding a common\nancestor. A use case (as described in that commit) is the scenario in\nwhich the local repository has more roots than the remote repository.\n\nHowever, during a negotiation in which stateless RPCs are used,\nMAX_IN_VAIN will (almost) never trigger (in the more-roots scenario\nabove and others) because in each new request, the client has to inform\nthe server of objects it already has and knows the server has (to remind\nthe server of the state), which the server then acks.\n\nMake fetch-pack only consider, as new acks for the purpose of\nMAX_IN_VAIN, acks for objects for which the client has never received an\nack before in this session.\n\nSigned-off-by: Jonathan Tan \n---\n\nThanks for your comments - I really appreciate them.\n\nUpdate from original:\no removed redundant text from commit message and comment in patch\no mentioned stopping the client from digging too deep in the commit\n message\n\nI tried looking at creating a helper function to reduce both the size\nand the nesting level of the loop, but it seems to me that a helper\nfunction can't be extracted so easily because the logic is quite\nintertwined with the rest of the function. For example, the \"if\n(args->stateless_rpc...\" block uses 6 variables from the outer scope:\nargs, ack, commit, result_sha1, req_buf, and state_len (and in_vain, but\nthis can be the return value of the function). Expanding it wider would\nallow us to make some of those 6 local, but also introduce new ones from\nthe outer scope.\n\n fetch-pack.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 85e77af..413937e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -428,10 +428,17 @@ static int find_common(struct fetch_pack_args *args,\n \t\t\t\t\t\tconst char *hex = sha1_to_hex(result_sha1);\n \t\t\t\t\t\tpacket_buf_write(&req_buf, \"have %s\\n\", hex);\n \t\t\t\t\t\tstate_len = req_buf.len;\n-\t\t\t\t\t}\n+\t\t\t\t\t\t/*\n+\t\t\t\t\t\t * Reset in_vain because an ack\n+\t\t\t\t\t\t * for this commit has not been\n+\t\t\t\t\t\t * seen.\n+\t\t\t\t\t\t */\n+\t\t\t\t\t\tin_vain = 0;\n+\t\t\t\t\t} else if (!args->stateless_rpc\n+\t\t\t\t\t\t || ack != ACK_common)\n+\t\t\t\t\t\tin_vain = 0;\n \t\t\t\t\tmark_common(commit, 0, 1);\n \t\t\t\t\tretval = 0;\n-\t\t\t\t\tin_vain = 0;\n \t\t\t\t\tgot_continue = 1;\n \t\t\t\t\tif (ack == ACK_ready) {\n \t\t\t\t\t\tclear_prio_queue(&rev_list);\n-- \n2.8.0.rc3.226.g39d4020\n\n"},{"id":"302459","messageId":"xmqq1t0a1k2r.fsf@gitster.mtv.corp.google.com","threadId":"44136","inReplyTo":"1474652495-26051-1-git-send-email-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch-pack: do not reset in_vain on non-novel acks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-23T19:40:12Z","receivedAt":"2016-09-23T19:40:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> I tried looking at creating a helper function to reduce both the size\n> and the nesting level of the loop, but it seems to me that a helper\n> function can't be extracted so easily because the logic is quite\n> intertwined with the rest of the function. For example, the \"if\n> (args->stateless_rpc...\" block uses 6 variables from the outer scope:\n> args, ack, commit, result_sha1, req_buf, and state_len (and in_vain, but\n> this can be the return value of the function). Expanding it wider would\n> allow us to make some of those 6 local, but also introduce new ones from\n> the outer scope.\n\nYup, I suspected that much when I wrote the message you are\nresponding to, but was sort-of hoping that you might come up with a\nmore clever way to restructure the code. It is OK to leave it\nas-is, and let others try making it cleaner ;-).\n\nThanks.\n\n>\n> fetch-pack.c | 11 +++++++++--\n> 1 file changed, 9 insertions(+), 2 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 85e77af..413937e 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -428,10 +428,17 @@ static int find_common(struct fetch_pack_args *args,\n> \t\t\t\t\t\tconst char *hex = sha1_to_hex(result_sha1);\n> \t\t\t\t\t\tpacket_buf_write(&req_buf, \"have %s\\n\", hex);\n> \t\t\t\t\t\tstate_len = req_buf.len;\n> -\t\t\t\t\t}\n> +\t\t\t\t\t\t/*\n> +\t\t\t\t\t\t * Reset in_vain because an ack\n> +\t\t\t\t\t\t * for this commit has not been\n> +\t\t\t\t\t\t * seen.\n> +\t\t\t\t\t\t */\n> +\t\t\t\t\t\tin_vain = 0;\n> +\t\t\t\t\t} else if (!args->stateless_rpc\n> +\t\t\t\t\t\t || ack != ACK_common)\n> +\t\t\t\t\t\tin_vain = 0;\n> \t\t\t\t\tmark_common(commit, 0, 1);\n> \t\t\t\t\tretval = 0;\n> -\t\t\t\t\tin_vain = 0;\n> \t\t\t\t\tgot_continue = 1;\n> \t\t\t\t\tif (ack == ACK_ready) {\n> \t\t\t\t\t\tclear_prio_queue(&rev_list);\n"}]}