[PATCH] Parallelize test read-dwarf

[Ondrej Oprala]

Thanks for the late-night review!

On 07.10.2015 00:39, Dodji Seketeli wrote:
> 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.
>
> [...]
Oops...missed that.
>> +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
Yeah, I somehow got confused by the struct's name, my bad :-D .
> 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.
I made several changes, so I'd like to ask you for a quick re-review 
before I push it.
> Thank you again for doing this.
>
> Cheers,
>
Thanks,
  Ondrej
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Parallelize-test-read-dwarf.patch
Type: text/x-patch
Size: 8327 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/libabigail/attachments/20150101/865ec8a5/attachment.bin>