[PATCH] Parallelize test read-dwarf
Thu Jan 1 00:00:00 GMT 2015
Thanks for the late-night review!
On 07.10.2015 00:39, Dodji Seketeli wrote:
> Ondrej Oprala <firstname.lastname@example.org> 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.
> So let's look at the patch then :-)
>> From 81393153db55ded3192b75cf1a96969fbd4d0784 Mon Sep 17 00:00:00 2001
>> From: Ondrej Oprala <email@example.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 <firstname.lastname@example.org>
> 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
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 8327 bytes
Desc: not available
More information about the Libabigail