{"thread":{"id":"52194","subject":"[PATCH 0/1] fetch: add trace2 instrumentation","startedAt":"2019-11-05T19:26:24Z","lastAt":"2019-11-20T01:07:58Z","messageCount":22,"participants":["erik chen via GitGitGadget","Erik Chen via GitGitGadget","Johannes Schindelin","Junio C Hamano","Derrick Stolee","Erik Chen"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"385480","messageId":"pull.451.git.1572981981.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":null,"subject":"[PATCH 0/1] fetch: add trace2 instrumentation","fromName":"erik chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-05T19:26:19Z","receivedAt":"2019-11-05T19:26:24Z","isPatch":true,"sender":{"key":"name:erik chen","avatar":null},"body":"Add trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n* matching common remote and local refs\n* marking local refs as complete (part of the matching process)\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen erikchen@chromium.org [erikchen@chromium.org]\n\nErik Chen (1):\n  fetch: add trace2 instrumentation\n\n fetch-pack.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-451%2Ferikchen%2Ftest12-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-451/erikchen/test12-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/451\n-- \ngitgitgadget\n"},{"id":"385481","messageId":"4fdbb9f504ee59e508b7df520f7890e29fbca7df.1572981981.git.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.git.1572981981.gitgitgadget@gmail.com","subject":"[PATCH 1/1] fetch: add trace2 instrumentation","fromName":"Erik Chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-05T19:26:20Z","receivedAt":"2019-11-05T19:26:26Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"From: Erik Chen <erikchen@chromium.org>\n\nAdd trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n    * matching common remote and local refs\n    * marking local refs as complete (part of the matching process)\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen <erikchen@chromium.org>\n---\n fetch-pack.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0130b44112..f2f3365bbe 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -666,9 +666,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tstruct ref *ref;\n \tint old_save_commit_buffer = save_commit_buffer;\n \ttimestamp_t cutoff = 0;\n-\n \tsave_commit_buffer = 0;\n \n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n+\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o;\n \n@@ -690,6 +691,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\t}\n \t}\n \n+\t/* This block marks all local refs as COMPLETE, and then recursively marks all\n+\t * parents of those refs as COMPLETE.\n+\t */\n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \tif (!args->deepen) {\n \t\tfor_each_ref(mark_complete_oid, NULL);\n \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n@@ -697,6 +702,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tif (cutoff)\n \t\t\tmark_recent_complete_commits(args, cutoff);\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \n \t/*\n \t * Mark all complete remote refs as common refs.\n@@ -716,6 +722,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t}\n \n \tsave_commit_buffer = old_save_commit_buffer;\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n }\n \n /*\n-- \ngitgitgadget\n"},{"id":"385589","messageId":"nycvar.QRO.7.76.6.1911061329340.46@tvgsbejvaqbjf.bet","threadId":"52194","inReplyTo":"4fdbb9f504ee59e508b7df520f7890e29fbca7df.1572981981.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/1] fetch: add trace2 instrumentation","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-06T12:30:03Z","receivedAt":"2019-11-06T12:30:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Erik,\n\nOn Tue, 5 Nov 2019, Erik Chen via GitGitGadget wrote:\n\n> From: Erik Chen <erikchen@chromium.org>\n>\n> Add trace2 regions to fetch-pack.c to better track time spent in the various\n> phases of a fetch:\n>\n>     * matching common remote and local refs\n>     * marking local refs as complete (part of the matching process)\n>\n> Both of these stages can be slow for repositories with many refs.\n>\n> Signed-off-by: Erik Chen <erikchen@chromium.org>\n> ---\n>  fetch-pack.c | 9 ++++++++-\n>  1 file changed, 8 insertions(+), 1 deletion(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 0130b44112..f2f3365bbe 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -666,9 +666,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \tstruct ref *ref;\n>  \tint old_save_commit_buffer = save_commit_buffer;\n>  \ttimestamp_t cutoff = 0;\n> -\n\nI would keep this empty line. Other than that, this patch looks good to\nme.\n\nThanks,\nJohannes\n\n>  \tsave_commit_buffer = 0;\n>\n> +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n> +\n>  \tfor (ref = *refs; ref; ref = ref->next) {\n>  \t\tstruct object *o;\n>\n> @@ -690,6 +691,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \t\t}\n>  \t}\n>\n> +\t/* This block marks all local refs as COMPLETE, and then recursively marks all\n> +\t * parents of those refs as COMPLETE.\n> +\t */\n> +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n>  \tif (!args->deepen) {\n>  \t\tfor_each_ref(mark_complete_oid, NULL);\n>  \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n> @@ -697,6 +702,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \t\tif (cutoff)\n>  \t\t\tmark_recent_complete_commits(args, cutoff);\n>  \t}\n> +\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n>\n>  \t/*\n>  \t * Mark all complete remote refs as common refs.\n> @@ -716,6 +722,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \t}\n>\n>  \tsave_commit_buffer = old_save_commit_buffer;\n> +\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n>  }\n>\n>  /*\n> --\n> gitgitgadget\n>\n"},{"id":"385614","messageId":"4fdbb9f504ee59e508b7df520f7890e29fbca7df.1573066300.git.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.v2.git.1573066300.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] fetch: add trace2 instrumentation","fromName":"Erik Chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T18:51:39Z","receivedAt":"2019-11-06T18:51:45Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"From: Erik Chen <erikchen@chromium.org>\n\nAdd trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n    * matching common remote and local refs\n    * marking local refs as complete (part of the matching process)\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen <erikchen@chromium.org>\n---\n fetch-pack.c | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0130b44112..f2f3365bbe 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -666,9 +666,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tstruct ref *ref;\n \tint old_save_commit_buffer = save_commit_buffer;\n \ttimestamp_t cutoff = 0;\n-\n \tsave_commit_buffer = 0;\n \n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n+\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o;\n \n@@ -690,6 +691,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\t}\n \t}\n \n+\t/* This block marks all local refs as COMPLETE, and then recursively marks all\n+\t * parents of those refs as COMPLETE.\n+\t */\n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \tif (!args->deepen) {\n \t\tfor_each_ref(mark_complete_oid, NULL);\n \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n@@ -697,6 +702,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tif (cutoff)\n \t\t\tmark_recent_complete_commits(args, cutoff);\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \n \t/*\n \t * Mark all complete remote refs as common refs.\n@@ -716,6 +722,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t}\n \n \tsave_commit_buffer = old_save_commit_buffer;\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n }\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"385615","messageId":"606756d7db834b8c16df6bad9a90c559a2fde1f3.1573066300.git.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.v2.git.1573066300.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] add whitespace","fromName":"Erik Chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T18:51:40Z","receivedAt":"2019-11-06T18:51:46Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"From: Erik Chen <erikchen@chromium.org>\n\nSigned-off-by: Erik Chen <erikchen@chromium.org>\n---\n fetch-pack.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f2f3365bbe..5e3eee0477 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -666,6 +666,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \tstruct ref *ref;\n \tint old_save_commit_buffer = save_commit_buffer;\n \ttimestamp_t cutoff = 0;\n+\n \tsave_commit_buffer = 0;\n \n \ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n-- \ngitgitgadget\n"},{"id":"385616","messageId":"pull.451.v2.git.1573066300.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.git.1572981981.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] fetch: add trace2 instrumentation","fromName":"erik chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T18:51:38Z","receivedAt":"2019-11-06T18:51:48Z","isPatch":true,"sender":{"key":"name:erik chen","avatar":null},"body":"Add trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n* matching common remote and local refs\n* marking local refs as complete (part of the matching process)\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen erikchen@chromium.org [erikchen@chromium.org]\n\nErik Chen (2):\n  fetch: add trace2 instrumentation\n  add whitespace\n\n fetch-pack.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-451%2Ferikchen%2Ftest12-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-451/erikchen/test12-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/451\n\nRange-diff vs v1:\n\n 1:  4fdbb9f504 = 1:  4fdbb9f504 fetch: add trace2 instrumentation\n -:  ---------- > 2:  606756d7db add whitespace\n\n-- \ngitgitgadget\n"},{"id":"385620","messageId":"364c526a5d674e3f137578b24143678256601d08.1573069152.git.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.v3.git.1573069152.gitgitgadget@gmail.com","subject":"[PATCH v3 1/1] fetch: add trace2 instrumentation","fromName":"Erik Chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T19:39:12Z","receivedAt":"2019-11-06T19:39:16Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"From: Erik Chen <erikchen@chromium.org>\n\nAdd trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n    * matching common remote and local refs\n    * marking local refs as complete (part of the matching process)\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen <erikchen@chromium.org>\n---\n fetch-pack.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 0130b44112..5e3eee0477 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -669,6 +669,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \n \tsave_commit_buffer = 0;\n \n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n+\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o;\n \n@@ -690,6 +692,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\t}\n \t}\n \n+\t/* This block marks all local refs as COMPLETE, and then recursively marks all\n+\t * parents of those refs as COMPLETE.\n+\t */\n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \tif (!args->deepen) {\n \t\tfor_each_ref(mark_complete_oid, NULL);\n \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n@@ -697,6 +703,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tif (cutoff)\n \t\t\tmark_recent_complete_commits(args, cutoff);\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \n \t/*\n \t * Mark all complete remote refs as common refs.\n@@ -716,6 +723,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t}\n \n \tsave_commit_buffer = old_save_commit_buffer;\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n }\n \n /*\n-- \ngitgitgadget\n"},{"id":"385619","messageId":"pull.451.v3.git.1573069152.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.v2.git.1573066300.gitgitgadget@gmail.com","subject":"[PATCH v3 0/1] fetch: add trace2 instrumentation","fromName":"erik chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-06T19:39:11Z","receivedAt":"2019-11-06T19:39:18Z","isPatch":true,"sender":{"key":"name:erik chen","avatar":null},"body":"Add trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n* matching common remote and local refs\n* marking local refs as complete (part of the matching process)\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen erikchen@chromium.org [erikchen@chromium.org]\n\nErik Chen (1):\n  fetch: add trace2 instrumentation\n\n fetch-pack.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\n\nbase-commit: 566a1439f6f56c2171b8853ddbca0ad3f5098770\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-451%2Ferikchen%2Ftest12-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-451/erikchen/test12-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/451\n\nRange-diff vs v2:\n\n 1:  4fdbb9f504 ! 1:  364c526a5d fetch: add trace2 instrumentation\n     @@ -16,10 +16,7 @@\n       --- a/fetch-pack.c\n       +++ b/fetch-pack.c\n      @@\n     - \tstruct ref *ref;\n     - \tint old_save_commit_buffer = save_commit_buffer;\n     - \ttimestamp_t cutoff = 0;\n     --\n     + \n       \tsave_commit_buffer = 0;\n       \n      +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n 2:  606756d7db < -:  ---------- add whitespace\n\n-- \ngitgitgadget\n"},{"id":"385655","messageId":"xmqqd0e4s1m2.fsf@gitster-ct.c.googlers.com","threadId":"52194","inReplyTo":"pull.451.v3.git.1573069152.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/1] fetch: add trace2 instrumentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-07T05:21:25Z","receivedAt":"2019-11-07T05:21:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"erik chen via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Add trace2 regions to fetch-pack.c to better track time spent in the various\n> phases of a fetch:\n>\n> * matching common remote and local refs\n> * marking local refs as complete (part of the matching process)\n>\n> Both of these stages can be slow for repositories with many refs.\n>\n> Signed-off-by: Erik Chen erikchen@chromium.org [erikchen@chromium.org]\n\nIf the above is verbatim copy of what you wrote in 1/1, that is very\nmuch unappreciated X-<.  A cover letter for a topic serves primarily\njust one purpose:\n\n   It is a good place to present a birds-eye-view of a multi-patch\n   topic; a high level description of the problem (e.g. how the\n   issue manifests to the end users), an explanation of division of\n   the problem into subproblems you made (if applicable), and\n   interesting highlights of the solution would all be good things\n   to have in there.\n\nAnd as a topic goes through iterations, it gives you a good place to\nsummarize what changed since the previously reviewed iterations.  It\ncould be just a single liner \"addressed all the review comments for\nthe previous iteration\".  A well-written multi-patch topic also uses\nthe same after-three-dash technique used for a single-patch topic\n(see below) to summarize what changed since the corresponding patch\nin the series in the previous iteration (or just says \"no changes\nsince the previous round\"---that helps the reviewers a lot).\n   \nFor a single-patch topic, there is no place for \"here is an overall\nbirds-eyes-view picture because the changes described in the\nproposed log message of individual patches are so big and complex\".\nA single-patch topic has one patch, that solves one problem and only\nthat problem well, so it should not need such a summary.  \n\nWhen you want to summarize the changes since the previous iteration,\nyou would write it between the three-dash-line (which appears after\nyour sign-off) and the diffstat.\n\nThanks.\n"},{"id":"385656","messageId":"xmqq5zjws12y.fsf@gitster-ct.c.googlers.com","threadId":"52194","inReplyTo":"364c526a5d674e3f137578b24143678256601d08.1573069152.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/1] fetch: add trace2 instrumentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-07T05:32:53Z","receivedAt":"2019-11-07T05:33:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Erik Chen via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Erik Chen <erikchen@chromium.org>\n>\n> Add trace2 regions to fetch-pack.c to better track time spent in the various\n> phases of a fetch:\n>\n>     * matching common remote and local refs\n>     * marking local refs as complete (part of the matching process)\n>\n> Both of these stages can be slow for repositories with many refs.\n>\n> Signed-off-by: Erik Chen <erikchen@chromium.org>\n> ---\n>  fetch-pack.c | 8 ++++++++\n>  1 file changed, 8 insertions(+)\n\nOK.\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 0130b44112..5e3eee0477 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -669,6 +669,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \n>  \tsave_commit_buffer = 0;\n>  \n> +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n> +\n>  \tfor (ref = *refs; ref; ref = ref->next) {\n>  \t\tstruct object *o;\n>  \n> @@ -690,6 +692,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \t\t}\n>  \t}\n>  \n> +\t/* This block marks all local refs as COMPLETE, and then recursively marks all\n> +\t * parents of those refs as COMPLETE.\n> +\t */\n\n        /*\n         * We write our multi-line comments like this, with the\n         * slash-asterisk at the beginning and the asterisk-slash\n         * at the end on its own line.  Learn such local conventions\n         * from the existing surrounding code and imitate, which\n         * would reduce stylistic errors.\n         */\n\nWill fix-up while queuing (no need to reroll only to fix this).\n\n> +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n>  \tif (!args->deepen) {\n>  \t\tfor_each_ref(mark_complete_oid, NULL);\n>  \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n> @@ -697,6 +703,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \t\tif (cutoff)\n>  \t\t\tmark_recent_complete_commits(args, cutoff);\n>  \t}\n> +\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n>  \n>  \t/*\n>  \t * Mark all complete remote refs as common refs.\n> @@ -716,6 +723,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>  \t}\n>  \n>  \tsave_commit_buffer = old_save_commit_buffer;\n> +\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n\n\nSo this introduces a single region around the entire function body\nof mark_complete_and_common_ref(), within which only one subpart is\nalso enclosed in a nested region.  Is that because the parts inside\nthe outer region before and after the inner region are known to\nconsume negligible time?  IOW I would understand\n\n        F () {\n                <region 1 begin>\n                    <region 1.1 begin>\n                        code\n                    <region 1.1 end>\n                    <region 1.2 begin>\n                        code\n                    <region 1.2 end>\n                    <region 1.3 begin>\n                        code\n                    <region 1.3 end>\n                <region 1 end>\n        }\n\nor\n\n        F () {\n                        trivial code\n                <region 1 begin>\n                        heavy code\n                <region 1 end>\n                        trivial code\n        }\n\nbut this appears to do\n\n\n        F () {\n                <region 1 begin>\n                        code\n                    <region 1.1 begin>\n                        code\n                    <region 1.1 end>\n                        code\n                <region 1 end>\n        }\n\nwhich is somewhat puzzling.\n\nThanks.\n"},{"id":"386453","messageId":"pull.451.v4.git.1574088732.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.v3.git.1573069152.gitgitgadget@gmail.com","subject":"[PATCH v4 0/1] fetch: add trace2 instrumentation","fromName":"erik chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-18T14:52:11Z","receivedAt":"2019-11-18T14:52:17Z","isPatch":true,"sender":{"key":"name:erik chen","avatar":null},"body":"Add trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n* matching common remote and local refs\n* marking local refs as complete (part of the matching process)\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen erikchen@chromium.org [erikchen@chromium.org]\n\nErik Chen (1):\n  fetch: add trace2 instrumentation\n\n fetch-pack.c | 16 +++++++++++++++-\n 1 file changed, 15 insertions(+), 1 deletion(-)\n\n\nbase-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-451%2Ferikchen%2Ftest12-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-451/erikchen/test12-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/451\n\nRange-diff vs v3:\n\n 1:  364c526a5d ! 1:  508d07a3eb fetch: add trace2 instrumentation\n     @@ -6,7 +6,9 @@\n          phases of a fetch:\n      \n              * matching common remote and local refs\n     -        * marking local refs as complete (part of the matching process)\n     +        * parsing remote refs and finding a cutoff\n     +        * marking local refs as complete\n     +        * marking complete remote refs as common\n      \n          Both of these stages can be slow for repositories with many refs.\n      \n     @@ -21,14 +23,28 @@\n       \n      +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n      +\n     ++\ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n       \tfor (ref = *refs; ref; ref = ref->next) {\n       \t\tstruct object *o;\n       \n      @@\n     + \t\tif (!o)\n     + \t\t\tcontinue;\n     + \n     +-\t\t/* We already have it -- which may mean that we were\n     ++\t\t/*\n     ++\t\t * We already have it -- which may mean that we were\n     + \t\t * in sync with the other side at some time after\n     + \t\t * that (it is OK if we guess wrong here).\n     + \t\t */\n     +@@\n     + \t\t\t\tcutoff = commit->date;\n       \t\t}\n       \t}\n     ++\ttrace2_region_leave(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n       \n     -+\t/* This block marks all local refs as COMPLETE, and then recursively marks all\n     ++\t/*\n     ++\t * This block marks all local refs as COMPLETE, and then recursively marks all\n      +\t * parents of those refs as COMPLETE.\n      +\t */\n      +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n     @@ -43,8 +59,17 @@\n       \n       \t/*\n       \t * Mark all complete remote refs as common refs.\n     + \t * Don't mark them common yet; the server has to be told so first.\n     + \t */\n     ++\ttrace2_region_enter(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n     + \tfor (ref = *refs; ref; ref = ref->next) {\n     + \t\tstruct object *o = deref_tag(the_repository,\n     + \t\t\t\t\t     lookup_object(the_repository,\n      @@\n     + \t\tnegotiator->known_common(negotiator,\n     + \t\t\t\t\t (struct commit *)o);\n       \t}\n     ++\ttrace2_region_leave(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n       \n       \tsave_commit_buffer = old_save_commit_buffer;\n      +\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n\n-- \ngitgitgadget\n"},{"id":"386454","messageId":"508d07a3eb035697813af253eaad739a880124da.1574088732.git.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.v4.git.1574088732.gitgitgadget@gmail.com","subject":"[PATCH v4 1/1] fetch: add trace2 instrumentation","fromName":"Erik Chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-18T14:52:12Z","receivedAt":"2019-11-18T14:52:17Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"From: Erik Chen <erikchen@chromium.org>\n\nAdd trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n    * matching common remote and local refs\n    * parsing remote refs and finding a cutoff\n    * marking local refs as complete\n    * marking complete remote refs as common\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen <erikchen@chromium.org>\n---\n fetch-pack.c | 16 +++++++++++++++-\n 1 file changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f80e2d1149..bca70e65c8 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -669,6 +669,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \n \tsave_commit_buffer = 0;\n \n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n+\n+\ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o;\n \n@@ -679,7 +682,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tif (!o)\n \t\t\tcontinue;\n \n-\t\t/* We already have it -- which may mean that we were\n+\t\t/*\n+\t\t * We already have it -- which may mean that we were\n \t\t * in sync with the other side at some time after\n \t\t * that (it is OK if we guess wrong here).\n \t\t */\n@@ -689,7 +693,13 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\t\t\tcutoff = commit->date;\n \t\t}\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n \n+\t/*\n+\t * This block marks all local refs as COMPLETE, and then recursively marks all\n+\t * parents of those refs as COMPLETE.\n+\t */\n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \tif (!args->deepen) {\n \t\tfor_each_ref(mark_complete_oid, NULL);\n \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n@@ -697,11 +707,13 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tif (cutoff)\n \t\t\tmark_recent_complete_commits(args, cutoff);\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \n \t/*\n \t * Mark all complete remote refs as common refs.\n \t * Don't mark them common yet; the server has to be told so first.\n \t */\n+\ttrace2_region_enter(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o = deref_tag(the_repository,\n \t\t\t\t\t     lookup_object(the_repository,\n@@ -714,8 +726,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tnegotiator->known_common(negotiator,\n \t\t\t\t\t (struct commit *)o);\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n \n \tsave_commit_buffer = old_save_commit_buffer;\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n }\n \n /*\n-- \ngitgitgadget\n"},{"id":"386455","messageId":"944c956e-dd9a-1a12-5cb1-0c263ee7d5bd@gmail.com","threadId":"52194","inReplyTo":"xmqq5zjws12y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] fetch: add trace2 instrumentation","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-11-18T15:46:56Z","receivedAt":"2019-11-18T15:47:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/7/2019 12:32 AM, Junio C Hamano wrote:\n> \"Erik Chen via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Erik Chen <erikchen@chromium.org>\n>>\n>> Add trace2 regions to fetch-pack.c to better track time spent in the various\n>> phases of a fetch:\n>>\n>>     * matching common remote and local refs\n>>     * marking local refs as complete (part of the matching process)\n>>\n>> Both of these stages can be slow for repositories with many refs.\n>>\n>> Signed-off-by: Erik Chen <erikchen@chromium.org>\n>> ---\n>>  fetch-pack.c | 8 ++++++++\n>>  1 file changed, 8 insertions(+)\n> \n> OK.\n> \n>> diff --git a/fetch-pack.c b/fetch-pack.c\n>> index 0130b44112..5e3eee0477 100644\n>> --- a/fetch-pack.c\n>> +++ b/fetch-pack.c\n>> @@ -669,6 +669,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>>  \n>>  \tsave_commit_buffer = 0;\n>>  \n>> +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n>> +\n>>  \tfor (ref = *refs; ref; ref = ref->next) {\n>>  \t\tstruct object *o;\n>>  \n>> @@ -690,6 +692,10 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>>  \t\t}\n>>  \t}\n>>  \n>> +\t/* This block marks all local refs as COMPLETE, and then recursively marks all\n>> +\t * parents of those refs as COMPLETE.\n>> +\t */\n> \n>         /*\n>          * We write our multi-line comments like this, with the\n>          * slash-asterisk at the beginning and the asterisk-slash\n>          * at the end on its own line.  Learn such local conventions\n>          * from the existing surrounding code and imitate, which\n>          * would reduce stylistic errors.\n>          */\n> \n> Will fix-up while queuing (no need to reroll only to fix this).\n> \n>> +\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n>>  \tif (!args->deepen) {\n>>  \t\tfor_each_ref(mark_complete_oid, NULL);\n>>  \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n>> @@ -697,6 +703,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>>  \t\tif (cutoff)\n>>  \t\t\tmark_recent_complete_commits(args, cutoff);\n>>  \t}\n>> +\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n>>  \n>>  \t/*\n>>  \t * Mark all complete remote refs as common refs.\n>> @@ -716,6 +723,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n>>  \t}\n>>  \n>>  \tsave_commit_buffer = old_save_commit_buffer;\n>> +\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n> \n> \n> So this introduces a single region around the entire function body\n> of mark_complete_and_common_ref(), within which only one subpart is\n> also enclosed in a nested region.  Is that because the parts inside\n> the outer region before and after the inner region are known to\n> consume negligible time?  IOW I would understand\n> \n>         F () {\n>                 <region 1 begin>\n>                     <region 1.1 begin>\n>                         code\n>                     <region 1.1 end>\n>                     <region 1.2 begin>\n>                         code\n>                     <region 1.2 end>\n>                     <region 1.3 begin>\n>                         code\n>                     <region 1.3 end>\n>                 <region 1 end>\n>         }\n> \n> or\n> \n>         F () {\n>                         trivial code\n>                 <region 1 begin>\n>                         heavy code\n>                 <region 1 end>\n>                         trivial code\n>         }\n> \n> but this appears to do\n> \n> \n>         F () {\n>                 <region 1 begin>\n>                         code\n>                     <region 1.1 begin>\n>                         code\n>                     <region 1.1 end>\n>                         code\n>                 <region 1 end>\n>         }\n> \n> which is somewhat puzzling.\n\nI notice that a v4 was sent that adds more sub-regions without actually\nresponding to this request. (It is worth also pointing out that you\nignored Junio's request you use the cover letter to explain your reasoning\nfor changes between versions.)\n\nThere is a real downside to nesting regions like this. Specifically, we\nfrequently limit the depth that we report nested regions to avoid\noverwhelming the logs.\n\nIn general, these sub-regions should be avoided when possible and instead\ncreate regions around important sections, such as the second option Junio\nlists above.\n\nThanks,\n-Stolee\n"},{"id":"386497","messageId":"xmqqsgmkzl1s.fsf@gitster-ct.c.googlers.com","threadId":"52194","inReplyTo":"944c956e-dd9a-1a12-5cb1-0c263ee7d5bd@gmail.com","subject":"Re: [PATCH v3 1/1] fetch: add trace2 instrumentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-19T01:55:59Z","receivedAt":"2019-11-19T01:56:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee <stolee@gmail.com> writes:\n\n> On 11/7/2019 12:32 AM, Junio C Hamano wrote:\n>> So this introduces a single region around the entire function body\n>> of mark_complete_and_common_ref(), within which only one subpart is\n>> also enclosed in a nested region.  Is that because the parts inside\n>> the outer region before and after the inner region are known to\n>> consume negligible time?  IOW I would understand\n>> \n>>         F () {\n>> ...\n>>         }\n>> \n>> or\n>> \n>>         F () {\n>>                         trivial code\n>>                 <region 1 begin>\n>>                         heavy code\n>>                 <region 1 end>\n>>                         trivial code\n>>         }\n>> \n>> but this appears to do\n>> ...\n>> which is somewhat puzzling.\n>\n> I notice that a v4 was sent that adds more sub-regions without actually\n> responding to this request. (It is worth also pointing out that you\n> ignored Junio's request you use the cover letter to explain your reasoning\n> for changes between versions.)\n\nThanks for noticing.  I wasn't requesting any change in particular\n(at least not yet), but was inquiring the reasoning behind what was\ndone.  From that point of view, the lack of answers was worse than\nyet another patch that does not explain why it was done that other\nway this time around.\n\n> There is a real downside to nesting regions like this. Specifically, we\n> frequently limit the depth that we report nested regions to avoid\n> overwhelming the logs.\n>\n> In general, these sub-regions should be avoided when possible and instead\n> create regions around important sections, such as the second option Junio\n> lists above.\n\nThanks for a clear direction as the area expert of trace2.\n"},{"id":"386554","messageId":"8b6d4bdf-d982-97ad-e475-672e6ea914c2@chromium.org","threadId":"52194","inReplyTo":"xmqqsgmkzl1s.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] fetch: add trace2 instrumentation","fromName":"Erik Chen","fromEmail":"erikchen@chromium.org","sentAt":"2019-11-19T21:24:50Z","receivedAt":"2019-11-19T21:24:57Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"\nOn 11/18/19 5:55 PM, Junio C Hamano wrote:\n> Derrick Stolee <stolee@gmail.com> writes:\n>\n>> On 11/7/2019 12:32 AM, Junio C Hamano wrote:\n>>> So this introduces a single region around the entire function body\n>>> of mark_complete_and_common_ref(), within which only one subpart is\n>>> also enclosed in a nested region.  Is that because the parts inside\n>>> the outer region before and after the inner region are known to\n>>> consume negligible time?  IOW I would understand\n>>>\n>>>          F () {\n>>> ...\n>>>          }\n>>>\n>>> or\n>>>\n>>>          F () {\n>>>                          trivial code\n>>>                  <region 1 begin>\n>>>                          heavy code\n>>>                  <region 1 end>\n>>>                          trivial code\n>>>          }\n>>>\n>>> but this appears to do\n>>> ...\n>>> which is somewhat puzzling.\n>> I notice that a v4 was sent that adds more sub-regions without actually\n>> responding to this request. (It is worth also pointing out that you\n>> ignored Junio's request you use the cover letter to explain your reasoning\n>> for changes between versions.)\n> Thanks for noticing.  I wasn't requesting any change in particular\n> (at least not yet), but was inquiring the reasoning behind what was\n> done.  From that point of view, the lack of answers was worse than\n> yet another patch that does not explain why it was done that other\n> way this time around.\n\nSorry, I've been replying on the GitGitGadget pull request thread:\n\ne.g. https://github.com/gitgitgadget/git/pull/451#issuecomment-555044068\n\n\nBut none of those replies have been making their way to this mailing \nlist. I'll attempt to send these replies to the mailing list now.\n\n\n>\n>> There is a real downside to nesting regions like this. Specifically, we\n>> frequently limit the depth that we report nested regions to avoid\n>> overwhelming the logs.\n>>\n>> In general, these sub-regions should be avoided when possible and instead\n>> create regions around important sections, such as the second option Junio\n>> lists above.\n> Thanks for a clear direction as the area expert of trace2.\n>\n>\n"},{"id":"386557","messageId":"54eb184e-5400-093f-a9fc-c804dd6f9c65@chromium.org","threadId":"52194","inReplyTo":"xmqqd0e4s1m2.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 0/1] fetch: add trace2 instrumentation","fromName":"Erik Chen","fromEmail":"erikchen@chromium.org","sentAt":"2019-11-19T21:44:44Z","receivedAt":"2019-11-19T21:44:48Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"Importing response from \nhttps://github.com/gitgitgadget/git/pull/451#issuecomment-555044068\n\nOn 11/6/19 9:21 PM, Junio C Hamano wrote:\n> \"erik chen via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> Add trace2 regions to fetch-pack.c to better track time spent in the various\n>> phases of a fetch:\n>>\n>> * matching common remote and local refs\n>> * marking local refs as complete (part of the matching process)\n>>\n>> Both of these stages can be slow for repositories with many refs.\n>>\n>> Signed-off-by: Erik Chen erikchen@chromium.org [erikchen@chromium.org]\n> \n> If the above is verbatim copy of what you wrote in 1/1, that is very\n> much unappreciated X-<.  A cover letter for a topic serves primarily\n> just one purpose:\n> \n>     It is a good place to present a birds-eye-view of a multi-patch\n>     topic; a high level description of the problem (e.g. how the\n>     issue manifests to the end users), an explanation of division of\n>     the problem into subproblems you made (if applicable), and\n>     interesting highlights of the solution would all be good things\n>     to have in there.\n> \n> And as a topic goes through iterations, it gives you a good place to\n> summarize what changed since the previously reviewed iterations.  It\n> could be just a single liner \"addressed all the review comments for\n> the previous iteration\".  A well-written multi-patch topic also uses\n> the same after-three-dash technique used for a single-patch topic\n> (see below) to summarize what changed since the corresponding patch\n> in the series in the previous iteration (or just says \"no changes\n> since the previous round\"---that helps the reviewers a lot).\n>     \n> For a single-patch topic, there is no place for \"here is an overall\n> birds-eyes-view picture because the changes described in the\n> proposed log message of individual patches are so big and complex\".\n> A single-patch topic has one patch, that solves one problem and only\n> that problem well, so it should not need such a summary.\n> \n> When you want to summarize the changes since the previous iteration,\n> you would write it between the three-dash-line (which appears after\n> your sign-off) and the diffstat.\n> \n> Thanks.\n> \n> \nSorry about that, this is my first time contributing to git, and I'm \nusing GitGitGadget. I believe it's too late to fix this now (?)\n"},{"id":"386558","messageId":"383762c9-06eb-707c-c0d8-c8ec8757133e@chromium.org","threadId":"52194","inReplyTo":"xmqq5zjws12y.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] fetch: add trace2 instrumentation","fromName":"Erik Chen","fromEmail":"erikchen@chromium.org","sentAt":"2019-11-19T21:47:26Z","receivedAt":"2019-11-19T21:47:30Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"Importing response from \nhttps://github.com/gitgitgadget/git/pull/451#issuecomment-555044068\n\n> \n>          /*\n>           * We write our multi-line comments like this, with the\n>           * slash-asterisk at the beginning and the asterisk-slash\n>           * at the end on its own line.  Learn such local conventions\n>           * from the existing surrounding code and imitate, which\n>           * would reduce stylistic errors.\n>           */\n> \n> Will fix-up while queuing (no need to reroll only to fix this).\n\nSorry about that. I was copying from line 682, which looks like it was \nalso using the wrong style. I've fixed that line as well.\n\n> So this introduces a single region around the entire function body\n> of mark_complete_and_common_ref(), within which only one subpart is\n> also enclosed in a nested region.  Is that because the parts inside\n> the outer region before and after the inner region are known to\n> consume negligible time?  IOW I would understand\nGood point. I used this structure because I observed locally that the \nmiddle block was slow, but was concerned that the initial/ending block \nmight be slow as well. I've gone ahead and adding tracing regions for \neach block.\n\n"},{"id":"386559","messageId":"c2cd57ed-8075-bdd3-67f0-21187580270a@chromium.org","threadId":"52194","inReplyTo":"944c956e-dd9a-1a12-5cb1-0c263ee7d5bd@gmail.com","subject":"Re: [PATCH v3 1/1] fetch: add trace2 instrumentation","fromName":"Erik Chen","fromEmail":"erikchen@chromium.org","sentAt":"2019-11-19T21:51:37Z","receivedAt":"2019-11-19T21:51:41Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"Importing response from \nhttps://github.com/gitgitgadget/git/pull/451#issuecomment-555077933\n\n> I notice that a v4 was sent that adds more sub-regions without actually\n> responding to this request. \n\nSorry, did I misunderstand the request? I was trying to implement the \nsuggestion:\n<Junio's first suggestion>\n\n > (It is worth also pointing out that you\n > ignored Junio's request you use the cover letter to explain your \nreasoning\n > for changes between versions.)\nSorry, this was not intentional. I'm using GitGitGadget, and it wasn't \nclear to me how to change this. See my comment here:\n<https://github.com/gitgitgadget/git/pull/451#issuecomment-555044068>\n\n\n> \n> There is a real downside to nesting regions like this. Specifically, we\n> frequently limit the depth that we report nested regions to avoid\n> overwhelming the logs.\n> \n> In general, these sub-regions should be avoided when possible and instead\n> create regions around important sections, such as the second option Junio\n> lists above.\n\nI thought it would be nice to have an encapsulating region for the full \nfunction, but I can switch to Junio's second option if that's \npreferable. It wasn't clear to me that that was preferred over the first \noption?\n"},{"id":"386562","messageId":"261fae86-c909-16a5-b7b9-3ab48b5680a8@chromium.org","threadId":"52194","inReplyTo":"xmqqsgmkzl1s.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] fetch: add trace2 instrumentation","fromName":"Erik Chen","fromEmail":"erikchen@chromium.org","sentAt":"2019-11-19T22:57:49Z","receivedAt":"2019-11-19T22:57:55Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"\n>> In general, these sub-regions should be avoided when possible and instead\n>> create regions around important sections, such as the second option Junio\n>> lists above.\n> \n> Thanks for a clear direction as the area expert of trace2.\nI'll upload a new patch set to do that.\n"},{"id":"386563","messageId":"pull.451.v5.git.1574204529.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.v4.git.1574088732.gitgitgadget@gmail.com","subject":"[PATCH v5 0/1] fetch: add trace2 instrumentation","fromName":"erik chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-19T23:02:08Z","receivedAt":"2019-11-19T23:02:14Z","isPatch":true,"sender":{"key":"name:erik chen","avatar":null},"body":"Add trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n* matching common remote and local refs\n* marking local refs as complete (part of the matching process)\n\nBoth of these stages can be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen erikchen@chromium.org [erikchen@chromium.org]\n\nErik Chen (1):\n  fetch: add trace2 instrumentation\n\n fetch-pack.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\n\nbase-commit: d9f6f3b6195a0ca35642561e530798ad1469bd41\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-451%2Ferikchen%2Ftest12-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-451/erikchen/test12-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/451\n\nRange-diff vs v4:\n\n 1:  508d07a3eb ! 1:  d7bf1849ce fetch: add trace2 instrumentation\n     @@ -5,12 +5,11 @@\n          Add trace2 regions to fetch-pack.c to better track time spent in the various\n          phases of a fetch:\n      \n     -        * matching common remote and local refs\n              * parsing remote refs and finding a cutoff\n              * marking local refs as complete\n              * marking complete remote refs as common\n      \n     -    Both of these stages can be slow for repositories with many refs.\n     +    All stages could potentially be slow for repositories with many refs.\n      \n          Signed-off-by: Erik Chen <erikchen@chromium.org>\n      \n     @@ -21,8 +20,6 @@\n       \n       \tsave_commit_buffer = 0;\n       \n     -+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n     -+\n      +\ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n       \tfor (ref = *refs; ref; ref = ref->next) {\n       \t\tstruct object *o;\n     @@ -72,7 +69,4 @@\n      +\ttrace2_region_leave(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n       \n       \tsave_commit_buffer = old_save_commit_buffer;\n     -+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_and_common_ref\", NULL);\n       }\n     - \n     - /*\n\n-- \ngitgitgadget\n"},{"id":"386564","messageId":"d7bf1849ce8430beed3619fb9d87b58746424a54.1574204529.git.gitgitgadget@gmail.com","threadId":"52194","inReplyTo":"pull.451.v5.git.1574204529.gitgitgadget@gmail.com","subject":"[PATCH v5 1/1] fetch: add trace2 instrumentation","fromName":"Erik Chen via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2019-11-19T23:02:09Z","receivedAt":"2019-11-19T23:02:15Z","isPatch":true,"sender":{"key":"erikchen@chromium.org","avatar":"https://avatars.githubusercontent.com/u/833296?v=4"},"body":"From: Erik Chen <erikchen@chromium.org>\n\nAdd trace2 regions to fetch-pack.c to better track time spent in the various\nphases of a fetch:\n\n    * parsing remote refs and finding a cutoff\n    * marking local refs as complete\n    * marking complete remote refs as common\n\nAll stages could potentially be slow for repositories with many refs.\n\nSigned-off-by: Erik Chen <erikchen@chromium.org>\n---\n fetch-pack.c | 13 ++++++++++++-\n 1 file changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f80e2d1149..a4a5e6cf9c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -669,6 +669,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \n \tsave_commit_buffer = 0;\n \n+\ttrace2_region_enter(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o;\n \n@@ -679,7 +680,8 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tif (!o)\n \t\t\tcontinue;\n \n-\t\t/* We already have it -- which may mean that we were\n+\t\t/*\n+\t\t * We already have it -- which may mean that we were\n \t\t * in sync with the other side at some time after\n \t\t * that (it is OK if we guess wrong here).\n \t\t */\n@@ -689,7 +691,13 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\t\t\tcutoff = commit->date;\n \t\t}\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"parse_remote_refs_and_find_cutoff\", NULL);\n \n+\t/*\n+\t * This block marks all local refs as COMPLETE, and then recursively marks all\n+\t * parents of those refs as COMPLETE.\n+\t */\n+\ttrace2_region_enter(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \tif (!args->deepen) {\n \t\tfor_each_ref(mark_complete_oid, NULL);\n \t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n@@ -697,11 +705,13 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tif (cutoff)\n \t\t\tmark_recent_complete_commits(args, cutoff);\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_complete_local_refs\", NULL);\n \n \t/*\n \t * Mark all complete remote refs as common refs.\n \t * Don't mark them common yet; the server has to be told so first.\n \t */\n+\ttrace2_region_enter(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n \tfor (ref = *refs; ref; ref = ref->next) {\n \t\tstruct object *o = deref_tag(the_repository,\n \t\t\t\t\t     lookup_object(the_repository,\n@@ -714,6 +724,7 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \t\tnegotiator->known_common(negotiator,\n \t\t\t\t\t (struct commit *)o);\n \t}\n+\ttrace2_region_leave(\"fetch-pack\", \"mark_common_remote_refs\", NULL);\n \n \tsave_commit_buffer = old_save_commit_buffer;\n }\n-- \ngitgitgadget\n"},{"id":"386596","messageId":"xmqqr223e4nq.fsf@gitster-ct.c.googlers.com","threadId":"52194","inReplyTo":"d7bf1849ce8430beed3619fb9d87b58746424a54.1574204529.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 1/1] fetch: add trace2 instrumentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-20T01:07:53Z","receivedAt":"2019-11-20T01:07:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Erik Chen via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Erik Chen <erikchen@chromium.org>\n>\n> Add trace2 regions to fetch-pack.c to better track time spent in the various\n> phases of a fetch:\n>\n>     * parsing remote refs and finding a cutoff\n>     * marking local refs as complete\n>     * marking complete remote refs as common\n>\n> All stages could potentially be slow for repositories with many refs.\n>\n> Signed-off-by: Erik Chen <erikchen@chromium.org>\n> ---\n>  fetch-pack.c | 13 ++++++++++++-\n>  1 file changed, 12 insertions(+), 1 deletion(-)\n\nThanks, will queue.\n"}]}