Re: [PATCH v1] perf test: Add a runs-per-test flag

[Liang, Kan]

On 2024-11-11 11:10 a.m., Ian Rogers wrote:
> On Mon, Nov 11, 2024 at 7:52 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>
>>
>>
>> On 2024-11-09 11:02 a.m., Ian Rogers wrote:
>>> To detect flakes it is useful to run tests more than once. Add a
>>> runs-per-test flag that will run each test multiple times.
>>>
>>> Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
>>> ---
>>>  tools/perf/tests/builtin-test.c | 38 ++++++++++++++++++++-------------
>>>  1 file changed, 23 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
>>> index d2cabaa8ad92..574fbd5caff0 100644
>>> --- a/tools/perf/tests/builtin-test.c
>>> +++ b/tools/perf/tests/builtin-test.c
>>> @@ -42,6 +42,8 @@
>>>  static bool dont_fork;
>>>  /* Fork the tests in parallel and wait for their completion. */
>>>  static bool sequential;
>>> +/* Numer of times each test is run. */
>>> +static unsigned int runs_per_test = 1;
>>>  const char *dso_to_test;
>>>  const char *test_objdump_path = "objdump";
>>>
>>> @@ -490,10 +492,10 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>>>                               len = strlen(test_description(*t, subi));
>>>                               if (width < len)
>>>                                       width = len;
>>> -                             num_tests++;
>>> +                             num_tests += runs_per_test;
>>>                       }
>>>               } else {
>>> -                     num_tests++;
>>> +                     num_tests += runs_per_test;
>>>               }
>>>       }
>>
>> Seems we just need to calculate the num_tests once at the end for each
>> loop. Something as below may works. (not tested)
>>
>> @@ -482,20 +490,19 @@ static int __cmd_test(struct test_suite **suites,
>> int argc, const char *argv[],
>>
>>         for (struct test_suite **t = suites; *t; t++) {
>>                 int len = strlen(test_description(*t, -1));
>> +               int subi = 0, subn = 1;
>>
>>                 if (width < len)
>>                         width = len;
>>
>>                 if (has_subtests(*t)) {
>> -                       for (int subi = 0, subn = num_subtests(*t); subi
>> < subn; subi++) {
>> +                       for (subn = num_subtests(*t); subi < subn; subi++) {
>>                                 len = strlen(test_description(*t, subi));
>>                                 if (width < len)
>>                                         width = len;
>> -                               num_tests++;
>>                         }
>> -               } else {
>> -                       num_tests++;
>>                 }
>> +               num_tests += subn * runs_per_test;
>>         }
>>         child_tests = calloc(num_tests, sizeof(*child_tests));
>>         if (!child_tests)
> 
> It's basically the same thing, instead of doing increments and then
> multiplying by runs_per_test you just add on runs_per_test and avoid
> the multiply.

The "else" should be unnecessary either. But the above is just a nit.

> 
>>>       child_tests = calloc(num_tests, sizeof(*child_tests));
>>> @@ -556,21 +558,25 @@ static int __cmd_test(struct test_suite **suites, int argc, const char *argv[],
>>>                       }
>>>
>>>                       if (!has_subtests(*t)) {
>>> -                             err = start_test(*t, curr, -1, &child_tests[child_test_num++],
>>> -                                              width, pass);
>>> -                             if (err)
>>> -                                     goto err_out;
>>> +                             for (unsigned int run = 0; run < runs_per_test; run++) {
>>> +                                     err = start_test(*t, curr, -1, &child_tests[child_test_num++],
>>> +                                                     width, pass);
>>> +                                     if (err)
>>> +                                             goto err_out;
>>> +                             }
>>>                               continue;
>>>                       }
>>> -                     for (int subi = 0, subn = num_subtests(*t); subi < subn; subi++) {
>>> -                             if (!perf_test__matches(test_description(*t, subi),
>>> -                                                     curr, argc, argv))
>>> -                                     continue;
>>> -
>>> -                             err = start_test(*t, curr, subi, &child_tests[child_test_num++],
>>> -                                              width, pass);
>>> -                             if (err)
>>> -                                     goto err_out;
>>> +                     for (unsigned int run = 0; run < runs_per_test; run++) {
>>> +                             for (int subi = 0, subn = num_subtests(*t); subi < subn; subi++) {
>>> +                                     if (!perf_test__matches(test_description(*t, subi),
>>> +                                                                     curr, argc, argv))
>>> +                                             continue;
>>> +
>>> +                                     err = start_test(*t, curr, subi, &child_tests[child_test_num++],
>>> +                                                     width, pass);
>>> +                                     if (err)
>>> +                                             goto err_out;
>>> +                             }
>>
>> Can we add a wrapper for the start_test()? Something similar to below?
>> It avoids adding the loop for every places using the start_test.
>>
>> +static int start_test(struct test_suite *test, int i, int subi, struct
>> child_test **child,
>> +               int width, int pass)
>> +{
>> +       for (unsigned int run = 0; run < runs_per_test; run++) {
>> +               __start_test();
>> +       }
>> +}
> 
> I think the issue is the code has become overly indented. 

And duplication.

> Having a
> start_test function that starts some number of tests feels less than
> intention revealing. Perhaps (in the future I'd like to tackle other
> things for now, such as new TMAs :-) ) we can create all the child
> tests in one pass, then just have start_test and finish_test work with
> the child tests. 

It may be easier to understand if we have both start_mul/all_tests() and
start_single_test().

> (Off topic) Something else I'd like is to move the
> slower running tests to the end of the list of tests so you can see
> the earlier results while waiting.

I'm not sure how useful it is. But for me, I always wait for all the
tests complete, no matter how fast the results of the first several
cases shows.

Thanks,
Kan
> 
> Thanks,
> Ian
> 
> 
>>>                       }
>>>               }
>>>               if (!sequential) {
>>> @@ -714,6 +720,8 @@ int cmd_test(int argc, const char **argv)
>>>                   "Do not fork for testcase"),
>>>       OPT_BOOLEAN('S', "sequential", &sequential,
>>>                   "Run the tests one after another rather than in parallel"),
>>> +     OPT_UINTEGER('r', "runs-per-test", &runs_per_test,
>>> +                  "Run each test the given number of times, default 1"),
>>>       OPT_STRING('w', "workload", &workload, "work", "workload to run for testing, use '--list-workloads' to list the available ones."),
>>>       OPT_BOOLEAN(0, "list-workloads", &list_workloads, "List the available builtin workloads to use with -w/--workload"),
>>>       OPT_STRING(0, "dso", &dso_to_test, "dso", "dso to test"),
>>
>