Message ID | 522fba5e-1048-3377-45c1-7107b55dc6e1@web.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | mergesort: improve tests and performance | expand |
René Scharfe <l.s.r@web.de> writes: > +static void dist_rand(int *arr, int n, int m) > +{ > + int i; > + for (i = 0; i < n; i++) > + arr[i] = rand() % m; > +} > ... > +static void dist_shuffle(int *arr, int n, int m) > +{ > + int i, j, k; > + for (i = j = 0, k = 1; i < n; i++) > + arr[i] = (rand() % m) ? (j += 2) : (k += 2); > +} I briefly wondered if we want to seed the rand() in some way to make the tests reproducible, but we'd need to ship our own rand() if we wanted to go that route, which would probably be too much. > +static int test(const struct dist *dist, const struct mode *mode, int n, int m) > +{ > +... > + for (i = 0, curr = list; i < n && curr; i++, curr = curr->next) { > + if (arr[i] != curr->value) > + is_sorted = 0; > + if (curr->next && curr->value == curr->next->value && > + curr->rank >= curr->next->rank) > + is_stable = 0; > + } > + if (i < n) { > + verdict = "too short"; > + } else if (curr) { > + verdict = "too long"; > + } else if (!is_sorted) { > + verdict = "not sorted"; > + } else if (!is_stable) { > + verdict = "unstable"; > + } else { > + verdict = "OK"; > + result = 0; > + } > + > + printf("%-9s %-16s %8d %8d %8d %8d %8d %s\n", > + dist->name, mode->name, n, m, stats.get_next, stats.set_next, > + stats.compare, verdict); > + > + clear_numbers(list); > + free(arr); > + > + return result; > +} Nice. > int cmd__mergesort(int argc, const char **argv) > { > if (argc == 2 && !strcmp(argv[1], "sort")) > return sort_stdin(); > - usage("test-tool mergesort sort"); > + if (argc > 1 && !strcmp(argv[1], "test")) > + return run_tests(argc - 2, argv + 2); > + fprintf(stderr, "usage: test-tool mergesort sort\n"); > + fprintf(stderr, " or: test-tool mergesort test [<n>...]\n"); > + return 129; If you can live with OPT_CMDMODE to implement sort/test subcommands, you'd get to have parse_options() to do the usage for you, I think. I am not sure if it is worth it, as t/helper/ is not end-user facing.
On Fri, Oct 01 2021, Junio C Hamano wrote: > René Scharfe <l.s.r@web.de> writes: > >> +static void dist_rand(int *arr, int n, int m) >> +{ >> + int i; >> + for (i = 0; i < n; i++) >> + arr[i] = rand() % m; >> +} >> ... >> +static void dist_shuffle(int *arr, int n, int m) >> +{ >> + int i, j, k; >> + for (i = j = 0, k = 1; i < n; i++) >> + arr[i] = (rand() % m) ? (j += 2) : (k += 2); >> +} > > I briefly wondered if we want to seed the rand() in some way to make > the tests reproducible, but we'd need to ship our own rand() if we > wanted to go that route, which would probably be too much. Wouldn't calling srand() with some constant value suffice on most platforms? I'm aware of it being a NOOP and rand() always being randomly seeded on (IIRC) OpenBSD, but that should work on e.g. glibc. >> int cmd__mergesort(int argc, const char **argv) >> { >> if (argc == 2 && !strcmp(argv[1], "sort")) >> return sort_stdin(); >> - usage("test-tool mergesort sort"); >> + if (argc > 1 && !strcmp(argv[1], "test")) >> + return run_tests(argc - 2, argv + 2); >> + fprintf(stderr, "usage: test-tool mergesort sort\n"); >> + fprintf(stderr, " or: test-tool mergesort test [<n>...]\n"); >> + return 129; > > If you can live with OPT_CMDMODE to implement sort/test subcommands, > you'd get to have parse_options() to do the usage for you, I think. > I am not sure if it is worth it, as t/helper/ is not end-user > facing. Yeah I think the one thing that could improve here is this custom getopts handling, in particular the manual formatting of "usage" and "or" is a bit ugly, considering that you'll get it for free with the parse_options() "usage" array.
Am 02.10.21 um 10:35 schrieb Ævar Arnfjörð Bjarmason: > > On Fri, Oct 01 2021, Junio C Hamano wrote: > >> René Scharfe <l.s.r@web.de> writes: >> >>> +static void dist_rand(int *arr, int n, int m) >>> +{ >>> + int i; >>> + for (i = 0; i < n; i++) >>> + arr[i] = rand() % m; >>> +} >>> ... >>> +static void dist_shuffle(int *arr, int n, int m) >>> +{ >>> + int i, j, k; >>> + for (i = j = 0, k = 1; i < n; i++) >>> + arr[i] = (rand() % m) ? (j += 2) : (k += 2); >>> +} >> >> I briefly wondered if we want to seed the rand() in some way to make >> the tests reproducible, but we'd need to ship our own rand() if we >> wanted to go that route, which would probably be too much. > > Wouldn't calling srand() with some constant value suffice on most > platforms? I'm aware of it being a NOOP and rand() always being randomly > seeded on (IIRC) OpenBSD, but that should work on e.g. glibc. Right, so we'd need to ship our own random number generator. Repeatable tests are not essential (the original paper didn't mention seeding), but shouldn't be much trouble to implement and would simplify comparisons across versions, systems and among testers. The only downside I can think of is that it may perhaps also simplify over-fitting, i.e. I might find micro-tweaks that only work for our specific rand() sequence and then misinterpret them as general improvements.. René
Am 02.10.21 um 10:35 schrieb Ævar Arnfjörð Bjarmason: > > On Fri, Oct 01 2021, Junio C Hamano wrote: > >> René Scharfe <l.s.r@web.de> writes: >> >>> int cmd__mergesort(int argc, const char **argv) >>> { >>> if (argc == 2 && !strcmp(argv[1], "sort")) >>> return sort_stdin(); >>> - usage("test-tool mergesort sort"); >>> + if (argc > 1 && !strcmp(argv[1], "test")) >>> + return run_tests(argc - 2, argv + 2); >>> + fprintf(stderr, "usage: test-tool mergesort sort\n"); >>> + fprintf(stderr, " or: test-tool mergesort test [<n>...]\n"); >>> + return 129; >> >> If you can live with OPT_CMDMODE to implement sort/test subcommands, >> you'd get to have parse_options() to do the usage for you, I think. >> I am not sure if it is worth it, as t/helper/ is not end-user >> facing. > > Yeah I think the one thing that could improve here is this custom > getopts handling, in particular the manual formatting of "usage" and > "or" is a bit ugly, considering that you'll get it for free with the > parse_options() "usage" array. I don't see how using parseopt would help here. Maintaining the "usage" and "or" strings manually is trivial. The meaty part of the usage string (e.g. "test [<n>...]") would not be generated, neither would the repeated part ("test-tool mergesort"). OPT_CMDMODE would require double dashes for no good reason. PowerShell's param array allows specifying value types, positions and group parameters into sets. I think it's expressive enough to allow declaring all three subcommands and their parameters, and then can parse command lines and generate help text automatically. Until parseopt gains similar capabilities I'd like to avoid that dependency. René
René Scharfe <l.s.r@web.de> writes: > Repeatable tests are not essential (the original paper didn't mention > seeding), but shouldn't be much trouble to implement and would simplify > comparisons across versions, systems and among testers. > > The only downside I can think of is that it may perhaps also simplify > over-fitting, i.e. I might find micro-tweaks that only work for our > specific rand() sequence and then misinterpret them as general > improvements.. Yup, I think you summarized the pros-and-cons nicely.
Am 03.10.21 um 19:33 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Repeatable tests are not essential (the original paper didn't mention >> seeding), but shouldn't be much trouble to implement and would simplify >> comparisons across versions, systems and among testers. >> >> The only downside I can think of is that it may perhaps also simplify >> over-fitting, i.e. I might find micro-tweaks that only work for our >> specific rand() sequence and then misinterpret them as general >> improvements.. > > Yup, I think you summarized the pros-and-cons nicely. Seeing that the series is in next already, here's a bonus patch for that. --- >8 --- Subject: [PATCH 10/9] test-mergesort: use repeatable random numbers Use MINSTD to generate pseudo-random numbers consistently instead of using rand(3), whose output can vary from system to system, and reset its seed before filling in the test values. This gives repeatable results across versions and systems, which simplifies sharing and comparing of results between developers. Signed-off-by: René Scharfe <l.s.r@web.de> --- t/helper/test-mergesort.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/t/helper/test-mergesort.c b/t/helper/test-mergesort.c index 43ec74e2d3..8d128ae437 100644 --- a/t/helper/test-mergesort.c +++ b/t/helper/test-mergesort.c @@ -2,6 +2,12 @@ #include "cache.h" #include "mergesort.h" +static unsigned int minstd_rand(unsigned int *state) +{ + *state = (uint64_t)*state * 48271 % 2147483647; + return *state; +} + struct line { char *text; struct line *next; @@ -60,8 +66,9 @@ static void dist_sawtooth(int *arr, int n, int m) static void dist_rand(int *arr, int n, int m) { int i; + unsigned int seed = 1; for (i = 0; i < n; i++) - arr[i] = rand() % m; + arr[i] = minstd_rand(&seed) % m; } static void dist_stagger(int *arr, int n, int m) @@ -81,8 +88,9 @@ static void dist_plateau(int *arr, int n, int m) static void dist_shuffle(int *arr, int n, int m) { int i, j, k; + unsigned int seed = 1; for (i = j = 0, k = 1; i < n; i++) - arr[i] = (rand() % m) ? (j += 2) : (k += 2); + arr[i] = minstd_rand(&seed) % m ? (j += 2) : (k += 2); } #define DIST(name) { #name, dist_##name } -- 2.33.0
diff --git a/t/helper/test-mergesort.c b/t/helper/test-mergesort.c index 05be0d067a..8006be8bf8 100644 --- a/t/helper/test-mergesort.c +++ b/t/helper/test-mergesort.c @@ -50,9 +50,239 @@ static int sort_stdin(void) return 0; } +static void dist_sawtooth(int *arr, int n, int m) +{ + int i; + for (i = 0; i < n; i++) + arr[i] = i % m; +} + +static void dist_rand(int *arr, int n, int m) +{ + int i; + for (i = 0; i < n; i++) + arr[i] = rand() % m; +} + +static void dist_stagger(int *arr, int n, int m) +{ + int i; + for (i = 0; i < n; i++) + arr[i] = (i * m + i) % n; +} + +static void dist_plateau(int *arr, int n, int m) +{ + int i; + for (i = 0; i < n; i++) + arr[i] = (i < m) ? i : m; +} + +static void dist_shuffle(int *arr, int n, int m) +{ + int i, j, k; + for (i = j = 0, k = 1; i < n; i++) + arr[i] = (rand() % m) ? (j += 2) : (k += 2); +} + +#define DIST(name) { #name, dist_##name } + +static struct dist { + const char *name; + void (*fn)(int *arr, int n, int m); +} dist[] = { + DIST(sawtooth), + DIST(rand), + DIST(stagger), + DIST(plateau), + DIST(shuffle), +}; + +static void mode_copy(int *arr, int n) +{ + /* nothing */ +} + +static void mode_reverse(int *arr, int n) +{ + int i, j; + for (i = 0, j = n - 1; i < j; i++, j--) + SWAP(arr[i], arr[j]); +} + +static void mode_reverse_1st_half(int *arr, int n) +{ + mode_reverse(arr, n / 2); +} + +static void mode_reverse_2nd_half(int *arr, int n) +{ + int half = n / 2; + mode_reverse(arr + half, n - half); +} + +static int compare_ints(const void *av, const void *bv) +{ + const int *ap = av, *bp = bv; + int a = *ap, b = *bp; + return (a > b) - (a < b); +} + +static void mode_sort(int *arr, int n) +{ + QSORT(arr, n, compare_ints); +} + +static void mode_dither(int *arr, int n) +{ + int i; + for (i = 0; i < n; i++) + arr[i] += i % 5; +} + +#define MODE(name) { #name, mode_##name } + +static struct mode { + const char *name; + void (*fn)(int *arr, int n); +} mode[] = { + MODE(copy), + MODE(reverse), + MODE(reverse_1st_half), + MODE(reverse_2nd_half), + MODE(sort), + MODE(dither), +}; + +static struct stats { + int get_next, set_next, compare; +} stats; + +struct number { + int value, rank; + struct number *next; +}; + +static void *get_next_number(const void *a) +{ + stats.get_next++; + return ((const struct number *)a)->next; +} + +static void set_next_number(void *a, void *b) +{ + stats.set_next++; + ((struct number *)a)->next = b; +} + +static int compare_numbers(const void *av, const void *bv) +{ + const struct number *an = av, *bn = bv; + int a = an->value, b = bn->value; + stats.compare++; + return (a > b) - (a < b); +} + +static void clear_numbers(struct number *list) +{ + while (list) { + struct number *next = list->next; + free(list); + list = next; + } +} + +static int test(const struct dist *dist, const struct mode *mode, int n, int m) +{ + int *arr; + size_t i; + struct number *curr, *list, **tail; + int is_sorted = 1; + int is_stable = 1; + const char *verdict; + int result = -1; + + ALLOC_ARRAY(arr, n); + dist->fn(arr, n, m); + mode->fn(arr, n); + for (i = 0, tail = &list; i < n; i++) { + curr = xmalloc(sizeof(*curr)); + curr->value = arr[i]; + curr->rank = i; + *tail = curr; + tail = &curr->next; + } + *tail = NULL; + + stats.get_next = stats.set_next = stats.compare = 0; + list = llist_mergesort(list, get_next_number, set_next_number, + compare_numbers); + + QSORT(arr, n, compare_ints); + for (i = 0, curr = list; i < n && curr; i++, curr = curr->next) { + if (arr[i] != curr->value) + is_sorted = 0; + if (curr->next && curr->value == curr->next->value && + curr->rank >= curr->next->rank) + is_stable = 0; + } + if (i < n) { + verdict = "too short"; + } else if (curr) { + verdict = "too long"; + } else if (!is_sorted) { + verdict = "not sorted"; + } else if (!is_stable) { + verdict = "unstable"; + } else { + verdict = "OK"; + result = 0; + } + + printf("%-9s %-16s %8d %8d %8d %8d %8d %s\n", + dist->name, mode->name, n, m, stats.get_next, stats.set_next, + stats.compare, verdict); + + clear_numbers(list); + free(arr); + + return result; +} + +/* + * A version of the qsort certification program from "Engineering a Sort + * Function" by Bentley and McIlroy, Software—Practice and Experience, + * Volume 23, Issue 11, 1249–1265 (November 1993). + */ +static int run_tests(int argc, const char **argv) +{ + const char *argv_default[] = { "100", "1023", "1024", "1025" }; + if (!argc) + return run_tests(ARRAY_SIZE(argv_default), argv_default); + printf("%-9s %-16s %8s %8s %8s %8s %8s %s\n", + "distribut", "mode", "n", "m", "get_next", "set_next", + "compare", "verdict"); + while (argc--) { + int i, j, m, n = strtol(*argv++, NULL, 10); + for (i = 0; i < ARRAY_SIZE(dist); i++) { + for (j = 0; j < ARRAY_SIZE(mode); j++) { + for (m = 1; m < 2 * n; m *= 2) { + if (test(&dist[i], &mode[j], n, m)) + return 1; + } + } + } + } + return 0; +} + int cmd__mergesort(int argc, const char **argv) { if (argc == 2 && !strcmp(argv[1], "sort")) return sort_stdin(); - usage("test-tool mergesort sort"); + if (argc > 1 && !strcmp(argv[1], "test")) + return run_tests(argc - 2, argv + 2); + fprintf(stderr, "usage: test-tool mergesort sort\n"); + fprintf(stderr, " or: test-tool mergesort test [<n>...]\n"); + return 129; } diff --git a/t/t0071-sort.sh b/t/t0071-sort.sh new file mode 100755 index 0000000000..a8ab174879 --- /dev/null +++ b/t/t0071-sort.sh @@ -0,0 +1,11 @@ +#!/bin/sh + +test_description='verify sort functions' + +. ./test-lib.sh + +test_expect_success 'llist_mergesort()' ' + test-tool mergesort test +' + +test_done
Adapt the qsort certification program from "Engineering a Sort Function" by Bentley and McIlroy for testing our linked list sort function. It generates several lists with various distribution patterns and counts the number of operations llist_mergesort() needs to order them. It compares the result to the output of a trusted sort function (qsort(1)) and also checks if the sort is stable. Also add a test script that makes use of the new subcommand. Signed-off-by: René Scharfe <l.s.r@web.de> --- t/helper/test-mergesort.c | 232 +++++++++++++++++++++++++++++++++++++- t/t0071-sort.sh | 11 ++ 2 files changed, 242 insertions(+), 1 deletion(-) create mode 100755 t/t0071-sort.sh -- 2.33.0