[PATCH] Parallelize test read-dwarf

[Dodji Seketeli]

Hi,

Ondrej Oprala <ooprala@redhat.com> a écrit:

> Hi, based on our IRC chats, here's my take on a simple parallelization
> in test read-dwarf.

Thank you for working on this, it's really appreciated :-)

This comes in handy as I was looking at the time of that test on my old
X220 laptop, which is now > 1 minute :-)  So really, this is greatly
appreciated :-)

> Make distcheck passes and timing the test itself, the runtime dropped
> from ~37 to ~14 seconds on my machine.

Whoah!

So let's look at the patch then :-)

> From 81393153db55ded3192b75cf1a96969fbd4d0784 Mon Sep 17 00:00:00 2001
> From: Ondrej Oprala <ooprala@redhat.com>
> Date: Tue, 6 Oct 2015 14:35:14 +0200
> Subject: [PATCH] Parallelize test read-dwarf.
>
> 	* tests/Makefile.am: Link runtestreaddwarf with libpthread.
> 	* tests/test-read-dwarf.cc (main) Create worker threads corresponding
> 	to the number of CPUs online and move the main loop...
> 	(handleInOutSpec) ...here.
>
> Signed-off-by: Ondrej Oprala <ooprala@redhat.com>

What can I say :-)  It looks nice.  I don't have much to say but some
really minor nits.

[...]

> +++ b/tests/test-read-dwarf.cc
[...]

> +// All the pthread_ts we've created.
> +pthread_t *pthr;
> +// Number of currently online processors in the system.
> +size_t nprocs = sysconf(_SC_NPROCESSORS_ONLN);

I would move these two global variables into main(), as I don't think
they are used anywhere else but in that function.

[...]

> +void handleInOutSpec(void)
> +{

Here, please avoid camel case in function names, as done elsewhere in
the code base.  This normal GNU coding standard, sorry :-)

Also, the name of the function should start at the beginning of the line
like:

void
handle_in_out_spec(void)
{

The reason for this (also GNU coding standard practice) is so that we
can easily grep function definitions by doing
 
    grep -E ^handle_in_out_spec *.cc

One other thing I'd ask you is to maybe run Hellgrind (the Valgrind
thread errors detection tool) on the test to see if it says something.

Ah, and the last thing that would be great -- if you have time -- would
be to allow a --no-parallel option to this test.  That would be useful
to get error output in a serial way, you know.  Because otherwise, the
error output would be kind of garbled because of all those threads
spitting their errors out in a random order.  But if you don't have
time, I'll add that myself later.  No problem.  What you have done is
huge already, and I love it :-)

If Valgrind/Hellgrind is happy, then I guess the patch is OK to go into
master, with the changes above.

Thank you again for doing this.

Cheers,

-- 
		Dodji