git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] trace2 tests: guard pthread test with "PTHREAD"
       [not found] <010201d9002ee2f9940nexbridge.com>
@ 2022-11-24 21:48 ` Ævar Arnfjörð Bjarmason
  2022-11-25  6:41   ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-24 21:48 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Randall S . Becker,
	Ævar Arnfjörð Bjarmason

Since 81071626ba1 (trace2: add global counter mechanism, 2022-10-24)
these tests have been failing when git is compiled with NO_PTHREADS=Y,
which is always the case e.g. if 'uname -s' is "NONSTOP_KERNEL".

Reported-by: Randall S. Becker <randall.becker@nexbridge.ca>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0211-trace2-perf.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
index 0b3436e8cac..b4e91351181 100755
--- a/t/t0211-trace2-perf.sh
+++ b/t/t0211-trace2-perf.sh
@@ -203,7 +203,7 @@ test_expect_success 'stopwatch timer test/test1' '
 	have_timer_event "main" "timer" "test" "test1" 5 actual
 '
 
-test_expect_success 'stopwatch timer test/test2' '
+test_expect_success PTHREAD 'stopwatch timer test/test2' '
 	test_when_finished "rm trace.perf actual" &&
 	test_config_global trace2.perfBrief 1 &&
 	test_config_global trace2.perfTarget "$(pwd)/trace.perf" &&
@@ -249,7 +249,7 @@ test_expect_success 'global counter test/test1' '
 	have_counter_event "main" "counter" "test" "test1" 15 actual
 '
 
-test_expect_success 'global counter test/test2' '
+test_expect_success PTHREAD 'global counter test/test2' '
 	test_when_finished "rm trace.perf actual" &&
 	test_config_global trace2.perfBrief 1 &&
 	test_config_global trace2.perfTarget "$(pwd)/trace.perf" &&
-- 
2.39.0.rc0.955.ge9b241be664


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] trace2 tests: guard pthread test with "PTHREAD"
  2022-11-24 21:48 ` [PATCH] trace2 tests: guard pthread test with "PTHREAD" Ævar Arnfjörð Bjarmason
@ 2022-11-25  6:41   ` Junio C Hamano
  2022-11-25  8:05     ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2022-11-25  6:41 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Jeff Hostetler
  Cc: git, Randall S . Becker

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Since 81071626ba1 (trace2: add global counter mechanism, 2022-10-24)
> these tests have been failing when git is compiled with NO_PTHREADS=Y,
> which is always the case e.g. if 'uname -s' is "NONSTOP_KERNEL".
>
> Reported-by: Randall S. Becker <randall.becker@nexbridge.ca>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t0211-trace2-perf.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Makes sense, to sweep the immediate breakage under prerequisite.

But why would a global counter need threading?  I know that the
counters can optionally report per-thread stats, but that is
optional and it would still be useful if we can count events in a
single-threaded program, no?

>
> diff --git a/t/t0211-trace2-perf.sh b/t/t0211-trace2-perf.sh
> index 0b3436e8cac..b4e91351181 100755
> --- a/t/t0211-trace2-perf.sh
> +++ b/t/t0211-trace2-perf.sh
> @@ -203,7 +203,7 @@ test_expect_success 'stopwatch timer test/test1' '
>  	have_timer_event "main" "timer" "test" "test1" 5 actual
>  '
>  
> -test_expect_success 'stopwatch timer test/test2' '
> +test_expect_success PTHREAD 'stopwatch timer test/test2' '
>  	test_when_finished "rm trace.perf actual" &&
>  	test_config_global trace2.perfBrief 1 &&
>  	test_config_global trace2.perfTarget "$(pwd)/trace.perf" &&
> @@ -249,7 +249,7 @@ test_expect_success 'global counter test/test1' '
>  	have_counter_event "main" "counter" "test" "test1" 15 actual
>  '
>  
> -test_expect_success 'global counter test/test2' '
> +test_expect_success PTHREAD 'global counter test/test2' '
>  	test_when_finished "rm trace.perf actual" &&
>  	test_config_global trace2.perfBrief 1 &&
>  	test_config_global trace2.perfTarget "$(pwd)/trace.perf" &&

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] trace2 tests: guard pthread test with "PTHREAD"
  2022-11-25  6:41   ` Junio C Hamano
@ 2022-11-25  8:05     ` Ævar Arnfjörð Bjarmason
  2022-11-28 16:37       ` Jeff Hostetler
  0 siblings, 1 reply; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-25  8:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff Hostetler, git, Randall S . Becker


On Fri, Nov 25 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Since 81071626ba1 (trace2: add global counter mechanism, 2022-10-24)
>> these tests have been failing when git is compiled with NO_PTHREADS=Y,
>> which is always the case e.g. if 'uname -s' is "NONSTOP_KERNEL".
>>
>> Reported-by: Randall S. Becker <randall.becker@nexbridge.ca>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  t/t0211-trace2-perf.sh | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> Makes sense, to sweep the immediate breakage under prerequisite.
>
> But why would a global counter need threading?  I know that the
> counters can optionally report per-thread stats, but that is
> optional and it would still be useful if we can count events in a
> single-threaded program, no?

