locale: Optimize tst-localedef-path-norm

  The locale generation are issues in parallel to try speed locale
generation.  The maximum number of jobs are limited to the online
CPU (in hope to not overcommit on environments with lower cores
than tests).

On a Ryzen 9, the test execution improves from ~6.7s to ~1.4s.
---
 locale/Makefile                  |   2 +
 locale/tst-localedef-path-norm.c | 237 ++++++++++++++++---------------
 2 files changed, 128 insertions(+), 111 deletions(-)
  

Hi Adhemerval,

On Tue, Jul 19, 2022 at 11:01:13AM -0300, Adhemerval Zanella wrote:
> The locale generation are issues in parallel to try speed locale
> generation.  The maximum number of jobs are limited to the online
> CPU (in hope to not overcommit on environments with lower cores
> than tests).
> 
> On a Ryzen 9, the test execution improves from ~6.7s to ~1.4s.

Very nice, on the glibc-fedora-arm64 builder it went from:

real	0m45.440s
user	0m43.361s
sys	0m2.020s

to:

real	0m10.865s
user	0m45.975s
sys	0m1.777s

Tested-by: Mark Wielaard <mark@klomp.org>

I am not sure I qualify as reviewer given how little experience I have
with glibc tests, but here goes:

> diff --git a/locale/Makefile b/locale/Makefile
> index c315683b3f..eb55750496 100644
> --- a/locale/Makefile
> +++ b/locale/Makefile
> @@ -116,3 +116,5 @@ include $(patsubst %,$(..)libof-iterator.mk,$(cpp-srcs-left))
>  $(objpfx)tst-locale-locpath.out : tst-locale-locpath.sh $(objpfx)locale
>  	$(SHELL) $< '$(common-objpfx)' '$(test-wrapper-env)' '$(run-program-env)' > $@; \
>  	$(evaluate-test)
> +
> +$(objpfx)tst-localedef-path-norm: $(shared-thread-library)

OK, we are using pthreads now.

> diff --git a/locale/tst-localedef-path-norm.c b/locale/tst-localedef-path-norm.c
> index 68995a415d..97cc1cc396 100644
> --- a/locale/tst-localedef-path-norm.c
> +++ b/locale/tst-localedef-path-norm.c
> @@ -29,6 +29,7 @@
>     present, regardless of verbosity.  POSIX requires that any warnings cause the
>     exit status to be non-zero.  */
>  
> +#include <array_length.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
>  #include <unistd.h>
> @@ -37,9 +38,10 @@
>  #include <support/check.h>
>  #include <support/support.h>
>  #include <support/xunistd.h>
> +#include <support/xthread.h>
>  
>  /* Full path to localedef.  */
> -char *prog;
> +static const char *prog;

Not a required change, but obvious better.
prog is assigned once at the start of do_test.

>  /* Execute localedef in a subprocess.  */
>  static void
> @@ -63,12 +65,13 @@ struct test_closure
>  
>  /* Run localedef with DATA.ARGV arguments (NULL terminated), and expect path to
>     the compiled locale is "DATA.COMPLOCALEDIR/DATA.EXP".  */
> -static void
> -run_test (struct test_closure data)
> +static void *
> +run_test (void *closure)
>  {
> -  const char * const *args = data.argv;
> -  const char *exp = data.exp;
> -  const char *complocaledir = data.complocaledir;
> +  struct test_closure *data = closure;
> +  const char * const *args = data->argv;
> +  const char *exp = data->exp;
> +  const char *complocaledir = data->complocaledir;
>    struct stat64 fs;

OK, signature change so run_test can be used with pthread_create.
Variables now come through void * -> struct test_closure * instead of
struct passed by value.

>    /* Expected output path.  */
> @@ -82,8 +85,14 @@ run_test (struct test_closure data)
>  
>    /* Verify path is present and is a directory.  */
>    xstat (path, &fs);
> -  TEST_VERIFY_EXIT (S_ISDIR (fs.st_mode));
> -  printf ("info: Directory '%s' exists.\n", path);
> +  if (!S_ISDIR (fs.st_mode))
> +    {
> +      support_record_failure ();
> +      printf ("error: Directory '%s' does not exist.\n", path);
> +      return (void *) -1ULL;
> +    }
> +
> +  return NULL;
>  }

