[PATCH] locale: Optimize tst-localedef-path-norm

[Mark Wielaard]

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