Message ID | 20220719140113.1604672-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | locale: Optimize tst-localedef-path-norm | expand |
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.
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) 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; /* 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; }