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
@@ -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)
@@ -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;
/* 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;
/* 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;
}
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
+ },
/* Test 2: No normalization past '@'.
Run localedef and expect output in $(complocaledir)/en_US2.utf8@tEsT,
with normalization changing UTF-8@tEsT to utf8@tEsT (everything after
@ is untouched). */
- run_test ((struct test_closure)
- {
- .argv = { prog,
- "--no-archive",
- "-i", "en_US",
- "-f", "UTF-8",
- "en_US2.UTF-8@tEsT", NULL },
- .exp = "en_US2.utf8@tEsT",
- .complocaledir = support_complocaledir_prefix
- });
-
+ {
+ .argv = { prog,
+ "--no-archive",
+ "-i", "en_US",
+ "-f", "UTF-8",
+ "en_US2.UTF-8@tEsT", NULL },
+ .exp = "en_US2.utf8@tEsT",
+ .complocaledir = support_complocaledir_prefix
+ },
/* Test 3: No normalization past '@' despite period.
Run localedef and expect output in $(complocaledir)/en_US3@tEsT.UTF-8,
with normalization changing nothing (everything after @ is untouched)
despite there being a period near the end. */
- run_test ((struct test_closure)
- {
- .argv = { prog,
- "--no-archive",
- "-i", "en_US",
- "-f", "UTF-8",
- "en_US3@tEsT.UTF-8", NULL },
- .exp = "en_US3@tEsT.UTF-8",
- .complocaledir = support_complocaledir_prefix
- });
-
+ {
+ .argv = { prog,
+ "--no-archive",
+ "-i", "en_US",
+ "-f", "UTF-8",
+ "en_US3@tEsT.UTF-8", NULL },
+ .exp = "en_US3@tEsT.UTF-8",
+ .complocaledir = support_complocaledir_prefix
+ },
/* Test 4: Normalize numeric codeset by adding 'iso' prefix.
Run localedef and expect output in $(complocaledir)/en_US4.88591,
with normalization changing 88591 to iso88591. */
- run_test ((struct test_closure)
- {
- .argv = { prog,
- "--no-archive",
- "-i", "en_US",
- "-f", "UTF-8",
- "en_US4.88591", NULL },
- .exp = "en_US4.iso88591",
- .complocaledir = support_complocaledir_prefix
- });
-
+ {
+ .argv = { prog,
+ "--no-archive",
+ "-i", "en_US",
+ "-f", "UTF-8",
+ "en_US4.88591", NULL },
+ .exp = "en_US4.iso88591",
+ .complocaledir = support_complocaledir_prefix
+ },
/* Test 5: Don't add 'iso' prefix if first char is alpha.
Run localedef and expect output in $(complocaledir)/en_US5.a88591,
with normalization changing nothing. */
- run_test ((struct test_closure)
- {
- .argv = { prog,
- "--no-archive",
- "-i", "en_US",
- "-f", "UTF-8",
- "en_US5.a88591", NULL },
- .exp = "en_US5.a88591",
- .complocaledir = support_complocaledir_prefix
- });
-
+ {
+ .argv = { prog,
+ "--no-archive",
+ "-i", "en_US",
+ "-f", "UTF-8",
+ "en_US5.a88591", NULL },
+ .exp = "en_US5.a88591",
+ .complocaledir = support_complocaledir_prefix
+ },
/* Test 6: Don't add 'iso' prefix if last char is alpha.
Run localedef and expect output in $(complocaledir)/en_US6.88591a,
with normalization changing nothing. */
- run_test ((struct test_closure)
- {
- .argv = { prog,
- "--no-archive",
- "-i", "en_US",
- "-f", "UTF-8",
- "en_US6.88591a", NULL },
- .exp = "en_US6.88591a",
- .complocaledir = support_complocaledir_prefix
- });
-
+ {
+ .argv = { prog,
+ "--no-archive",
+ "-i", "en_US",
+ "-f", "UTF-8",
+ "en_US6.88591a", NULL },
+ .exp = "en_US6.88591a",
+ .complocaledir = support_complocaledir_prefix
+ },
/* Test 7: Don't normalize anything with an absolute path.
Run localedef and expect output in ABSDIR/en_US7.UTF-8,
with normalization changing nothing. */
- run_test ((struct test_closure)
- {
- .argv = { prog,
- "--no-archive",
- "-i", "en_US",
- "-f", "UTF-8",
- ABSDIR "/en_US7.UTF-8", NULL },
- .exp = "en_US7.UTF-8",
- .complocaledir = ABSDIR
- });
-
+ {
+ .argv = { prog,
+ "--no-archive",
+ "-i", "en_US",
+ "-f", "UTF-8",
+ ABSDIR "/en_US7.UTF-8", NULL },
+ .exp = "en_US7.UTF-8",
+ .complocaledir = ABSDIR
+ },
/* Test 8: Don't normalize anything with an absolute path.
Run localedef and expect output in ABSDIR/en_US8.UTF-8@tEsT,
with normalization changing nothing. */
- run_test ((struct test_closure)
- {
- .argv = { prog,
- "--no-archive",
- "-i", "en_US",
- "-f", "UTF-8",
- ABSDIR "/en_US8.UTF-8@tEsT", NULL },
- .exp = "en_US8.UTF-8@tEsT",
- .complocaledir = ABSDIR
- });
-
+ {
+ .argv = { prog,
+ "--no-archive",
+ "-i", "en_US",
+ "-f", "UTF-8",
+ ABSDIR "/en_US8.UTF-8@tEsT", NULL },
+ .exp = "en_US8.UTF-8@tEsT",
+ .complocaledir = ABSDIR
+ },
/* Test 9: Don't normalize anything with an absolute path.
Run localedef and expect output in ABSDIR/en_US9@tEsT.UTF-8,
with normalization changing nothing. */
- run_test ((struct test_closure)
- {
- .argv = { prog,
- "--no-archive",
- "-i", "en_US",
- "-f", "UTF-8",
- ABSDIR "/en_US9@tEsT.UTF-8", NULL },
- .exp = "en_US9@tEsT.UTF-8",
- .complocaledir = ABSDIR
- });
+ {
+ .argv = { prog,
+ "--no-archive",
+ "-i", "en_US",
+ "-f", "UTF-8",
+ ABSDIR "/en_US9@tEsT.UTF-8", NULL },
+ .exp = "en_US9@tEsT.UTF-8",
+ .complocaledir = ABSDIR
+ }
+ };
+
+ /* 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);
+ }
return 0;
}