pthread function should return a void *.
Will later be checked to call TEST_VERIFY_EXIT in main thread.
Is this to avoid exit () race conditions?

support_record_failure is for reporting issues through support_test_main?

>  static int
> @@ -99,139 +108,145 @@ do_test (void)
>  #define ABSDIR "/output"
>    xmkdirp (ABSDIR, 0777);
>  
> -  /* It takes ~10 seconds to serially execute 9 localedef test.  We
> -     could run the compilations in parallel if we want to reduce test
> -     time.  We don't want to split this out into distinct tests because
> -     it would require multiple chroots.  Batching the same localedef
> -     tests saves disk space during testing.  */
> +  /* It takes ~10 seconds to serially execute 9 localedef test.  We run the
> +     compilations in parallel to reduce test time.  We don't want to split
> +     this out into distinct tests because it would require multiple chroots.
> +     Batching the same localedef tests saves disk space during testing.  */
>  
> +  struct test_closure tests[] =
> +  {
>    /* Test 1: Expected normalization.
>       Run localedef and expect output in $(complocaledir)/en_US1.utf8,
>       with normalization changing UTF-8 to utf8.  */
> -  run_test ((struct test_closure)
> -	    {
> -	      .argv = { prog,
> -			"--no-archive",
> -			"-i", "en_US",
> -			"-f", "UTF-8",
> -			"en_US1.UTF-8", NULL },
> -	      .exp = "en_US1.utf8",
> -	      .complocaledir = support_complocaledir_prefix
> -	    });
> -
> +    {
> +      .argv = { (const char *const) prog,
> +		"--no-archive",
> +		"-i", "en_US",
> +		"-f", "UTF-8",
> +		"en_US1.UTF-8", NULL },
> +      .exp = "en_US1.utf8",
> +      .complocaledir = support_complocaledir_prefix
> +    },

OK array of struct test_closure instead of calling run_runtest
directly on each struct. Idem for Test 2 .. Test 9

> +  /* Do not run more threads than the maximum of online CPUs.  */
> +  size_t ntests = array_length (tests);
> +  long int cpus = sysconf (_SC_NPROCESSORS_ONLN);
> +  cpus = cpus == -1 ? 1 : cpus;
> +  printf ("info: cpus=%ld ntests=%zu\n", cpus, ntests);
> +
> +  pthread_t thr[ntests];
> +
> +  for (int i = 0; i < ntests; i += cpus)
> +    {
> +      int max = i + cpus;
> +      if (max > ntests)
> +	max = ntests;
> +
> +      for (int j = i; j < max; j++)
> +	thr[j] = xpthread_create (NULL, run_test, &tests[j]);
> +
> +      for (int j = i; j < max; j++)
> +	TEST_VERIFY_EXIT (xpthread_join (thr[j]) == NULL);
> +    }

Nice pthread loop. We really only need thr[max] inside the loop, but
thr[ntests] makes things simpler to understand (also it is only 9
tests anyway, so who cares).

TEST_VERIFY_EXIT will call exit if the run_test returns (void *)-1.

Cheers,

Mark
  

On 19/07/22 16:43, Mark Wielaard wrote:
> Hi Adhemerval,
> 
> On Tue, Jul 19, 2022 at 11:01:13AM -0300, Adhemerval Zanella wrote:
>> The locale generation are issues in parallel to try speed locale
>> generation.  The maximum number of jobs are limited to the online
>> CPU (in hope to not overcommit on environments with lower cores
>> than tests).
>>
>> On a Ryzen 9, the test execution improves from ~6.7s to ~1.4s.
> 
> Very nice, on the glibc-fedora-arm64 builder it went from:
> 
> real	0m45.440s
> user	0m43.361s
> sys	0m2.020s
> 
> to:
> 
> real	0m10.865s
> user	0m45.975s
> sys	0m1.777s
> 
> Tested-by: Mark Wielaard <mark@klomp.org>

Thanks.

Is it ok for upstream Carlos?

> 
> I am not sure I qualify as reviewer given how little experience I have
> with glibc tests, but here goes:
> 
>> diff --git a/locale/Makefile b/locale/Makefile
>> index c315683b3f..eb55750496 100644
>> --- a/locale/Makefile
>> +++ b/locale/Makefile
>> @@ -116,3 +116,5 @@ include $(patsubst %,$(..)libof-iterator.mk,$(cpp-srcs-left))
>>  $(objpfx)tst-locale-locpath.out : tst-locale-locpath.sh $(objpfx)locale
>>  	$(SHELL) $< '$(common-objpfx)' '$(test-wrapper-env)' '$(run-program-env)' > $@; \
>>  	$(evaluate-test)
>> +
>> +$(objpfx)tst-localedef-path-norm: $(shared-thread-library)
> 
> OK, we are using pthreads now.
> 
>> diff --git a/locale/tst-localedef-path-norm.c b/locale/tst-localedef-path-norm.c
>> index 68995a415d..97cc1cc396 100644
>> --- a/locale/tst-localedef-path-norm.c
>> +++ b/locale/tst-localedef-path-norm.c
>> @@ -29,6 +29,7 @@
>>     present, regardless of verbosity.  POSIX requires that any warnings cause the
>>     exit status to be non-zero.  */
>>  
>> +#include <array_length.h>
>>  #include <sys/types.h>
>>  #include <sys/stat.h>
>>  #include <unistd.h>
>> @@ -37,9 +38,10 @@
>>  #include <support/check.h>
>>  #include <support/support.h>
>>  #include <support/xunistd.h>
>> +#include <support/xthread.h>
>>  
>>  /* Full path to localedef.  */
>> -char *prog;
>> +static const char *prog;
> 
> Not a required change, but obvious better.
> prog is assigned once at the start of do_test.
> 
>>  /* Execute localedef in a subprocess.  */
>>  static void
>> @@ -63,12 +65,13 @@ struct test_closure
>>  
>>  /* Run localedef with DATA.ARGV arguments (NULL terminated), and expect path to
>>     the compiled locale is "DATA.COMPLOCALEDIR/DATA.EXP".  */
>> -static void
>> -run_test (struct test_closure data)
>> +static void *
>> +run_test (void *closure)
>>  {
>> -  const char * const *args = data.argv;
>> -  const char *exp = data.exp;
>> -  const char *complocaledir = data.complocaledir;
>> +  struct test_closure *data = closure;
>> +  const char * const *args = data->argv;
>> +  const char *exp = data->exp;
>> +  const char *complocaledir = data->complocaledir;
>>    struct stat64 fs;
> 
> OK, signature change so run_test can be used with pthread_create.
> Variables now come through void * -> struct test_closure * instead of
> struct passed by value.
> 
>>    /* Expected output path.  */
>> @@ -82,8 +85,14 @@ run_test (struct test_closure data)
>>  
>>    /* Verify path is present and is a directory.  */
>>    xstat (path, &fs);
>> -  TEST_VERIFY_EXIT (S_ISDIR (fs.st_mode));
>> -  printf ("info: Directory '%s' exists.\n", path);
>> +  if (!S_ISDIR (fs.st_mode))
>> +    {
>> +      support_record_failure ();
>> +      printf ("error: Directory '%s' does not exist.\n", path);
>> +      return (void *) -1ULL;
>> +    }
>> +
>> +  return NULL;
>>  }
> 
> pthread function should return a void *.
> Will later be checked to call TEST_VERIFY_EXIT in main thread.
> Is this to avoid exit () race conditions?

If fact it is to avoid early exit the process without checking the return 
code of the other threads (TEST_VERIFY_EXIT call exit).

> 
> support_record_failure is for reporting issues through support_test_main?
> 
>>  static int
>> @@ -99,139 +108,145 @@ do_test (void)
>>  #define ABSDIR "/output"
>>    xmkdirp (ABSDIR, 0777);
>>  
>> -  /* It takes ~10 seconds to serially execute 9 localedef test.  We
>> -     could run the compilations in parallel if we want to reduce test
>> -     time.  We don't want to split this out into distinct tests because
>> -     it would require multiple chroots.  Batching the same localedef
>> -     tests saves disk space during testing.  */
>> +  /* It takes ~10 seconds to serially execute 9 localedef test.  We run the
>> +     compilations in parallel to reduce test time.  We don't want to split
>> +     this out into distinct tests because it would require multiple chroots.
>> +     Batching the same localedef tests saves disk space during testing.  */
>>  
>> +  struct test_closure tests[] =
>> +  {
>>    /* Test 1: Expected normalization.
>>       Run localedef and expect output in $(complocaledir)/en_US1.utf8,
>>       with normalization changing UTF-8 to utf8.  */
>> -  run_test ((struct test_closure)
>> -	    {
>> -	      .argv = { prog,
>> -			"--no-archive",
>> -			"-i", "en_US",
>> -			"-f", "UTF-8",
>> -			"en_US1.UTF-8", NULL },
>> -	      .exp = "en_US1.utf8",
>> -	      .complocaledir = support_complocaledir_prefix
>> -	    });
>> -
>> +    {
>> +      .argv = { (const char *const) prog,
>> +		"--no-archive",
>> +		"-i", "en_US",
>> +		"-f", "UTF-8",
>> +		"en_US1.UTF-8", NULL },
>> +      .exp = "en_US1.utf8",
>> +      .complocaledir = support_complocaledir_prefix
>> +    },
> 
> OK array of struct test_closure instead of calling run_runtest
> directly on each struct. Idem for Test 2 .. Test 9
> 
>> +  /* Do not run more threads than the maximum of online CPUs.  */
>> +  size_t ntests = array_length (tests);
>> +  long int cpus = sysconf (_SC_NPROCESSORS_ONLN);
>> +  cpus = cpus == -1 ? 1 : cpus;
>> +  printf ("info: cpus=%ld ntests=%zu\n", cpus, ntests);
>> +
>> +  pthread_t thr[ntests];
>> +
>> +  for (int i = 0; i < ntests; i += cpus)
>> +    {
>> +      int max = i + cpus;
>> +      if (max > ntests)
>> +	max = ntests;
>> +
>> +      for (int j = i; j < max; j++)
>> +	thr[j] = xpthread_create (NULL, run_test, &tests[j]);
>> +
>> +      for (int j = i; j < max; j++)
>> +	TEST_VERIFY_EXIT (xpthread_join (thr[j]) == NULL);
>> +    }
> 
> Nice pthread loop. We really only need thr[max] inside the loop, but
> thr[ntests] makes things simpler to understand (also it is only 9
> tests anyway, so who cares).
> 
> TEST_VERIFY_EXIT will call exit if the run_test returns (void *)-1.

Indeed, I will change to TEST_VERIFY so we don't early exit here.

> 
> Cheers,
> 
> Mark
  

On 7/20/22 14:58, Adhemerval Zanella Netto wrote:
> 
> 
> On 19/07/22 16:43, Mark Wielaard wrote:
>> Hi Adhemerval,
>>
>> On Tue, Jul 19, 2022 at 11:01:13AM -0300, Adhemerval Zanella wrote:
>>> The locale generation are issues in parallel to try speed locale
>>> generation.  The maximum number of jobs are limited to the online
>>> CPU (in hope to not overcommit on environments with lower cores
>>> than tests).
>>>
>>> On a Ryzen 9, the test execution improves from ~6.7s to ~1.4s.
>>
>> Very nice, on the glibc-fedora-arm64 builder it went from:
>>
>> real	0m45.440s
>> user	0m43.361s
>> sys	0m2.020s
>>
>> to:
>>
>> real	0m10.865s
>> user	0m45.975s
>> sys	0m1.777s
>>
>> Tested-by: Mark Wielaard <mark@klomp.org>
> 
> Thanks.
> 
> Is it ok for upstream Carlos?

OK for 2.36.

I trust Mark's review and testing.

We can always adjust the parallelism in the future.