[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Parallelize test read-dwarf
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
>From 92e0824ec414dc89ed5ffd780ea33cd3286a335a 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, add a "--no-parallel" option and move
the main loop...
(handleInOutSpec) ...here.
Signed-off-by: Ondrej Oprala <ooprala@redhat.com>
---
tests/Makefile.am | 1 +
tests/test-read-dwarf.cc | 215 ++++++++++++++++++++++++++++++-----------------
2 files changed, 137 insertions(+), 79 deletions(-)
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 8ab7bb6..cf8f2af 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -55,6 +55,7 @@ runtestwritereadarchive_LDADD= libtestutils.la $(top_builddir)/src/libabigail.la
runtestreaddwarf_SOURCES=test-read-dwarf.cc
runtestreaddwarf_LDADD=libtestutils.la $(top_builddir)/src/libabigail.la
+runtestreaddwarf_LDFLAGS=-pthread
runtestlookupsyms_SOURCES=test-lookup-syms.cc
runtestlookupsyms_LDADD=libtestutils.la $(top_builddir)/src/libabigail.la
diff --git a/tests/test-read-dwarf.cc b/tests/test-read-dwarf.cc
index c4fed5f..e6f2040 100644
--- a/tests/test-read-dwarf.cc
+++ b/tests/test-read-dwarf.cc
@@ -28,6 +28,8 @@
#include <fstream>
#include <iostream>
#include <cstdlib>
+#include <unistd.h>
+#include <pthread.h>
#include "abg-ir.h"
#include "abg-dwarf-reader.h"
#include "abg-writer.h"
@@ -38,6 +40,7 @@ using std::string;
using std::ofstream;
using std::cerr;
using abigail::tests::get_build_dir;
+using abigail::dwarf_reader::read_corpus_from_elf;
/// This is an aggregate that specifies where a test shall get its
/// input from, and where it shall write its ouput to.
@@ -150,96 +153,150 @@ InOutSpec in_out_specs[] =
{NULL, NULL, NULL}
};
-int
-main()
-{
- using abigail::dwarf_reader::read_corpus_from_elf;
- unsigned result = 1;
+// The global pointer to the testsuite paths.
+InOutSpec *iospec = in_out_specs;
+// Lock to help atomically increment iospec.
+pthread_mutex_t spec_lock = PTHREAD_MUTEX_INITIALIZER;
+pthread_mutex_t write_lock = PTHREAD_MUTEX_INITIALIZER;
+// No lock needed here, since is_ok is only ever re-set to false.
+bool is_ok = true;
+bool no_parallel;
- bool is_ok = true;
+// These prefixes don't change during the program's lifetime, so
+// we only get them once.
+const string out_abi_base = get_build_dir() + "/tests/";
+const string in_elf_base = abigail::tests::get_src_dir() + "/tests/";
+const string in_abi_base = in_elf_base;
+
+void
+handle_in_out_spec(void)
+{
string in_elf_path, in_abi_path, out_abi_path;
abigail::ir::environment_sptr env;
- abigail::corpus_sptr corp;
+ InOutSpec *s;
- for (InOutSpec* s = in_out_specs; s->in_elf_path; ++s)
- {
- in_elf_path = abigail::tests::get_src_dir() + "/tests/" + s->in_elf_path;
- env.reset(new abigail::ir::environment);
- abigail::dwarf_reader::status status =
- abigail::dwarf_reader::STATUS_UNKNOWN;
- corp = read_corpus_from_elf(in_elf_path,
- /*debug_info_root_path=*/0,
- env.get(),
- /*load_all_types=*/false,
- status);
- if (!corp)
- {
- cerr << "failed to read " << in_elf_path << "\n";
- is_ok = false;
- continue;
- }
- corp->set_path(s->in_elf_path);
- // Do not take architecture names in comparison so that these
- // test input binaries can come from whatever arch the
- // programmer likes.
- corp->set_architecture_name("");
-
- out_abi_path = get_build_dir() + "/tests/" + s->out_abi_path;
- if (!abigail::tools_utils::ensure_parent_dir_created(out_abi_path))
- {
- cerr << "Could not create parent director for " << out_abi_path;
- is_ok = false;
- return result;
- }
+ while (true)
+ {
+ pthread_mutex_lock(&spec_lock);
+ if (iospec->in_elf_path)
+ s = iospec++;
+ else
+ s = NULL;
+ pthread_mutex_unlock(&spec_lock);
- ofstream of(out_abi_path.c_str(), std::ios_base::trunc);
- if (!of.is_open())
- {
- cerr << "failed to read " << out_abi_path << "\n";
- is_ok = false;
- continue;
- }
+ if (!s)
+ goto end;
+ in_elf_path = in_elf_base + s->in_elf_path;
+ env.reset(new abigail::ir::environment);
+ abigail::dwarf_reader::status status =
+ abigail::dwarf_reader::STATUS_UNKNOWN;
+ abigail::corpus_sptr corp =
+ read_corpus_from_elf(in_elf_path,
+ /*debug_info_root_path=*/0,
+ env.get(),
+ /*load_all_types=*/false,
+ status);
+ if (!corp)
+ {
+ cerr << "failed to read " << in_elf_path << "\n";
+ is_ok = false;
+ continue;
+ }
+ corp->set_path(s->in_elf_path);
+ // Do not take architecture names in comparison so that these
+ // test input binaries can come from whatever arch the
+ // programmer likes.
+ corp->set_architecture_name("");
- bool r =
- abigail::xml_writer::write_corpus_to_native_xml(corp,
- /*indent=*/0,
- of);
- is_ok = (is_ok && r);
- of.close();
-
- string abilint = get_build_dir() + "/tools/abilint";
- abilint += " --noout";
- string cmd = abilint + " " + out_abi_path;
- if (system(cmd.c_str()))
- {
- cerr << "output file doesn't pass abilint: " << out_abi_path << "\n";
- r = false;
- }
- is_ok = (is_ok && r);
+ out_abi_path = out_abi_base + s->out_abi_path;
+ if (!abigail::tools_utils::ensure_parent_dir_created(out_abi_path))
+ {
+ cerr << "Could not create parent directory for " << out_abi_path;
+ is_ok = false;
+ exit(1);
+ }
+ ofstream of(out_abi_path.c_str(), std::ios_base::trunc);
+ if (!of.is_open())
+ {
+ cerr << "failed to read " << out_abi_path << "\n";
+ is_ok = false;
+ continue;
+ }
- in_elf_path = abigail::tests::get_src_dir() + "/tests/" + s->in_elf_path;
- string abidiff = get_build_dir() + "/tools/abidiff";
- cmd = abidiff + " --no-architecture " + in_elf_path + " " + out_abi_path;
- r = true;
- if (system(cmd.c_str()))
+ pthread_mutex_lock(&write_lock);
+ is_ok =
+ abigail::xml_writer::write_corpus_to_native_xml(corp,
+ /*indent=*/0,
+ of);
+ pthread_mutex_unlock(&write_lock);
+ of.close();
+
+ string abilint = get_build_dir() + "/tools/abilint";
+ abilint += " --noout";
+ string cmd = abilint + " " + out_abi_path;
+ if (system(cmd.c_str()))
+ {
+ cerr << "output file doesn't pass abilint: " << out_abi_path << "\n";
+ is_ok = false;
+ }
+
+ string abidiff = get_build_dir() + "/tools/abidiff";
+ cmd = abidiff + " --no-architecture " + in_elf_path + " " + out_abi_path;
+ if (system(cmd.c_str()))
+ {
+ cerr << "ABIs differ:\n"
+ << in_elf_path
+ << "\nand:\n"
+ << out_abi_path
+ << "\n";
+ is_ok = false;
+ }
+
+ in_abi_path = in_abi_base + s->in_abi_path;
+ cmd = "diff -u " + in_abi_path + " " + out_abi_path;
+ if (system(cmd.c_str()))
+ is_ok = false;
+ }
+end:
+ if (no_parallel)
+ return;
+ pthread_exit(NULL);
+}
+
+int
+main(int argc, char *argv[])
+{
+ // Number of currently online processors in the system.
+ size_t nprocs = sysconf(_SC_NPROCESSORS_ONLN);
+ // All the pthread_ts we've created.
+ pthread_t *pthr = new pthread_t[nprocs];
+
+ if (argc == 2)
+ {
+ no_parallel = argv[1] == string("--no-parallel");
+ if (!no_parallel)
{
- cerr << "ABIs differ:\n"
- << in_elf_path
- << "\nand:\n"
- << out_abi_path
- << "\n";
- r = false;
+ cerr << "unrecognized option\n";
+ cerr << "usage: argv[0] [--no-parallel]\n" ;
+ return 1;
}
- is_ok = (is_ok && r);
-
- in_abi_path = abigail::tests::get_src_dir() + "/tests/" + s->in_abi_path;
- cmd = "diff -u " + in_abi_path + " " + out_abi_path;
- r = true;
- if (system(cmd.c_str()))
- r = false;
- is_ok = (is_ok && r);
}
+ if (no_parallel)
+ handle_in_out_spec();
+ else
+ {
+ for (size_t i = 0; i < nprocs; ++i)
+ pthread_create(&pthr[i], NULL,
+ (void*(*)(void*))handle_in_out_spec,
+ NULL);
+
+ for (size_t i = 0; i < nprocs; ++i)
+ pthread_join(pthr[i], NULL);
+ }
+
+ delete pthr;
+
return !is_ok;
}
--
2.4.3