That was my thought as well, but these tests are specifically testing
how it interacts with threading. The counter mechanism works in general
without threading.

The test descriptions don't help, and should really say that they're to
do with threading in particular, but I wanted to keep this as small as
possible for rc[12] and the final, so I didn't fix that while-at-it.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] trace2 tests: guard pthread test with "PTHREAD"
  2022-11-25  8:05     ` Ævar Arnfjörð Bjarmason
@ 2022-11-28 16:37       ` Jeff Hostetler
  2022-11-28 17:50         ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 5+ messages in thread
From: Jeff Hostetler @ 2022-11-28 16:37 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Junio C Hamano
  Cc: Jeff Hostetler, git, Randall S . Becker



On 11/25/22 3:05 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Nov 25 2022, Junio C Hamano wrote:
> 
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> Since 81071626ba1 (trace2: add global counter mechanism, 2022-10-24)
>>> these tests have been failing when git is compiled with NO_PTHREADS=Y,
>>> which is always the case e.g. if 'uname -s' is "NONSTOP_KERNEL".
>>>
>>> Reported-by: Randall S. Becker <randall.becker@nexbridge.ca>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> ---
>>>   t/t0211-trace2-perf.sh | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> Makes sense, to sweep the immediate breakage under prerequisite.

This change LGTM.  The test-tool will report an error trying to
create the requested threads, so there will not be any per-thread
output for the grep to match.


>>
>> But why would a global counter need threading?  I know that the
>> counters can optionally report per-thread stats, but that is
>> optional and it would still be useful if we can count events in a
>> single-threaded program, no?

Timers & counters are accumulated in per-thread local storage.
This allows them to work in a lock-free manner.

Timer/Counter usage by a single-threaded program just accumulates
data in the thread local storage of the "main" thread so that the
code is the same in all cases.

Global timers and counters are accumulated from per-thread ones during
thread-exit.  (See calls to `tr2_update_final_timers()` and
`tr2_update_final_counters()` in `trace2_thread_exit_fl()`.)
This is the only step that requires a lock.

There are also calls to those two routines in `tr2main_atexit_handler()`
to accumulate any timer/counter usage in the main thread into the
globals.

The net-net of all of this is that the global (aka final) values
of the timers and counters are always safely computed (and in the
same way) regardless of whether threads were used at any point
during the life of the process.  That is, if "main" was the only
thread, the global/final values are just the values from the main
thread.


> That was my thought as well, but these tests are specifically testing
> how it interacts with threading. The counter mechanism works in general
> without threading.
> 
> The test descriptions don't help, and should really say that they're to
> do with threading in particular, but I wanted to keep this as small as
> possible for rc[12] and the final, so I didn't fix that while-at-it.

There is large comment block above `have_timer_event()` and
`have_counter_event()` in t0211 that explained the purpose of the
test1 and test2 tests for each.  Would it help if that text were moved
down before each of the individual tests rather than where it is now?

Thanks
Jeff

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] trace2 tests: guard pthread test with "PTHREAD"
  2022-11-28 16:37       ` Jeff Hostetler
@ 2022-11-28 17:50         ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 5+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-11-28 17:50 UTC (permalink / raw)
  To: Jeff Hostetler; +Cc: Junio C Hamano, Jeff Hostetler, git, Randall S . Becker


On Mon, Nov 28 2022, Jeff Hostetler wrote:

> On 11/25/22 3:05 AM, Ævar Arnfjörð Bjarmason wrote:
> [...]
>> That was my thought as well, but these tests are specifically testing
>> how it interacts with threading. The counter mechanism works in general
>> without threading.
>> The test descriptions don't help, and should really say that they're
>> to
>> do with threading in particular, but I wanted to keep this as small as
>> possible for rc[12] and the final, so I didn't fix that while-at-it.
>
> There is large comment block above `have_timer_event()` and
> `have_counter_event()` in t0211 that explained the purpose of the
> test1 and test2 tests for each.  Would it help if that text were moved
> down before each of the individual tests rather than where it is now?

You did ask :)

I think that better than a comment is to have the test description
itself reflect the nature & purpose of the test.

Now the two are:

	test_expect_success 'global counter test/test1' '
	test_expect_success PTHREAD 'global counter test/test2' '

So at least the PTHREAD shows that it's something to do with threading,
but if it fails with that prereq passed you'll need to consult the
source to see what "test2" is supposed to do. Better would be to just
skip the comment & work "single threaded" and "multi-threaded" etc. into
the test name itself.

Ditto symbol names "ut_200counter" and "ut_201counter", again, a comment
somewhere in t/helper/test-trace2.c notes that they're single- and
multi-threaded, respectively, but why not skip that and make the symbol
names self-descriptive?

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2022-11-28 18:15 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <010201d9002ee2f9940nexbridge.com>
2022-11-24 21:48 ` [PATCH] trace2 tests: guard pthread test with "PTHREAD" Ævar Arnfjörð Bjarmason
2022-11-25  6:41   ` Junio C Hamano
2022-11-25  8:05     ` Ævar Arnfjörð Bjarmason
2022-11-28 16:37       ` Jeff Hostetler
2022-11-28 17:50         ` Ævar Arnfjörð Bjarmason

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).