[review] Demangle minsyms in parallel
Commit Message
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/173
......................................................................
Demangle minsyms in parallel
This patch introduces a simple parallel for_each and changes the
minimal symbol reader to use it when computing the demangled name for
a minimal symbol. This yields a speedup when reading minimal symbols.
gdb/ChangeLog
2019-10-19 Christian Biesinger <cbiesinger@google.com>
Tom Tromey <tom@tromey.com>
* minsyms.c (minimal_symbol_reader::install): Use
parallel_for_each.
* gdbsupport/parallel-for.h: New file.
* Makefile.in (HFILES_NO_SRCDIR): Add gdbsupport/parallel-for.h.
Change-Id: I220341f70e94dd02df5dd424272c50a5afb64978
---
M gdb/ChangeLog
M gdb/Makefile.in
A gdb/gdbsupport/parallel-for.h
M gdb/minsyms.c
M gdb/symtab.c
M gdb/symtab.h
6 files changed, 154 insertions(+), 17 deletions(-)
Comments
Christian Biesinger has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/173
......................................................................
Patch Set 1: Code-Review+1
This looks good to me. Updated performance numbers, with my usual "attach to chrome" test:
Before this patch:
real 0m36.525s
user 0m28.044s
sys 0m8.481s
(this is faster than it used to be because of a change I made to chrome's new objfile handler & because of https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/124)
After this patch set:
real 0m18.179s
user 0m28.180s
sys 0m4.612s
So takes half the time now! The various other patches I've send to the list bring it down to ~10 sec, I will upload them to gerrit later.
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/173
......................................................................
Patch Set 3:
(1 comment)
| --- /dev/null
| +++ gdb/gdbsupport/parallel-for.h
| @@ -1,0 +45,19 @@ parallel_for_each (RandomIt first, RandomIt last, RangeFunction callback)
| +#if CXX_STD_THREAD
| + /* So we can use a local array below. */
| + const size_t local_max = 16;
| + size_t n_threads = std::min (thread_pool::g_thread_pool->count (),
| + local_max);
| + size_t n_actual_threads = 0;
| + std::future<void> futures[local_max];
| +
| + size_t n_elements = last - first;
| + if (n_threads > 1 && 2 * n_threads <= n_elements)
PS3, Line 54:
I think this check doesn't really make sense -- if one has many
elements but also many threads available, the answer shouldn't be
to give up.
In the next revision, I've changed it to reduce the number of threads
used when there aren't many elements per thread. I picked an
arbitrary cutoff of a minimum of 10 elements.
| + {
| + size_t elts_per_thread = n_elements / n_threads;
| + n_actual_threads = n_threads - 1;
| + for (int i = 0; i < n_actual_threads; ++i)
| + {
| + RandomIt end = first + elts_per_thread;
| + auto task = [=] ()
| + {
| + callback (first, end);
Pedro Alves has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/173
......................................................................
Patch Set 4:
(2 comments)
Sending these review comments I had on patchset 3. I'll take a look at v4.
| --- /dev/null
| +++ gdb/gdbsupport/parallel-for.h
| @@ -1,0 +16,19 @@ /* Parallel for loops
| +
| + You should have received a copy of the GNU General Public License
| + along with this program. If not, see <http://www.gnu.org/licenses/>. */
| +
| +#ifndef GDBSUPPORT_PARALLEL_FOR_H
| +#define GDBSUPPORT_PARALLEL_FOR_H
| +
| +#include <algorithm>
| +#if CXX_STD_THREAD
| +#include <system_error>
PS3, Line 25:
Is <system_error> necessary?
| +#include <thread>
| +#include "gdbsupport/thread-pool.h"
| +#endif
| +
| +namespace gdb
| +{
| +
| +/* A very simple "parallel for". This splits the range of iterators
| + into subranges, and then passes each subrange to the callback. The
...
| @@ -1,0 +45,19 @@ parallel_for_each (RandomIt first, RandomIt last, RangeFunction callback)
| +#if CXX_STD_THREAD
| + /* So we can use a local array below. */
| + const size_t local_max = 16;
| + size_t n_threads = std::min (thread_pool::g_thread_pool->count (),
| + local_max);
| + size_t n_actual_threads = 0;
| + std::future<void> futures[local_max];
| +
| + size_t n_elements = last - first;
| + if (n_threads > 1 && 2 * n_threads <= n_elements)
PS3, Line 54:
This "2 * n_threads <= n_elements" gave me pause. I think it warrants
a comment.
(I see you've addressed this already.)
| + {
| + size_t elts_per_thread = n_elements / n_threads;
| + n_actual_threads = n_threads - 1;
| + for (int i = 0; i < n_actual_threads; ++i)
| + {
| + RandomIt end = first + elts_per_thread;
| + auto task = [=] ()
| + {
| + callback (first, end);
Pedro Alves has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/173
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
LGTM. I'm just curious on the <system_error> include.
| --- /dev/null
| +++ gdb/gdbsupport/parallel-for.h
| @@ -1,0 +16,19 @@ /* Parallel for loops
| +
| + You should have received a copy of the GNU General Public License
| + along with this program. If not, see <http://www.gnu.org/licenses/>. */
| +
| +#ifndef GDBSUPPORT_PARALLEL_FOR_H
| +#define GDBSUPPORT_PARALLEL_FOR_H
| +
| +#include <algorithm>
| +#if CXX_STD_THREAD
| +#include <system_error>
PS4, Line 25:
Same comment as v3, do we need this?
| +#include <thread>
| +#include "gdbsupport/thread-pool.h"
| +#endif
| +
| +namespace gdb
| +{
| +
| +/* A very simple "parallel for". This splits the range of iterators
| + into subranges, and then passes each subrange to the callback. The
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/173
......................................................................
Patch Set 4:
(1 comment)
| --- /dev/null
| +++ gdb/gdbsupport/parallel-for.h
| @@ -1,0 +16,19 @@ /* Parallel for loops
| +
| + You should have received a copy of the GNU General Public License
| + along with this program. If not, see <http://www.gnu.org/licenses/>. */
| +
| +#ifndef GDBSUPPORT_PARALLEL_FOR_H
| +#define GDBSUPPORT_PARALLEL_FOR_H
| +
| +#include <algorithm>
| +#if CXX_STD_THREAD
| +#include <system_error>
PS4, Line 25:
> Same comment as v3, do we need this?
Nope, I've removed it.
| +#include <thread>
| +#include "gdbsupport/thread-pool.h"
| +#endif
| +
| +namespace gdb
| +{
| +
| +/* A very simple "parallel for". This splits the range of iterators
| + into subranges, and then passes each subrange to the callback. The
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/173
......................................................................
Patch Set 4:
(1 comment)
| --- /dev/null
| +++ gdb/gdbsupport/parallel-for.h
| @@ -1,0 +50,19 @@ #if CXX_STD_THREAD
| + size_t n_actual_threads = 0;
| + std::future<void> futures[local_max];
| +
| + size_t n_elements = last - first;
| + if (n_threads > 1)
| + {
| + /* Arbitrarily require that there should be at least 10 elements
| + in a thread. */
| + if (n_elements / n_threads < 10)
| + n_threads = n_elements / 10;
PS4, Line 59:
This could yield zero, causing a SIGFPE.
I've fixed this now.
| + size_t elts_per_thread = n_elements / n_threads;
| + n_actual_threads = n_threads - 1;
| + for (int i = 0; i < n_actual_threads; ++i)
| + {
| + RandomIt end = first + elts_per_thread;
| + auto task = [=] ()
| + {
| + callback (first, end);
| + };
@@ -1,6 +1,14 @@
2019-10-19 Christian Biesinger <cbiesinger@google.com>
Tom Tromey <tom@tromey.com>
+ * minsyms.c (minimal_symbol_reader::install): Use
+ parallel_for_each.
+ * gdbsupport/parallel-for.h: New file.
+ * Makefile.in (HFILES_NO_SRCDIR): Add gdbsupport/parallel-for.h.
+
+2019-10-19 Christian Biesinger <cbiesinger@google.com>
+ Tom Tromey <tom@tromey.com>
+
* gdbsupport/thread-pool.h: New file.
* gdbsupport/thread-pool.c: New file.
* Makefile.in (COMMON_SFILES): Add thread-pool.c.
@@ -1477,6 +1477,7 @@
gdbsupport/common-inferior.h \
gdbsupport/netstuff.h \
gdbsupport/host-defs.h \
+ gdbsupport/parallel-for.h \
gdbsupport/pathstuff.h \
gdbsupport/print-utils.h \
gdbsupport/ptid.h \
new file mode 100644
@@ -0,0 +1,82 @@
+/* Parallel for loops
+
+ Copyright (C) 2019 Free Software Foundation, Inc.
+
+ This file is part of GDB.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+#ifndef GDBSUPPORT_PARALLEL_FOR_H
+#define GDBSUPPORT_PARALLEL_FOR_H
+
+#include <algorithm>
+#if CXX_STD_THREAD
+#include <system_error>
+#include <thread>
+#include "gdbsupport/thread-pool.h"
+#endif
+
+namespace gdb
+{
+
+/* A very simple "parallel for". This splits the range of iterators
+ into subranges, and then passes each subrange to the callback. The
+ work may or may not be done in separate threads.
+
+ This approach was chosen over having the callback work on single
+ items because it makes it simple for the caller to do
+ once-per-subrange initialization and destruction. */
+
+template<class RandomIt, class RangeFunction>
+void
+parallel_for_each (RandomIt first, RandomIt last, RangeFunction callback)
+{
+#if CXX_STD_THREAD
+ /* So we can use a local array below. */
+ const size_t local_max = 16;
+ size_t n_threads = std::min (thread_pool::g_thread_pool.count (), local_max);
+ size_t n_actual_threads = 0;
+ std::future<void> futures[local_max];
+
+ size_t n_elements = last - first;
+ if (n_threads > 1 && 2 * n_threads <= n_elements)
+ {
+ size_t elts_per_thread = n_elements / n_threads;
+ n_actual_threads = n_threads - 1;
+ for (int i = 0; i < n_actual_threads; ++i)
+ {
+ RandomIt end = first + elts_per_thread;
+ auto task = [=] ()
+ {
+ callback (first, end);
+ };
+
+ futures[i] = gdb::thread_pool::g_thread_pool.post_task (task);
+ first = end;
+ }
+ }
+#endif /* CXX_STD_THREAD */
+
+ /* Process all the remaining elements in the main thread. */
+ callback (first, last);
+
+#if CXX_STD_THREAD
+ for (int i = 0; i < n_actual_threads; ++i)
+ futures[i].wait ();
+#endif /* CXX_STD_THREAD */
+}
+
+}
+
+#endif /* GDBSUPPORT_PARALLEL_FOR_H */
@@ -53,6 +53,11 @@
#include "gdbsupport/symbol.h"
#include <algorithm>
#include "safe-ctype.h"
+#include "gdbsupport/parallel-for.h"
+
+#if CXX_STD_THREAD
+#include <mutex>
+#endif
/* See minsyms.h. */
@@ -1354,17 +1359,44 @@
m_objfile->per_bfd->minimal_symbol_count = mcount;
m_objfile->per_bfd->msymbols = std::move (msym_holder);
+#if CXX_STD_THREAD
+ /* Mutex that is used when modifying or accessing the demangled
+ hash table. */
+ std::mutex demangled_mutex;
+#endif
+
msymbols = m_objfile->per_bfd->msymbols.get ();
- for (int i = 0; i < mcount; ++i)
- {
- if (!msymbols[i].name_set)
- {
- symbol_set_names (&msymbols[i], msymbols[i].name,
- strlen (msymbols[i].name), 0,
- m_objfile->per_bfd);
- msymbols[i].name_set = 1;
- }
- }
+ gdb::parallel_for_each
+ (&msymbols[0], &msymbols[mcount],
+ [&] (minimal_symbol *start, minimal_symbol *end)
+ {
+ for (minimal_symbol *msym = start; msym < end; ++msym)
+ {
+ if (!msym->name_set)
+ {
+ /* This will be freed later, by symbol_set_names. */
+ char *demangled_name
+ = symbol_find_demangled_name (msym, msym->name);
+ symbol_set_demangled_name
+ (msym, demangled_name,
+ &m_objfile->per_bfd->storage_obstack);
+ msym->name_set = 1;
+ }
+ }
+ {
+ /* To limit how long we hold the lock, we only acquire it here
+ and not while we demangle the names above. */
+#if CXX_STD_THREAD
+ std::lock_guard<std::mutex> guard (demangled_mutex);
+#endif
+ for (minimal_symbol *msym = start; msym < end; ++msym)
+ {
+ symbol_set_names (msym, msym->name,
+ strlen (msym->name), 0,
+ m_objfile->per_bfd);
+ }
+ }
+ });
build_minimal_symbol_hash_tables (m_objfile);
}
@@ -759,13 +759,9 @@
NULL, xcalloc, xfree));
}
-/* Try to determine the demangled name for a symbol, based on the
- language of that symbol. If the language is set to language_auto,
- it will attempt to find any demangling algorithm that works and
- then set the language appropriately. The returned name is allocated
- by the demangler and should be xfree'd. */
+/* See symtab.h */
-static char *
+char *
symbol_find_demangled_name (struct general_symbol_info *gsymbol,
const char *mangled)
{
@@ -867,8 +863,16 @@
|| (gsymbol->language == language_go
&& (*slot)->demangled[0] == '\0'))
{
+ /* The const_cast is safe because the only reason it is already
+ initialized is if we purposefully set it from a background
+ thread to avoid doing the work here. However, it is still
+ allocated from the heap and needs to be freed by us, just
+ like if we called symbol_find_demangled_name here. */
char *demangled_name_ptr
- = symbol_find_demangled_name (gsymbol, linkage_name_copy);
+ = (gsymbol->language_specific.demangled_name
+ ? const_cast<char *> (gsymbol->language_specific.demangled_name)
+ : symbol_find_demangled_name (gsymbol, linkage_name_copy));
+
gdb::unique_xmalloc_ptr<char> demangled_name (demangled_name_ptr);
int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
@@ -495,6 +495,16 @@
enum language language,
struct obstack *obstack);
+
+/* Try to determine the demangled name for a symbol, based on the
+ language of that symbol. If the language is set to language_auto,
+ it will attempt to find any demangling algorithm that works and
+ then set the language appropriately. The returned name is allocated
+ by the demangler and should be xfree'd. */
+
+extern char *symbol_find_demangled_name (struct general_symbol_info *gsymbol,
+ const char *mangled);
+
/* Set just the linkage name of a symbol; do not try to demangle
it. Used for constructs which do not have a mangled name,
e.g. struct tags. Unlike SYMBOL_SET_NAMES, linkage_name must