{"thread":{"id":"58854","subject":"[PATCH] trace2 tests: guard pthread test with \"PTHREAD\"","startedAt":"2022-11-24T21:48:28Z","lastAt":"2022-11-30T18:41:19Z","messageCount":6,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff Hostetler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"467956","messageId":"patch-1.1-f7f21c94a6c-20221124T214813Z-avarab@gmail.com","threadId":"58854","inReplyTo":"010201d9002ee2f9940nexbridge.com","subject":"[PATCH] trace2 tests: guard pthread test with \"PTHREAD\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-24T21:48:21Z","receivedAt":"2022-11-24T21:48:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Since 81071626ba1 (trace2: add global counter mechanism, 2022-10-24)\nthese tests have been failing when git is compiled with NO_PTHREADS=Y,\nwhich is always the case e.g. if 'uname -s' is \"NONSTOP_KERNEL\".\n\nReported-by: Randall S. Becker \nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0211-trace2-perf.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\nindex 0b3436e8cac..b4e91351181 100755\n--- a/t/t0211-trace2-perf.sh\n+++ b/t/t0211-trace2-perf.sh\n@@ -203,7 +203,7 @@ test_expect_success 'stopwatch timer test/test1' '\n \thave_timer_event \"main\" \"timer\" \"test\" \"test1\" 5 actual\n '\n \n-test_expect_success 'stopwatch timer test/test2' '\n+test_expect_success PTHREAD 'stopwatch timer test/test2' '\n \ttest_when_finished \"rm trace.perf actual\" &&\n \ttest_config_global trace2.perfBrief 1 &&\n \ttest_config_global trace2.perfTarget \"$(pwd)/trace.perf\" &&\n@@ -249,7 +249,7 @@ test_expect_success 'global counter test/test1' '\n \thave_counter_event \"main\" \"counter\" \"test\" \"test1\" 15 actual\n '\n \n-test_expect_success 'global counter test/test2' '\n+test_expect_success PTHREAD 'global counter test/test2' '\n \ttest_when_finished \"rm trace.perf actual\" &&\n \ttest_config_global trace2.perfBrief 1 &&\n \ttest_config_global trace2.perfTarget \"$(pwd)/trace.perf\" &&\n-- \n2.39.0.rc0.955.ge9b241be664\n\n"},{"id":"467972","messageId":"xmqqilj3edtr.fsf@gitster.g","threadId":"58854","inReplyTo":"patch-1.1-f7f21c94a6c-20221124T214813Z-avarab@gmail.com","subject":"Re: [PATCH] trace2 tests: guard pthread test with \"PTHREAD\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-11-25T06:41:04Z","receivedAt":"2022-11-25T06:41:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Since 81071626ba1 (trace2: add global counter mechanism, 2022-10-24)\n> these tests have been failing when git is compiled with NO_PTHREADS=Y,\n> which is always the case e.g. if 'uname -s' is \"NONSTOP_KERNEL\".\n>\n> Reported-by: Randall S. Becker \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t0211-trace2-perf.sh | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n\nMakes sense, to sweep the immediate breakage under prerequisite.\n\nBut why would a global counter need threading? I know that the\ncounters can optionally report per-thread stats, but that is\noptional and it would still be useful if we can count events in a\nsingle-threaded program, no?\n\n>\n> diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh\n> index 0b3436e8cac..b4e91351181 100755\n> --- a/t/t0211-trace2-perf.sh\n> +++ b/t/t0211-trace2-perf.sh\n> @@ -203,7 +203,7 @@ test_expect_success 'stopwatch timer test/test1' '\n> \thave_timer_event \"main\" \"timer\" \"test\" \"test1\" 5 actual\n> '\n> \n> -test_expect_success 'stopwatch timer test/test2' '\n> +test_expect_success PTHREAD 'stopwatch timer test/test2' '\n> \ttest_when_finished \"rm trace.perf actual\" &&\n> \ttest_config_global trace2.perfBrief 1 &&\n> \ttest_config_global trace2.perfTarget \"$(pwd)/trace.perf\" &&\n> @@ -249,7 +249,7 @@ test_expect_success 'global counter test/test1' '\n> \thave_counter_event \"main\" \"counter\" \"test\" \"test1\" 15 actual\n> '\n> \n> -test_expect_success 'global counter test/test2' '\n> +test_expect_success PTHREAD 'global counter test/test2' '\n> \ttest_when_finished \"rm trace.perf actual\" &&\n> \ttest_config_global trace2.perfBrief 1 &&\n> \ttest_config_global trace2.perfTarget \"$(pwd)/trace.perf\" &&\n"},{"id":"467980","messageId":"221125.867czjo3ud.gmgdl@evledraar.gmail.com","threadId":"58854","inReplyTo":"xmqqilj3edtr.fsf@gitster.g","subject":"Re: [PATCH] trace2 tests: guard pthread test with \"PTHREAD\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-25T08:05:20Z","receivedAt":"2022-11-25T08:08:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 25 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Since 81071626ba1 (trace2: add global counter mechanism, 2022-10-24)\n>> these tests have been failing when git is compiled with NO_PTHREADS=Y,\n>> which is always the case e.g. if 'uname -s' is \"NONSTOP_KERNEL\".\n>>\n>> Reported-by: Randall S. Becker \n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> t/t0211-trace2-perf.sh | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> Makes sense, to sweep the immediate breakage under prerequisite.\n>\n> But why would a global counter need threading? I know that the\n> counters can optionally report per-thread stats, but that is\n> optional and it would still be useful if we can count events in a\n> single-threaded program, no?\n\nThat was my thought as well, but these tests are specifically testing\nhow it interacts with threading. The counter mechanism works in general\nwithout threading.\n\nThe test descriptions don't help, and should really say that they're to\ndo with threading in particular, but I wanted to keep this as small as\npossible for rc[12] and the final, so I didn't fix that while-at-it.\n"},{"id":"468114","messageId":"6c725ed6-4d74-c260-98fb-dc965b4647ee@jeffhostetler.com","threadId":"58854","inReplyTo":"221125.867czjo3ud.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] trace2 tests: guard pthread test with \"PTHREAD\"","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-11-28T16:37:55Z","receivedAt":"2022-11-28T16:38:00Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 11/25/22 3:05 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Nov 25 2022, Junio C Hamano wrote:\n> \n>> Ævar Arnfjörð Bjarmason writes:\n>>\n>>> Since 81071626ba1 (trace2: add global counter mechanism, 2022-10-24)\n>>> these tests have been failing when git is compiled with NO_PTHREADS=Y,\n>>> which is always the case e.g. if 'uname -s' is \"NONSTOP_KERNEL\".\n>>>\n>>> Reported-by: Randall S. Becker \n>>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>>> ---\n>>> t/t0211-trace2-perf.sh | 4 ++--\n>>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> Makes sense, to sweep the immediate breakage under prerequisite.\n\nThis change LGTM. The test-tool will report an error trying to\ncreate the requested threads, so there will not be any per-thread\noutput for the grep to match.\n\n\n>>\n>> But why would a global counter need threading? I know that the\n>> counters can optionally report per-thread stats, but that is\n>> optional and it would still be useful if we can count events in a\n>> single-threaded program, no?\n\nTimers & counters are accumulated in per-thread local storage.\nThis allows them to work in a lock-free manner.\n\nTimer/Counter usage by a single-threaded program just accumulates\ndata in the thread local storage of the \"main\" thread so that the\ncode is the same in all cases.\n\nGlobal timers and counters are accumulated from per-thread ones during\nthread-exit. (See calls to `tr2_update_final_timers()` and\n`tr2_update_final_counters()` in `trace2_thread_exit_fl()`.)\nThis is the only step that requires a lock.\n\nThere are also calls to those two routines in `tr2main_atexit_handler()`\nto accumulate any timer/counter usage in the main thread into the\nglobals.\n\nThe net-net of all of this is that the global (aka final) values\nof the timers and counters are always safely computed (and in the\nsame way) regardless of whether threads were used at any point\nduring the life of the process. That is, if \"main\" was the only\nthread, the global/final values are just the values from the main\nthread.\n\n\n> That was my thought as well, but these tests are specifically testing\n> how it interacts with threading. The counter mechanism works in general\n> without threading.\n> \n> The test descriptions don't help, and should really say that they're to\n> do with threading in particular, but I wanted to keep this as small as\n> possible for rc[12] and the final, so I didn't fix that while-at-it.\n\nThere is large comment block above `have_timer_event()` and\n`have_counter_event()` in t0211 that explained the purpose of the\ntest1 and test2 tests for each. Would it help if that text were moved\ndown before each of the individual tests rather than where it is now?\n\nThanks\nJeff\n"},{"id":"468116","messageId":"221128.86wn7fj72k.gmgdl@evledraar.gmail.com","threadId":"58854","inReplyTo":"6c725ed6-4d74-c260-98fb-dc965b4647ee@jeffhostetler.com","subject":"Re: [PATCH] trace2 tests: guard pthread test with \"PTHREAD\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-28T17:50:19Z","receivedAt":"2022-11-28T18:15:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 28 2022, Jeff Hostetler wrote:\n\n> On 11/25/22 3:05 AM, Ævar Arnfjörð Bjarmason wrote:\n> [...]\n>> That was my thought as well, but these tests are specifically testing\n>> how it interacts with threading. The counter mechanism works in general\n>> without threading.\n>> The test descriptions don't help, and should really say that they're\n>> to\n>> do with threading in particular, but I wanted to keep this as small as\n>> possible for rc[12] and the final, so I didn't fix that while-at-it.\n>\n> There is large comment block above `have_timer_event()` and\n> `have_counter_event()` in t0211 that explained the purpose of the\n> test1 and test2 tests for each. Would it help if that text were moved\n> down before each of the individual tests rather than where it is now?\n\nYou did ask :)\n\nI think that better than a comment is to have the test description\nitself reflect the nature & purpose of the test.\n\nNow the two are:\n\n\ttest_expect_success 'global counter test/test1' '\n\ttest_expect_success PTHREAD 'global counter test/test2' '\n\nSo at least the PTHREAD shows that it's something to do with threading,\nbut if it fails with that prereq passed you'll need to consult the\nsource to see what \"test2\" is supposed to do. Better would be to just\nskip the comment & work \"single threaded\" and \"multi-threaded\" etc. into\nthe test name itself.\n\nDitto symbol names \"ut_200counter\" and \"ut_201counter\", again, a comment\nsomewhere in t/helper/test-trace2.c notes that they're single- and\nmulti-threaded, respectively, but why not skip that and make the symbol\nnames self-descriptive?\n"},{"id":"468270","messageId":"c9ac37d3-99a9-a358-4d49-fea6a5d92efc@jeffhostetler.com","threadId":"58854","inReplyTo":"221128.86wn7fj72k.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] trace2 tests: guard pthread test with \"PTHREAD\"","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2022-11-30T18:41:03Z","receivedAt":"2022-11-30T18:41:19Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 11/28/22 12:50 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Nov 28 2022, Jeff Hostetler wrote:\n> \n>> On 11/25/22 3:05 AM, Ævar Arnfjörð Bjarmason wrote:\n>> [...]\n>>> That was my thought as well, but these tests are specifically testing\n>>> how it interacts with threading. The counter mechanism works in general\n>>> without threading.\n>>> The test descriptions don't help, and should really say that they're\n>>> to\n>>> do with threading in particular, but I wanted to keep this as small as\n>>> possible for rc[12] and the final, so I didn't fix that while-at-it.\n>>\n>> There is large comment block above `have_timer_event()` and\n>> `have_counter_event()` in t0211 that explained the purpose of the\n>> test1 and test2 tests for each. Would it help if that text were moved\n>> down before each of the individual tests rather than where it is now?\n> \n> You did ask :)\n\nYes, I did. :-) :-)\n\n\n> I think that better than a comment is to have the test description\n> itself reflect the nature & purpose of the test.\n> \n> Now the two are:\n> \n> \ttest_expect_success 'global counter test/test1' '\n> \ttest_expect_success PTHREAD 'global counter test/test2' '\n> \n> So at least the PTHREAD shows that it's something to do with threading,\n> but if it fails with that prereq passed you'll need to consult the\n> source to see what \"test2\" is supposed to do. Better would be to just\n> skip the comment & work \"single threaded\" and \"multi-threaded\" etc. into\n> the test name itself.\n> \n> Ditto symbol names \"ut_200counter\" and \"ut_201counter\", again, a comment\n> somewhere in t/helper/test-trace2.c notes that they're single- and\n> multi-threaded, respectively, but why not skip that and make the symbol\n> names self-descriptive?\n\nGood points. I'll keep this in mind for the future. I don't\nthink I want to send a fix for them now, but maybe the next\ntime I'm in the area...\n\n\nThanks again,\nJeff\n"}]}