[review] Demangle minsyms in parallel

Message ID gerrit.1571543710000.I220341f70e94dd02df5dd424272c50a5afb64978@gnutoolchain-gerrit.osci.io
State New, archived
Headers

Commit Message

Simon Marchi (Code Review) Oct. 20, 2019, 3:55 a.m. UTC
  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

Simon Marchi (Code Review) Oct. 20, 2019, 11:06 a.m. UTC | #1
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.
  
Simon Marchi (Code Review) Nov. 22, 2019, 11:45 p.m. UTC | #2
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);
  
Simon Marchi (Code Review) Nov. 26, 2019, 7:06 p.m. UTC | #3
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);
  
Simon Marchi (Code Review) Nov. 26, 2019, 7:11 p.m. UTC | #4
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
  
Simon Marchi (Code Review) Nov. 26, 2019, 7:42 p.m. UTC | #5
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
  
Simon Marchi (Code Review) Nov. 26, 2019, 8:50 p.m. UTC | #6
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);
| +		      };
  

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index ab67332..f4ec322 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -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.
diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 3be798a..0d98c21 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -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 \
diff --git a/gdb/gdbsupport/parallel-for.h b/gdb/gdbsupport/parallel-for.h
new file mode 100644
index 0000000..52a5edf
--- /dev/null
+++ b/gdb/gdbsupport/parallel-for.h
@@ -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 */
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 4e6bd39..bfcd5d5 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -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);
     }
diff --git a/gdb/symtab.c b/gdb/symtab.c
index fc736fd..da7b703 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -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;
 
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 8552bf1..7d41de9 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -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