diff mbox series

[3/9] test-mergesort: add test subcommand

Message ID 522fba5e-1048-3377-45c1-7107b55dc6e1@web.de (mailing list archive)
State New, archived
Headers show
Series mergesort: improve tests and performance | expand

Commit Message

René Scharfe Oct. 1, 2021, 9:12 a.m. UTC
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

Comments

Junio C Hamano Oct. 1, 2021, 8:26 p.m. UTC | #1
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.
Ævar Arnfjörð Bjarmason Oct. 2, 2021, 8:35 a.m. UTC | #2
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.
René Scharfe Oct. 3, 2021, 10:15 a.m. UTC | #3
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é
René Scharfe Oct. 3, 2021, 10:15 a.m. UTC | #4
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é
Junio C Hamano Oct. 3, 2021, 5:33 p.m. UTC | #5
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.
René Scharfe Oct. 7, 2021, 8 p.m. UTC | #6
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 mbox series

Patch

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