{"thread":{"id":"53455","subject":"[PATCH] trace2: log progress time and throughput","startedAt":"2020-05-12T21:44:35Z","lastAt":"2021-06-23T03:43:04Z","messageCount":17,"participants":["Emily Shaffer","Junio C Hamano","Josh Steadmon","Derrick Stolee","Jeff Hostetler","Ævar Arnfjörð Bjarmason","Elijah Newren","Taylor Blau","Chris Torek"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"397634","messageId":"20200512214420.36329-1-emilyshaffer@google.com","threadId":"53455","inReplyTo":null,"subject":"[PATCH] trace2: log progress time and throughput","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2020-05-12T21:44:20Z","receivedAt":"2020-05-12T21:44:35Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Rather than teaching only one operation, like 'git fetch', how to write\ndown throughput to traces, we can learn about a wide range of user\noperations that may seem slow by adding tooling to the progress library\nitself. Operations which display progress are likely to be slow-running\nand the kind of thing we want to monitor for performance anyways. By\nshowing object counts and data transfer size, we should be able to\nmake some derived measurements to ensure operations are scaling the way\nwe expect.\n\nSigned-off-by: Emily Shaffer <emilyshaffer@google.com>\n---\nOne note: by putting trace collection into the progress library, we end\nup with data events which have titles like \"Receiving objects\" - not\nvery machine-parseable. An alternative might be to ask for a\nmachine-readable title in the progress struct, but I didn't think it was\nworth the code churn. However, I don't have experience with processing\nthe trace data after it's been collected, so if this is a bigger problem\nthan I think, please say so and I'll figure something out.\n\nCI run here, although it failed on the same error Junio noted today[1]:\nhttps://github.com/nasamuffin/git/runs/668457062\n\n - Emily\n\n[1]: https://lore.kernel.org/git/xmqqtv0kc2q1.fsf@gitster.c.googlers.com\n\n progress.c                  | 17 +++++++++++++++++\n t/t0500-progress-display.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+)\n\ndiff --git a/progress.c b/progress.c\nindex 75633e9c5e..6d2dcff0b6 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -265,6 +265,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n \tprogress->title_len = utf8_strwidth(title);\n \tprogress->split = 0;\n \tset_progress_signal();\n+\ttrace2_region_enter(\"progress\", title, the_repository);\n \treturn progress;\n }\n \n@@ -320,6 +321,22 @@ void stop_progress(struct progress **p_progress)\n {\n \tfinish_if_sparse(*p_progress);\n \n+\tif (p_progress && *p_progress) {\n+\t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n+\t\t\t\t   (*p_progress)->total);\n+\n+\t\tif ((*p_progress)->throughput)\n+\t\t\ttrace2_data_intmax(\"progress\", the_repository,\n+\t\t\t\t\t   \"total_bytes\",\n+\t\t\t\t\t   (*p_progress)->throughput->curr_total);\n+\t}\n+\n+\ttrace2_region_leave(\"progress\",\n+\t\t\t    p_progress && *p_progress\n+\t\t\t\t? (*p_progress)->title\n+\t\t\t\t: NULL,\n+\t\t\t    the_repository);\n+\n \tstop_progress_msg(p_progress, _(\"done\"));\n }\n \ndiff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\nindex d2d088d9a0..1ed1df351c 100755\n--- a/t/t0500-progress-display.sh\n+++ b/t/t0500-progress-display.sh\n@@ -283,4 +283,30 @@ test_expect_success 'cover up after throughput shortens a lot' '\n \ttest_i18ncmp expect out\n '\n \n+test_expect_success 'progress generates traces' '\n+\tcat >in <<-\\EOF &&\n+\tthroughput 102400 1000\n+\tupdate\n+\tprogress 10\n+\tthroughput 204800 2000\n+\tupdate\n+\tprogress 20\n+\tthroughput 307200 3000\n+\tupdate\n+\tprogress 30\n+\tthroughput 409600 4000\n+\tupdate\n+\tprogress 40\n+\tEOF\n+\n+\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress --total=40 \\\n+\t\t\"Working hard\" <in 2>stderr &&\n+\n+\t# t0212/parse_events.perl intentionally omits regions and data.\n+\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n+\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n+\tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n+\tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n+'\n+\n test_done\n-- \n2.26.2.645.ge9eca65c58-goog\n\n"},{"id":"397635","messageId":"xmqqr1voajii.fsf@gitster.c.googlers.com","threadId":"53455","inReplyTo":"20200512214420.36329-1-emilyshaffer@google.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-12T22:14:13Z","receivedAt":"2020-05-12T22:14:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Emily Shaffer <emilyshaffer@google.com> writes:\n\n> Rather than teaching only one operation, like 'git fetch', how to write\n> down throughput to traces, we can learn about a wide range of user\n> operations that may seem slow by adding tooling to the progress library\n> itself. Operations which display progress are likely to be slow-running\n> and the kind of thing we want to monitor for performance anyways. By\n> showing object counts and data transfer size, we should be able to\n> make some derived measurements to ensure operations are scaling the way\n> we expect.\n\nExcellent observation ;-)\n\n>\n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n> One note: by putting trace collection into the progress library, we end\n> up with data events which have titles like \"Receiving objects\" - not\n> very machine-parseable. An alternative might be to ask for a\n> machine-readable title in the progress struct, but I didn't think it was\n> worth the code churn.\n\nPerhaps make it an optional member of the struct, and convert only a\nselected few callers to set it and see how well it helps, while\nleaving all the other callers set NULL to the member to keep showing\nnot-very-useful titles?\n\n> CI run here, although it failed on the same error Junio noted today[1]:\n\nI think Dscho's latest patch, even though it didn't mention my message,\nis to fix that error.  So with that queued in the same batch, we'll\nsee a full CI coverage again.\n\nWill queue.  Thanks.\n\n"},{"id":"397748","messageId":"20200513194604.GA163566@google.com","threadId":"53455","inReplyTo":"20200512214420.36329-1-emilyshaffer@google.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-05-13T19:46:04Z","receivedAt":"2020-05-13T19:46:12Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.05.12 14:44, Emily Shaffer wrote:\n> Rather than teaching only one operation, like 'git fetch', how to write\n> down throughput to traces, we can learn about a wide range of user\n> operations that may seem slow by adding tooling to the progress library\n> itself. Operations which display progress are likely to be slow-running\n> and the kind of thing we want to monitor for performance anyways. By\n> showing object counts and data transfer size, we should be able to\n> make some derived measurements to ensure operations are scaling the way\n> we expect.\n> \n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n> One note: by putting trace collection into the progress library, we end\n> up with data events which have titles like \"Receiving objects\" - not\n> very machine-parseable. An alternative might be to ask for a\n> machine-readable title in the progress struct, but I didn't think it was\n> worth the code churn. However, I don't have experience with processing\n> the trace data after it's been collected, so if this is a bigger problem\n> than I think, please say so and I'll figure something out.\n> \n> CI run here, although it failed on the same error Junio noted today[1]:\n> https://github.com/nasamuffin/git/runs/668457062\n> \n>  - Emily\n> \n> [1]: https://lore.kernel.org/git/xmqqtv0kc2q1.fsf@gitster.c.googlers.com\n\n\nI like Junio's idea of adding an optional machine-readable field in the\nprogress struct, but I don't think it is necessarily a blocker for this\nchange. Everything looks good to me.\n\nReviewed-by: Josh Steadmon <steadmon@google.com>\n"},{"id":"397870","messageId":"8f159f13-ed61-61ea-8e9a-c1ffbc5fddb3@gmail.com","threadId":"53455","inReplyTo":"20200512214420.36329-1-emilyshaffer@google.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-15T10:59:33Z","receivedAt":"2020-05-15T10:59:39Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/12/2020 5:44 PM, Emily Shaffer wrote:\n> Rather than teaching only one operation, like 'git fetch', how to write\n> down throughput to traces, we can learn about a wide range of user\n> operations that may seem slow by adding tooling to the progress library\n> itself. Operations which display progress are likely to be slow-running\n> and the kind of thing we want to monitor for performance anyways. By\n> showing object counts and data transfer size, we should be able to\n> make some derived measurements to ensure operations are scaling the way\n> we expect.\n> \n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> ---\n> One note: by putting trace collection into the progress library, we end\n> up with data events which have titles like \"Receiving objects\" - not\n> very machine-parseable. An alternative might be to ask for a\n> machine-readable title in the progress struct, but I didn't think it was\n> worth the code churn. However, I don't have experience with processing\n> the trace data after it's been collected, so if this is a bigger problem\n> than I think, please say so and I'll figure something out.\n> \n> CI run here, although it failed on the same error Junio noted today[1]:\n> https://github.com/nasamuffin/git/runs/668457062\n> \n>  - Emily\n> \n> [1]: https://lore.kernel.org/git/xmqqtv0kc2q1.fsf@gitster.c.googlers.com\n> \n>  progress.c                  | 17 +++++++++++++++++\n>  t/t0500-progress-display.sh | 26 ++++++++++++++++++++++++++\n>  2 files changed, 43 insertions(+)\n> \n> diff --git a/progress.c b/progress.c\n> index 75633e9c5e..6d2dcff0b6 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -265,6 +265,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n>  \tprogress->title_len = utf8_strwidth(title);\n>  \tprogress->split = 0;\n>  \tset_progress_signal();\n> +\ttrace2_region_enter(\"progress\", title, the_repository);\n>  \treturn progress;\n>  }\n>  \n> @@ -320,6 +321,22 @@ void stop_progress(struct progress **p_progress)\n>  {\n>  \tfinish_if_sparse(*p_progress);\n>  \n> +\tif (p_progress && *p_progress) {\n> +\t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n> +\t\t\t\t   (*p_progress)->total);\n\nShould this be \"total_objects\"? Progress can iterate over lots of things,\nsuch as cache entries. Perhaps leave it as \"total\"?\n\n> +\n> +\t\tif ((*p_progress)->throughput)\n> +\t\t\ttrace2_data_intmax(\"progress\", the_repository,\n> +\t\t\t\t\t   \"total_bytes\",\n> +\t\t\t\t\t   (*p_progress)->throughput->curr_total);\n\nI like the extra detail here for the specific kind of progress used in\nnetwork transfer.\n\n> +\t}\n> +\n> +\ttrace2_region_leave(\"progress\",\n> +\t\t\t    p_progress && *p_progress\n> +\t\t\t\t? (*p_progress)->title\n> +\t\t\t\t: NULL,\n> +\t\t\t    the_repository);\n> +\n>  \tstop_progress_msg(p_progress, _(\"done\"));\n>  }\n\nThis trace2_region_leave() needs to be conditional on the progress\nbeing non-null. The pattern used by consumers of the progress API is:\n\n\tif (my_progress_condition)\n\t\tstart_progress(&progress);\n\n\tdo {\n\t\tdisplay_progress(&progress, count);\n\t} while (condition);\n\n\tstop_progress(&progress);\n\nThe condition to show progress or not is conditional on an option that\nis external to the progress API.\n\nThe fix for this patch is simple: make the trace2_region_leave() be\nconditional on \"p_progress && *p_progress\".\n\nThis leads to an extra problem: if a user uses an option such as \"--quiet\",\nthen the trace2 regions won't appear at all. This becomes even more important\nwhen thinking about scripts or tools that have stderr as a non-TTY, which\ndisables progress most of the time.\n\nIt's best to have trace2 data be consistent across commands. I think this can\nbe accomplished, but it is a more invasive change to the rest of the codebase.\nIt requires invoking the progress API in all cases, and having the progress\nAPI conditionally initialize the progress struct. The new pattern would look\nlike this:\n\n\tstart_progress(&progress, my_progress_condition);\n\n\tdo {\n\t\tdisplay_progress(&progress, count);\n\t} while (condition);\n\n\tstop_progress(&progress);\n\nThen, start_progress() (and variants) could always start the trace2 region,\nand stop_progress() could always end the trace2 region.\n\n>  \n> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\n> index d2d088d9a0..1ed1df351c 100755\n> --- a/t/t0500-progress-display.sh\n> +++ b/t/t0500-progress-display.sh\n> @@ -283,4 +283,30 @@ test_expect_success 'cover up after throughput shortens a lot' '\n>  \ttest_i18ncmp expect out\n>  '\n>  \n> +test_expect_success 'progress generates traces' '\n> +\tcat >in <<-\\EOF &&\n> +\tthroughput 102400 1000\n> +\tupdate\n> +\tprogress 10\n> +\tthroughput 204800 2000\n> +\tupdate\n> +\tprogress 20\n> +\tthroughput 307200 3000\n> +\tupdate\n> +\tprogress 30\n> +\tthroughput 409600 4000\n> +\tupdate\n> +\tprogress 40\n> +\tEOF\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress --total=40 \\\n> +\t\t\"Working hard\" <in 2>stderr &&\n> +\n> +\t# t0212/parse_events.perl intentionally omits regions and data.\n> +\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> +\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> +\tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n> +\tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n> +'\n> +\n\nThanks for the test! While this is sufficient to test the trace2 in the\nhappy path, I think that if you run a Git command such as `git commit-graph write`\nthat automatically quiets progress in the test suite (non-TTY stderr) you will\nfind the trace2 logs malformed due to excess end regions.\n\nThanks,\n-Stolee\n"},{"id":"397885","messageId":"xmqqzha9md5d.fsf@gitster.c.googlers.com","threadId":"53455","inReplyTo":"8f159f13-ed61-61ea-8e9a-c1ffbc5fddb3@gmail.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-15T15:27:42Z","receivedAt":"2020-05-15T15:27:48Z","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>> +\tif (p_progress && *p_progress) {\n>> +\t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n>> +\t\t\t\t   (*p_progress)->total);\n>\n> Should this be \"total_objects\"? Progress can iterate over lots of things,\n> such as cache entries. Perhaps leave it as \"total\"?\n\nGood point.  \n\nI think the literal strings like \"total_objects\" we see in this\npatch can later be customized in the same future change that lets us\ngive a more useful output than \"Receiving objects\" by having a\nhandful of customizable strings in the progress struct as hinted by\nEmily under the three-dash line.  So I do not mind leaving it as-is.\nIf a reroll has to come without making these strings customizable,\nit may be an improvement to change it to \"total\" if we do not forget,\nbut as long as we know where we are going in the longer term, I do\nnot think it is a big deal.\n\n>> +\n>> +\t\tif ((*p_progress)->throughput)\n>> +\t\t\ttrace2_data_intmax(\"progress\", the_repository,\n>> +\t\t\t\t\t   \"total_bytes\",\n>> +\t\t\t\t\t   (*p_progress)->throughput->curr_total);\n>\n> I like the extra detail here for the specific kind of progress used in\n> network transfer.\n\nThe curr_total field must be counted in bytes, by the fact that\nstrbuf_humanise_bytes() is called by throughput_string() to show it,\nso \"total_bytes\" does make perfect sense.  The field might want to\nget renamed to reflect this but obviously that's the kind of change\nthat we would not want in the middle of a more meaningful change\nlike this one.\n\n>> +\t}\n>> +\n>> +\ttrace2_region_leave(\"progress\",\n>> +\t\t\t    p_progress && *p_progress\n>> +\t\t\t\t? (*p_progress)->title\n>> +\t\t\t\t: NULL,\n>> +\t\t\t    the_repository);\n>> +\n>>  \tstop_progress_msg(p_progress, _(\"done\"));\n>>  }\n>\n> This trace2_region_leave() needs to be conditional on the progress\n> being non-null. The pattern used by consumers of the progress API is:\n>\n> \tif (my_progress_condition)\n> \t\tstart_progress(&progress);\n>\n> \tdo {\n> \t\tdisplay_progress(&progress, count);\n> \t} while (condition);\n>\n> \tstop_progress(&progress);\n>\n> The condition to show progress or not is conditional on an option that\n> is external to the progress API.\n>\n> The fix for this patch is simple: make the trace2_region_leave() be\n> conditional on \"p_progress && *p_progress\".\n\nMakes sense.\n\n> This leads to an extra problem: if a user uses an option such as \"--quiet\",\n> then the trace2 regions won't appear at all. This becomes even more important\n> when thinking about scripts or tools that have stderr as a non-TTY, which\n> disables progress most of the time.\n>\n> It's best to have trace2 data be consistent across commands. I think this can\n> be accomplished, but it is a more invasive change to the rest of the codebase.\n\nTrue, because \"--quiet\" means we cannot piggyback on the progress code.\n\n> It requires invoking the progress API in all cases, and having the progress\n> API conditionally initialize the progress struct. The new pattern would look\n> like this:\n>\n> \tstart_progress(&progress, my_progress_condition);\n>\n> \tdo {\n> \t\tdisplay_progress(&progress, count);\n> \t} while (condition);\n>\n> \tstop_progress(&progress);\n>\n> Then, start_progress() (and variants) could always start the trace2 region,\n> and stop_progress() could always end the trace2 region.\n\nOK.  Good analysis.\n\nThanks.\n\n"},{"id":"397891","messageId":"xmqqtv0hmb7r.fsf@gitster.c.googlers.com","threadId":"53455","inReplyTo":"xmqqzha9md5d.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-15T16:09:28Z","receivedAt":"2020-05-15T16:09:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Derrick Stolee <stolee@gmail.com> writes:\n> ...\n>> This trace2_region_leave() needs to be conditional on the progress\n>> being non-null. The pattern used by consumers of the progress API is:\n>>\n>> \tif (my_progress_condition)\n>> \t\tstart_progress(&progress);\n>>\n>> \tdo {\n>> \t\tdisplay_progress(&progress, count);\n>> \t} while (condition);\n>>\n>> \tstop_progress(&progress);\n>>\n>> The condition to show progress or not is conditional on an option that\n>> is external to the progress API.\n>>\n>> The fix for this patch is simple: make the trace2_region_leave() be\n>> conditional on \"p_progress && *p_progress\".\n\nOops.  That means that we need to apply the fix before -rc1 to\n'master' X-<.\n\nSomething like this?\n\n-- >8 --\nSubject: progress: call trace2_region_leave() only after calling _enter()\nFrom: Derrick Stolee <dstolee@microsoft.com>\n\nA user of progress API calls start_progress() conditionally and\ndepends on the display_progress() and stop_progress() functions to\nbecome no-op when start_progress() hasn't been called.\n\nAs we added a call to trace2_region_enter() to start_progress(), the\ncalls to other trace2 API calls from the progress API functions must\nmake sure that these trace2 calls are skipped when start_progress()\nhasn't been called on the progress struct.  Specifically, do not\ncall trace2_region_leave() from stop_progress() when we haven't\ncalled start_progress(), which would have called the matching\ntrace2_region_enter().\n\nSigned-off-by: Derrick Stolee <dstolee@microsoft.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n progress.c | 8 ++------\n 1 file changed, 2 insertions(+), 6 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 6d2dcff0b6..3eda914518 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -329,13 +329,9 @@ void stop_progress(struct progress **p_progress)\n \t\t\ttrace2_data_intmax(\"progress\", the_repository,\n \t\t\t\t\t   \"total_bytes\",\n \t\t\t\t\t   (*p_progress)->throughput->curr_total);\n-\t}\n \n-\ttrace2_region_leave(\"progress\",\n-\t\t\t    p_progress && *p_progress\n-\t\t\t\t? (*p_progress)->title\n-\t\t\t\t: NULL,\n-\t\t\t    the_repository);\n+\t\ttrace2_region_leave(\"progress\", (*p_progress)->title, the_repository);\n+\t}\n \n \tstop_progress_msg(p_progress, _(\"done\"));\n }\n\n\n\n\n"},{"id":"397894","messageId":"3e7b0eaf-3deb-31e4-6927-45429c6deed4@gmail.com","threadId":"53455","inReplyTo":"xmqqtv0hmb7r.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-15T16:49:47Z","receivedAt":"2020-05-15T16:49:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/15/2020 12:09 PM, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n>> Derrick Stolee <stolee@gmail.com> writes:\n>> ...\n>>> This trace2_region_leave() needs to be conditional on the progress\n>>> being non-null. The pattern used by consumers of the progress API is:\n>>>\n>>> \tif (my_progress_condition)\n>>> \t\tstart_progress(&progress);\n>>>\n>>> \tdo {\n>>> \t\tdisplay_progress(&progress, count);\n>>> \t} while (condition);\n>>>\n>>> \tstop_progress(&progress);\n>>>\n>>> The condition to show progress or not is conditional on an option that\n>>> is external to the progress API.\n>>>\n>>> The fix for this patch is simple: make the trace2_region_leave() be\n>>> conditional on \"p_progress && *p_progress\".\n> \n> Oops.  That means that we need to apply the fix before -rc1 to\n> 'master' X-<.\n\nSorry I didn't catch this in time for rc0!\n\n> Something like this?\n\nThe patch below is exactly what I would have recommended. It looks like\nI wrote it, too! ;)\n\n> -- >8 --\n> Subject: progress: call trace2_region_leave() only after calling _enter()\n> From: Derrick Stolee <dstolee@microsoft.com>\n> \n> A user of progress API calls start_progress() conditionally and\n> depends on the display_progress() and stop_progress() functions to\n> become no-op when start_progress() hasn't been called.\n> \n> As we added a call to trace2_region_enter() to start_progress(), the\n> calls to other trace2 API calls from the progress API functions must\n> make sure that these trace2 calls are skipped when start_progress()\n> hasn't been called on the progress struct.  Specifically, do not\n> call trace2_region_leave() from stop_progress() when we haven't\n> called start_progress(), which would have called the matching\n> trace2_region_enter().\n> \n> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  progress.c | 8 ++------\n>  1 file changed, 2 insertions(+), 6 deletions(-)\n> \n> diff --git a/progress.c b/progress.c\n> index 6d2dcff0b6..3eda914518 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -329,13 +329,9 @@ void stop_progress(struct progress **p_progress)\n>  \t\t\ttrace2_data_intmax(\"progress\", the_repository,\n>  \t\t\t\t\t   \"total_bytes\",\n>  \t\t\t\t\t   (*p_progress)->throughput->curr_total);\n> -\t}\n>  \n> -\ttrace2_region_leave(\"progress\",\n> -\t\t\t    p_progress && *p_progress\n> -\t\t\t\t? (*p_progress)->title\n> -\t\t\t\t: NULL,\n> -\t\t\t    the_repository);\n> +\t\ttrace2_region_leave(\"progress\", (*p_progress)->title, the_repository);\n> +\t}\n>  \n>  \tstop_progress_msg(p_progress, _(\"done\"));\n>  }\n\nThanks,\n-Stolee\n\n"},{"id":"397897","messageId":"xmqqblmpm8vg.fsf@gitster.c.googlers.com","threadId":"53455","inReplyTo":"3e7b0eaf-3deb-31e4-6927-45429c6deed4@gmail.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-15T17:00:03Z","receivedAt":"2020-05-15T17:00:16Z","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>> Something like this?\n>\n> The patch below is exactly what I would have recommended. It looks like\n> I wrote it, too! ;)\n\nThanks, will queue.\n"},{"id":"397929","messageId":"90f800d5-c647-6750-52d1-da973f036bd0@jeffhostetler.com","threadId":"53455","inReplyTo":"20200512214420.36329-1-emilyshaffer@google.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2020-05-15T19:37:20Z","receivedAt":"2020-05-15T19:37:24Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/12/20 5:44 PM, Emily Shaffer wrote:\n> Rather than teaching only one operation, like 'git fetch', how to write\n> down throughput to traces, we can learn about a wide range of user\n> operations that may seem slow by adding tooling to the progress library\n> itself. Operations which display progress are likely to be slow-running\n> and the kind of thing we want to monitor for performance anyways. By\n> showing object counts and data transfer size, we should be able to\n> make some derived measurements to ensure operations are scaling the way\n> we expect.\n> \n> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n\nWe need to be careful here.  The region_enter and _leave calls need\nto always be properly nested.  Having an implicit region within the\nprogress code means the code path between all of the different start_ \nand stop_progress calls need to not interleave with explicit regions.\n\nHow about putting something like this in stop_progress:\n\n     struct json_writer jw = JSON_WRITER_INIT;\n     jw_object_begin(&jw, 0);\n     jw_object_intmax(&jw, \"total\", p->total);\n     if (p->throughput)\n         jw_object_intmax(&jw, \"throughput\", p->throughput->curr_total);\n     /*\n      * and so on...\n      * and maybe include the elapsed time in since the start_progress.\n      */\n     jw_end(&jw);\n     trace2_data_json(\"progress\", NULL, p->title, &jw);\n     jw_release(&jw);\n\nThat will give you a single record summary of the progress meter\nand you won't have to worry about any interleaving.\n\nYou could also add a bit to `struct progress` to let you opt-in\nor opt-out of the message on a case-by-case basis.\n\nJeff\n\n\n\n> ---\n> One note: by putting trace collection into the progress library, we end\n> up with data events which have titles like \"Receiving objects\" - not\n> very machine-parseable. An alternative might be to ask for a\n> machine-readable title in the progress struct, but I didn't think it was\n> worth the code churn. However, I don't have experience with processing\n> the trace data after it's been collected, so if this is a bigger problem\n> than I think, please say so and I'll figure something out.\n> \n> CI run here, although it failed on the same error Junio noted today[1]:\n> https://github.com/nasamuffin/git/runs/668457062\n> \n>   - Emily\n> \n> [1]: https://lore.kernel.org/git/xmqqtv0kc2q1.fsf@gitster.c.googlers.com\n> \n>   progress.c                  | 17 +++++++++++++++++\n>   t/t0500-progress-display.sh | 26 ++++++++++++++++++++++++++\n>   2 files changed, 43 insertions(+)\n> \n> diff --git a/progress.c b/progress.c\n> index 75633e9c5e..6d2dcff0b6 100644\n> --- a/progress.c\n> +++ b/progress.c\n> @@ -265,6 +265,7 @@ static struct progress *start_progress_delay(const char *title, uint64_t total,\n>   \tprogress->title_len = utf8_strwidth(title);\n>   \tprogress->split = 0;\n>   \tset_progress_signal();\n> +\ttrace2_region_enter(\"progress\", title, the_repository);\n>   \treturn progress;\n>   }\n>   \n> @@ -320,6 +321,22 @@ void stop_progress(struct progress **p_progress)\n>   {\n>   \tfinish_if_sparse(*p_progress);\n>   \n> +\tif (p_progress && *p_progress) {\n> +\t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n> +\t\t\t\t   (*p_progress)->total);\n> +\n> +\t\tif ((*p_progress)->throughput)\n> +\t\t\ttrace2_data_intmax(\"progress\", the_repository,\n> +\t\t\t\t\t   \"total_bytes\",\n> +\t\t\t\t\t   (*p_progress)->throughput->curr_total);\n> +\t}\n> +\n> +\ttrace2_region_leave(\"progress\",\n> +\t\t\t    p_progress && *p_progress\n> +\t\t\t\t? (*p_progress)->title\n> +\t\t\t\t: NULL,\n> +\t\t\t    the_repository);\n> +\n>   \tstop_progress_msg(p_progress, _(\"done\"));\n>   }\n>   \n> diff --git a/t/t0500-progress-display.sh b/t/t0500-progress-display.sh\n> index d2d088d9a0..1ed1df351c 100755\n> --- a/t/t0500-progress-display.sh\n> +++ b/t/t0500-progress-display.sh\n> @@ -283,4 +283,30 @@ test_expect_success 'cover up after throughput shortens a lot' '\n>   \ttest_i18ncmp expect out\n>   '\n>   \n> +test_expect_success 'progress generates traces' '\n> +\tcat >in <<-\\EOF &&\n> +\tthroughput 102400 1000\n> +\tupdate\n> +\tprogress 10\n> +\tthroughput 204800 2000\n> +\tupdate\n> +\tprogress 20\n> +\tthroughput 307200 3000\n> +\tupdate\n> +\tprogress 30\n> +\tthroughput 409600 4000\n> +\tupdate\n> +\tprogress 40\n> +\tEOF\n> +\n> +\tGIT_TRACE2_EVENT=\"$(pwd)/trace.event\" test-tool progress --total=40 \\\n> +\t\t\"Working hard\" <in 2>stderr &&\n> +\n> +\t# t0212/parse_events.perl intentionally omits regions and data.\n> +\tgrep -e \"region_enter\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> +\tgrep -e \"region_leave\" -e \"\\\"category\\\":\\\"progress\\\"\" trace.event &&\n> +\tgrep \"\\\"key\\\":\\\"total_objects\\\",\\\"value\\\":\\\"40\\\"\" trace.event &&\n> +\tgrep \"\\\"key\\\":\\\"total_bytes\\\",\\\"value\\\":\\\"409600\\\"\" trace.event\n> +'\n> +\n>   test_done\n> \n"},{"id":"397930","messageId":"369ab235-e23a-712a-d2bf-82735194ceff@jeffhostetler.com","threadId":"53455","inReplyTo":"90f800d5-c647-6750-52d1-da973f036bd0@jeffhostetler.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2020-05-15T19:44:58Z","receivedAt":"2020-05-15T19:45:01Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 5/15/20 3:37 PM, Jeff Hostetler wrote:\n> \n> \n> On 5/12/20 5:44 PM, Emily Shaffer wrote:\n>> Rather than teaching only one operation, like 'git fetch', how to write\n>> down throughput to traces, we can learn about a wide range of user\n>> operations that may seem slow by adding tooling to the progress library\n>> itself. Operations which display progress are likely to be slow-running\n>> and the kind of thing we want to monitor for performance anyways. By\n>> showing object counts and data transfer size, we should be able to\n>> make some derived measurements to ensure operations are scaling the way\n>> we expect.\n>>\n>> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>\n> \n> We need to be careful here.  The region_enter and _leave calls need\n> to always be properly nested.  Having an implicit region within the\n> progress code means the code path between all of the different start_ \n> and stop_progress calls need to not interleave with explicit regions.\n> \n> How about putting something like this in stop_progress:\n> \n>      struct json_writer jw = JSON_WRITER_INIT;\n>      jw_object_begin(&jw, 0);\n>      jw_object_intmax(&jw, \"total\", p->total);\n>      if (p->throughput)\n>          jw_object_intmax(&jw, \"throughput\", p->throughput->curr_total);\n>      /*\n>       * and so on...\n>       * and maybe include the elapsed time in since the start_progress.\n>       */\n>      jw_end(&jw);\n>      trace2_data_json(\"progress\", NULL, p->title, &jw);\n>      jw_release(&jw);\n> \n> That will give you a single record summary of the progress meter\n> and you won't have to worry about any interleaving.\n> \n> You could also add a bit to `struct progress` to let you opt-in\n> or opt-out of the message on a case-by-case basis.\n> \n> Jeff\n\nHit send too quickly...\n\nOne of the advantages of a \"data_json\" event is that it will keep\nthe multiple values together in one event.  This makes it easier to\ncorrelate them during post processing.\n\nFor example, you could grep for \"\\\"data_json\\\".*\\\"progress\\\".*<title>\"\nand easily parse and see the total and time and etc in one record.\n\n\n\n"},{"id":"428009","messageId":"871r8w3sxu.fsf@evledraar.gmail.com","threadId":"53455","inReplyTo":"20200512214420.36329-1-emilyshaffer@google.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T01:24:47Z","receivedAt":"2021-06-21T01:30:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, May 12 2020, Emily Shaffer wrote:\n\n[Replying to a change long-since merged into git.git's \"master\"]\n\n> Rather than teaching only one operation, like 'git fetch', how to write\n> down throughput to traces, we can learn about a wide range of user\n> operations that may seem slow by adding tooling to the progress library\n> itself. Operations which display progress are likely to be slow-running\n> and the kind of thing we want to monitor for performance anyways. By\n> showing object counts and data transfer size, we should be able to\n> make some derived measurements to ensure operations are scaling the way\n> we expect.\n\nDid you end up using this data for anything?\n\n> [...]\n> @@ -320,6 +321,22 @@ void stop_progress(struct progress **p_progress)\n>  {\n>  \tfinish_if_sparse(*p_progress);\n>  \n> +\tif (p_progress && *p_progress) {\n> +\t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n> +\t\t\t\t   (*p_progress)->total);\n\nWe start progress bars for various things in git, yet the trace2 data\ncalls every such progress bar with a total \"total_objects\", even though\nwe may not be counting anything to do with objects.\n\nWouldn't simply s/total_objects/total/ make more sense here, do you rely\non the name of the current key?\n"},{"id":"428035","messageId":"CABPp-BFDfCqpDGZ1D=U6V5--KkcuCJON5-CcX-PVLcvH4pc5cg@mail.gmail.com","threadId":"53455","inReplyTo":"871r8w3sxu.fsf@evledraar.gmail.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T13:55:59Z","receivedAt":"2021-06-21T13:56:13Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Jun 20, 2021 at 6:32 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Tue, May 12 2020, Emily Shaffer wrote:\n>\n> [Replying to a change long-since merged into git.git's \"master\"]\n>\n> > Rather than teaching only one operation, like 'git fetch', how to write\n> > down throughput to traces, we can learn about a wide range of user\n> > operations that may seem slow by adding tooling to the progress library\n> > itself. Operations which display progress are likely to be slow-running\n> > and the kind of thing we want to monitor for performance anyways. By\n> > showing object counts and data transfer size, we should be able to\n> > make some derived measurements to ensure operations are scaling the way\n> > we expect.\n>\n> Did you end up using this data for anything?\n\nI know you were asking Emily, but independently, I found it useful\nwhile doing merge-ort and diffcore-rename optimizations.  I thought it\nwas a clever idea for quickly adding more measurement regions easily,\nand wished I had thought of it myself earlier.\n"},{"id":"428037","messageId":"87pmwf2rq5.fsf@evledraar.gmail.com","threadId":"53455","inReplyTo":"CABPp-BFDfCqpDGZ1D=U6V5--KkcuCJON5-CcX-PVLcvH4pc5cg@mail.gmail.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T14:51:53Z","receivedAt":"2021-06-21T14:54:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jun 21 2021, Elijah Newren wrote:\n\n> On Sun, Jun 20, 2021 at 6:32 PM Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n>>\n>> On Tue, May 12 2020, Emily Shaffer wrote:\n>>\n>> [Replying to a change long-since merged into git.git's \"master\"]\n>>\n>> > Rather than teaching only one operation, like 'git fetch', how to write\n>> > down throughput to traces, we can learn about a wide range of user\n>> > operations that may seem slow by adding tooling to the progress library\n>> > itself. Operations which display progress are likely to be slow-running\n>> > and the kind of thing we want to monitor for performance anyways. By\n>> > showing object counts and data transfer size, we should be able to\n>> > make some derived measurements to ensure operations are scaling the way\n>> > we expect.\n>>\n>> Did you end up using this data for anything?\n>\n> I know you were asking Emily, but independently, I found it useful\n> while doing merge-ort and diffcore-rename optimizations.  I thought it\n> was a clever idea for quickly adding more measurement regions easily,\n> and wished I had thought of it myself earlier.\n\nIndeed, the data's useful. I'm just wondering about the key name.\n\nI saw after I sent this that this was brought up when the patch was\ndiscussed, and the name was just left in there:\n\nhttps://lore.kernel.org/git/8f159f13-ed61-61ea-8e9a-c1ffbc5fddb3@gmail.com/\n\n"},{"id":"428100","messageId":"CABPp-BEe9r5vRYouhRRXmbWMJc7X+FqHC7CbzLpq-ALPRcc3fQ@mail.gmail.com","threadId":"53455","inReplyTo":"87pmwf2rq5.fsf@evledraar.gmail.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-21T20:28:56Z","receivedAt":"2021-06-21T20:29:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jun 21, 2021 at 7:54 AM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n> On Mon, Jun 21 2021, Elijah Newren wrote:\n>\n> > On Sun, Jun 20, 2021 at 6:32 PM Ævar Arnfjörð Bjarmason\n> > <avarab@gmail.com> wrote:\n> >>\n> >> On Tue, May 12 2020, Emily Shaffer wrote:\n> >>\n> >> [Replying to a change long-since merged into git.git's \"master\"]\n> >>\n> >> > Rather than teaching only one operation, like 'git fetch', how to write\n> >> > down throughput to traces, we can learn about a wide range of user\n> >> > operations that may seem slow by adding tooling to the progress library\n> >> > itself. Operations which display progress are likely to be slow-running\n> >> > and the kind of thing we want to monitor for performance anyways. By\n> >> > showing object counts and data transfer size, we should be able to\n> >> > make some derived measurements to ensure operations are scaling the way\n> >> > we expect.\n> >>\n> >> Did you end up using this data for anything?\n> >\n> > I know you were asking Emily, but independently, I found it useful\n> > while doing merge-ort and diffcore-rename optimizations.  I thought it\n> > was a clever idea for quickly adding more measurement regions easily,\n> > and wished I had thought of it myself earlier.\n>\n> Indeed, the data's useful. I'm just wondering about the key name.\n>\n> I saw after I sent this that this was brought up when the patch was\n> discussed, and the name was just left in there:\n>\n> https://lore.kernel.org/git/8f159f13-ed61-61ea-8e9a-c1ffbc5fddb3@gmail.com/\n\nYeah, I didn't comment on the key name because the name wasn't\nimportant for my purposes: I didn't have scripts or anything looking\nat the logs, just me reading them.\n\n\"total_objects\" worked just fine (and was slightly more precise\nwording) for my case, but since this code is used in lots of areas,\nI'd be totally fine to see it switched to \"total\".\n"},{"id":"428250","messageId":"YNKiv2Ar77xAxixz@nand.local","threadId":"53455","inReplyTo":"871r8w3sxu.fsf@evledraar.gmail.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-23T02:55:59Z","receivedAt":"2021-06-23T02:56:02Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Jun 21, 2021 at 03:24:47AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > [...]\n> > @@ -320,6 +321,22 @@ void stop_progress(struct progress **p_progress)\n> >  {\n> >  \tfinish_if_sparse(*p_progress);\n> >\n> > +\tif (p_progress && *p_progress) {\n> > +\t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n> > +\t\t\t\t   (*p_progress)->total);\n>\n> We start progress bars for various things in git, yet the trace2 data\n> calls every such progress bar with a total \"total_objects\", even though\n> we may not be counting anything to do with objects.\n>\n> Wouldn't simply s/total_objects/total/ make more sense here, do you rely\n> on the name of the current key?\n\nYeah, I think that the latter of just \"total\" makes more sense here. I\nwas going to comment on the fact that \"(*p_progress)->total\" could be\nwritten simply as \"*p_progress->total\", but I'm (a) not sure that I\nactually prefer the latter to the former, and (b) I find that kind of\nstyle comment generally useless.\n\nBut it may make sense to sidestep the whole thing and have a \"struct\nprogress *progress = *p_progress\" (that is assigned after we check\np_progress to make sure it's non-NULL) like in stop_progress_msg, which\nwould clean up a lot of this.\n\n--- 8< ---\n\nSubject: [PATCH] progress.c: avoid repeatedly dereferencing p_progress\n\nstop_progress() takes a double-pointer to a \"progress\" struct, and\ndereferences it twice in each use except one (checking whether\n*p_progress is NULL or not).\n\nMirror the implementation of stop_progress_msg() below by having a local\nsingle-pointer to a progress struct (which is the dereference of\np_progress) to avoid repeatedly dereferencing it.\n\nSigned-off-by: Taylor Blau <me@ttaylorr.com>\n---\n progress.c | 16 ++++++++++------\n 1 file changed, 10 insertions(+), 6 deletions(-)\n\ndiff --git a/progress.c b/progress.c\nindex 680c6a8bf9..390c76b22a 100644\n--- a/progress.c\n+++ b/progress.c\n@@ -319,21 +319,25 @@ static void finish_if_sparse(struct progress *progress)\n\n void stop_progress(struct progress **p_progress)\n {\n+\tstruct progress *progress;\n+\n \tif (!p_progress)\n \t\tBUG(\"don't provide NULL to stop_progress\");\n\n-\tfinish_if_sparse(*p_progress);\n+\tprogress = *p_progress;\n\n-\tif (*p_progress) {\n+\tfinish_if_sparse(progress);\n+\n+\tif (progress) {\n \t\ttrace2_data_intmax(\"progress\", the_repository, \"total_objects\",\n-\t\t\t\t   (*p_progress)->total);\n+\t\t\t\t   progress->total);\n\n-\t\tif ((*p_progress)->throughput)\n+\t\tif (progress->throughput)\n \t\t\ttrace2_data_intmax(\"progress\", the_repository,\n \t\t\t\t\t   \"total_bytes\",\n-\t\t\t\t\t   (*p_progress)->throughput->curr_total);\n+\t\t\t\t\t   progress->throughput->curr_total);\n\n-\t\ttrace2_region_leave(\"progress\", (*p_progress)->title, the_repository);\n+\t\ttrace2_region_leave(\"progress\", progress->title, the_repository);\n \t}\n\n \tstop_progress_msg(p_progress, _(\"done\"));\n--\n2.31.1.163.ga65ce7f831\n\n"},{"id":"428251","messageId":"CAPx1Gve_KqzQHanDDTSJ_2QywzNybq-J3wkG8xBC_DUhwVB5Fg@mail.gmail.com","threadId":"53455","inReplyTo":"YNKiv2Ar77xAxixz@nand.local","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2021-06-23T03:29:35Z","receivedAt":"2021-06-23T03:30:01Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Tue, Jun 22, 2021 at 7:56 PM Taylor Blau <me@ttaylorr.com> wrote:\n>... I was going to comment on the fact that \"(*p_progress)->total\" could\n> be written simply as \"*p_progress->total\", but I'm (a) not sure that I\n> actually prefer the latter to the former, and (b) I find that kind of\n> style comment generally useless.\n\nAlso, it can't. :-) The binding order is wrong; *p_progress->total binds as\n*(p_progress->total), and `p_progress` has to be followed first, so this\njust doesn't work.\n\n(Go does precedence and operator syntax a bit better than does C, but\neven in Go one must still parenthesize certain pointer-following operations.)\n\n> But it may make sense to sidestep the whole thing and have a \"struct\n> progress *progress = *p_progress\" (that is assigned after we check\n> p_progress to make sure it's non-NULL) like in stop_progress_msg, which\n> would clean up a lot of this.\n\nThis is the way to go.\n\nChris\n"},{"id":"428252","messageId":"YNKtvj4z2dwSUvOS@nand.local","threadId":"53455","inReplyTo":"CAPx1Gve_KqzQHanDDTSJ_2QywzNybq-J3wkG8xBC_DUhwVB5Fg@mail.gmail.com","subject":"Re: [PATCH] trace2: log progress time and throughput","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-06-23T03:42:54Z","receivedAt":"2021-06-23T03:43:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 22, 2021 at 08:29:35PM -0700, Chris Torek wrote:\n> On Tue, Jun 22, 2021 at 7:56 PM Taylor Blau <me@ttaylorr.com> wrote:\n> >... I was going to comment on the fact that \"(*p_progress)->total\" could\n> > be written simply as \"*p_progress->total\", but I'm (a) not sure that I\n> > actually prefer the latter to the former, and (b) I find that kind of\n> > style comment generally useless.\n>\n> Also, it can't. :-) The binding order is wrong; *p_progress->total binds as\n> *(p_progress->total), and `p_progress` has to be followed first, so this\n> just doesn't work.\n\nAck, serves me right for starting a discussion based on operator\nprecedence. Yes, you're right, I was mistaken and forgot that -> binds\nwith highest precedence in C (above *, which is why this doesn't work).\n\nIn any case, my confusion is probably a good reason to avoid this\nentirely by manipulating a variable which stores *p_progress.\n\nThanks,\nTaylor\n"}]}