Re: [PATCH 3/3] lib: vsprintf: added a put_dec() test and benchmark tool

[Denys Vlasenko <vda.linux@googlemail.com>]

On Friday 06 August 2010 00:38, Michal Nazarewicz wrote:
> This commit adds a test application for the put_dec() and
> family of functions that are used by the previous two commits.
> 
> Signed-off-by: Michal Nazarewicz <mina86@mina86.com>

> +put-dec-test: put-dec-test.c
> +	exec $(CC) $(CFLAGS) -o $@ $<

(1) Why exec?
(2) Add -Wall, you'd be surprised


> +static uint64_t rand_64(void)
> +{
> +	uint64_t v = 0, m = 1;
> +	for (;;) {
> +		uint64_t n;
> +		v = m * rand();
> +		n = m + m * RAND_MAX;
> +		if (n < m)
> +			break;
> +		m = n;
> +	}
> +	return v;
> +}

What this function do? Looks cryptic. In my testing, it picks 0
quite often.


> +static char buf1[24];

Can you size the array safely, without assuming that long long
is no wider than 64 bits?


> +#define FUNC(func, outer, inner, correction, format, value) do {	\
> +		struct timeval start;					\
> +		unsigned i, o;						\
> +		for (i = (inner); i--; ) {				\
> +			typeof(value) v = (value);			\
> +			ret |= test(#func, func(buf1, v), format, v);	\

I'd add memset(buf1, 77, sizeof(buf1)) before test


> +int main(int argc, char **argv) {
> +	unsigned long iterations = 1000, i;
> +	struct timeval correction;
> +	int ret = 0;
> +
> +	srand(time(NULL));
> +
> +	if (argc > 1)
> +		iterations = atoi(argv[1]);
> +
> +	gettimeofday(&correction, NULL);
> +	for (i = 25000 * iterations; i; --i)
> +		rand_64();
> +	stop(NULL, &correction, NULL);

Why is this "correction" thing needed? I looked at the entire machinery
and I see no reason to have it.


> +	puts(">> Benchmarks:\n\tput_dec_full()");
> +	fflush(NULL);
> +
> +	FUNC(orig_put_dec_full, iterations, 100000, NULL, "%05u", i);

You have variable named i, you pass it as macro parameter,
but macro has local variable named i too.
Is it an International Obfuscated C Code Contest entry?


> +	FUNC(mod1_put_dec_full, iterations, 100000, NULL, "%05u", i);
> +	FUNC(mod3_put_dec_full, iterations, 100000, NULL, "%05u", i);
> +	FUNC(mod5_put_dec_full, iterations * 10, 10000, NULL, "%04u", i);

> +	puts("\tput_dec_trunc()");
> +	fflush(NULL);
> +
> +	FUNC(orig_put_dec_trunc, iterations, 100000, NULL, "%u", i);
> +	FUNC(mod1_put_dec_trunc, iterations, 100000, NULL, "%u", i);
> +	FUNC(mod3_put_dec_trunc, iterations, 100000, NULL, "%u", i);
> +	FUNC(mod5_put_dec_trunc, iterations * 10, 10000, NULL, "%u", i);
> +	FUNC(mod3_put_dec_8bit, iterations * 500, 256, NULL, "%u", i);

> +	puts("\n\tput_dec()");
> +	fflush(NULL);
> +
> +	FUNC(orig_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());

"%llu" fmt is for unsigned long long, not uint64_t.


> +	FUNC(mod1_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());
> +	FUNC(mod2_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());
> +	FUNC(mod3_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());
> +	FUNC(mod4_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());
> +	FUNC(mod5_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());
> +	FUNC(mod6_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());
> +	FUNC(mod7_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());
> +	FUNC(mod8_put_dec, iterations / 4, 100000, &correction, "%llu", rand_64());

Here a lot of CPU time is taken by rand() calls. Also, you use different
values for different functions, which is wrong.


-- 
vda