Don't use the mutex for each symbol_set_names call

Message ID 20190929003539.58325-1-cbiesinger@google.com
State New, archived
Headers

Commit Message

Terekhov, Mikhail via Gdb-patches Sept. 29, 2019, 12:35 a.m. UTC
  [So how's this version to avoid the lock overhead? In case threading
doesn't work correctly for some reason, this is in response to
https://sourceware.org/ml/gdb-patches/2019-05/msg00753.html and
applies on top of that patchset.

Also available, rebased to trunk, on
https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/cbiesinger/parallel-minsyms-mutex
]

This avoids the lock contention that was seen with tromey's patch (it
moves it to a once- per-thread lock call from a once-per-symbol call).

It speeds up "attach to Chrome's content_shell binary" from 44 sec -> 30
sec (32%) (compared to GDB trunk), or from 37 sec compared to this
patchset before this patch.

gdb/ChangeLog:

2019-09-28  Christian Biesinger  <cbiesinger@google.com>

	* minsyms.c (minimal_symbol_reader::install): Only do
	demangling on the background thread; still call
	symbol_set_names on the main thread.
	* symtab.c (symbol_find_demangled_name): Make public,
	so that minsyms.c can call it.
	(symbol_set_names): Remove mutex. Avoid demangle call
	if the demangled name is already set.
	* symtab.h (symbol_find_demangled_name): Make public.
---
 gdb/gdbsupport/parallel-for.h |   5 +-
 gdb/minsyms.c                 |  48 +++++++---
 gdb/symtab.c                  | 162 +++++++++++++++-------------------
 gdb/symtab.h                  |  10 +++
 4 files changed, 123 insertions(+), 102 deletions(-)
  

Comments

Tom Tromey Sept. 30, 2019, 2:18 p.m. UTC | #1
>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> Also available, rebased to trunk, on
Christian> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/cbiesinger/parallel-minsyms-mutex
Christian> ]

I didn't read the patch yet, but this weekend I did rebase my branch and
fixed most of the review issues.  So, you may want to consider rebasing
on that.

Tom
  
Terekhov, Mikhail via Gdb-patches Sept. 30, 2019, 9:44 p.m. UTC | #2
Hi Tom,

On Mon, Sep 30, 2019 at 9:18 AM Tom Tromey <tom@tromey.com> wrote:
>
> >>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:
>
> Christian> Also available, rebased to trunk, on
> Christian> https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/cbiesinger/parallel-minsyms-mutex
> Christian> ]
>
> I didn't read the patch yet, but this weekend I did rebase my branch and
> fixed most of the review issues.  So, you may want to consider rebasing
> on that.

Looking at your current patch version, I noticed this line in parallel-for.h:
+  if (n_threads > 1 && 2 * n_threads >= n_elements)

I am fairly sure this should be <= in the threads comparison,
otherwise you only use threads if there are few elements, which is the
opposite of what is desired (perhaps a merge error?)

Christian
  
Tom Tromey Oct. 1, 2019, 5:01 p.m. UTC | #3
>>>>> "Christian" == Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> writes:

Christian> I am fairly sure this should be <= in the threads comparison,
Christian> otherwise you only use threads if there are few elements, which is the
Christian> opposite of what is desired (perhaps a merge error?)

I probably messed it up when adding the "maint" control.

Thanks very much for pointing this out.

Tom
  

Patch

diff --git a/gdb/gdbsupport/parallel-for.h b/gdb/gdbsupport/parallel-for.h
index d63a8c4e82..9041435d7a 100644
--- a/gdb/gdbsupport/parallel-for.h
+++ b/gdb/gdbsupport/parallel-for.h
@@ -38,8 +38,8 @@  extern int max_threads;
    iterators.  For each element, it calls the callback function.  The
    work may or may not be done by separate threads.  */
 
-template<class RandomIt, class UnaryFunction>
-void parallel_for_each (RandomIt first, RandomIt last, UnaryFunction f)
+template<class RandomIt, class UnaryFunction, class PerThreadFinishFunction>
+void parallel_for_each (RandomIt first, RandomIt last, UnaryFunction f, PerThreadFinishFunction per_thread_finish)
 {
   auto body = [&] (RandomIt start, RandomIt end)
     {
@@ -53,6 +53,7 @@  void parallel_for_each (RandomIt first, RandomIt last, UnaryFunction f)
 	    /* Just ignore exceptions.  Each item here must be
 	       independent.  */
 	  }
+      per_thread_finish (start, end);
     };
 
 #if CXX_STD_THREAD
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 10e3c8a548..4802b0bea0 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -55,6 +55,20 @@ 
 #include "safe-ctype.h"
 #include "gdbsupport/parallel-for.h"
 
+#if CXX_STD_THREAD
+#include <mutex>
+#endif
+
+#if CXX_STD_THREAD
+/* Mutex that is used when modifying or accessing the demangled hash
+   table.  This is a global mutex simply because the only current
+   multi-threaded user of the hash table does not process multiple
+   objfiles in parallel.  The mutex could easily live on the per-BFD
+   object, but this approach avoids using extra memory when it is not
+   needed.  */
+static std::mutex demangled_mutex;
+#endif
+
 /* See minsyms.h.  */
 
 bool
@@ -1340,17 +1354,29 @@  minimal_symbol_reader::install ()
 
       msymbols = m_objfile->per_bfd->msymbols.get ();
       gdb::parallel_for_each
-	(&msymbols[0], &msymbols[mcount],
-	 [&] (minimal_symbol &msym)
-	 {
-	   if (!msym.name_set)
-	     {
-	       symbol_set_names (&msym, msym.name,
-				 strlen (msym.name), 0,
-				 m_objfile->per_bfd);
-	       msym.name_set = 1;
-	     }
-	 });
+	(&msymbols[0], &msymbols[mcount], [&] (minimal_symbol &msym)
+	  {
+	    if (!msym.name_set)
+	      {
+	        if (msym.language != language_ada)
+		  {
+		    /* 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;
+		  }
+	      }
+	  },
+	  [&] (minimal_symbol *first, minimal_symbol* last) {
+	    std::lock_guard<std::mutex> guard (demangled_mutex);
+	    for (; first < last; ++first) {
+	      symbol_set_names (first, first->name,
+		  strlen (first->name), 0,
+		  m_objfile->per_bfd);
+	    }
+	  });
 
       build_minimal_symbol_hash_tables (m_objfile);
     }
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 8a7ac6f036..3600e0b0ff 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -69,9 +69,6 @@ 
 #include "arch-utils.h"
 #include <algorithm>
 #include "gdbsupport/pathstuff.h"
-#if CXX_STD_THREAD
-#include <mutex>
-#endif
 
 /* Forward declarations for local functions.  */
 
@@ -713,16 +710,6 @@  symbol_set_language (struct general_symbol_info *gsymbol,
 
 /* Functions to initialize a symbol's mangled name.  */
 
-#if CXX_STD_THREAD
-/* Mutex that is used when modifying or accessing the demangled hash
-   table.  This is a global mutex simply because the only current
-   multi-threaded user of the hash table does not process multiple
-   objfiles in parallel.  The mutex could easily live on the per-BFD
-   object, but this approach avoids using extra memory when it is not
-   needed.  */
-static std::mutex demangled_mutex;
-#endif
-
 /* Objects of this type are stored in the demangled name hash table.  */
 struct demangled_name_entry
 {
@@ -772,13 +759,9 @@  create_demangled_names_hash (struct objfile_per_bfd_storage *per_bfd)
      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,78 +850,79 @@  symbol_set_names (struct general_symbol_info *gsymbol,
 
   struct demangled_name_entry *found_entry;
 
-  {
-#if CXX_STD_THREAD
-    std::lock_guard<std::mutex> guard (demangled_mutex);
-#endif
-
-    if (per_bfd->demangled_names_hash == NULL)
-      create_demangled_names_hash (per_bfd);
-
-    entry.mangled = linkage_name_copy;
-    slot = ((struct demangled_name_entry **)
-	    htab_find_slot (per_bfd->demangled_names_hash.get (),
-			    &entry, INSERT));
-
-    /* If this name is not in the hash table, add it.  */
-    if (*slot == NULL
-	/* A C version of the symbol may have already snuck into the table.
-	   This happens to, e.g., main.init (__go_init_main).  Cope.  */
-	|| (gsymbol->language == language_go
-	    && (*slot)->demangled[0] == '\0'))
-      {
-	char *demangled_name_ptr
-	  = 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;
-
-	/* Suppose we have demangled_name==NULL, copy_name==0, and
-	   linkage_name_copy==linkage_name.  In this case, we already have the
-	   mangled name saved, and we don't have a demangled name.  So,
-	   you might think we could save a little space by not recording
-	   this in the hash table at all.
-	   
-	   It turns out that it is actually important to still save such
-	   an entry in the hash table, because storing this name gives
-	   us better bcache hit rates for partial symbols.  */
-	if (!copy_name && linkage_name_copy == linkage_name)
-	  {
-	    *slot
-	      = ((struct demangled_name_entry *)
-		 obstack_alloc (&per_bfd->storage_obstack,
-				offsetof (struct demangled_name_entry, demangled)
-				+ demangled_len + 1));
-	    (*slot)->mangled = linkage_name;
-	  }
-	else
-	  {
-	    char *mangled_ptr;
-
-	    /* If we must copy the mangled name, put it directly after
-	       the demangled name so we can have a single
-	       allocation.  */
-	    *slot
-	      = ((struct demangled_name_entry *)
-		 obstack_alloc (&per_bfd->storage_obstack,
-				offsetof (struct demangled_name_entry, demangled)
-				+ len + demangled_len + 2));
-	    mangled_ptr = &((*slot)->demangled[demangled_len + 1]);
-	    strcpy (mangled_ptr, linkage_name_copy);
-	    (*slot)->mangled = mangled_ptr;
-	  }
-	(*slot)->language = gsymbol->language;
+  if (per_bfd->demangled_names_hash == NULL)
+    create_demangled_names_hash (per_bfd);
+
+  entry.mangled = linkage_name_copy;
+  slot = ((struct demangled_name_entry **)
+          htab_find_slot (per_bfd->demangled_names_hash.get (),
+      		    &entry, INSERT));
+
+  /* If this name is not in the hash table, add it.  */
+  if (*slot == NULL
+      /* A C version of the symbol may have already snuck into the table.
+         This happens to, e.g., main.init (__go_init_main).  Cope.  */
+      || (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
+        = 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;
+
+      /* Suppose we have demangled_name==NULL, copy_name==0, and
+         linkage_name_copy==linkage_name.  In this case, we already have the
+         mangled name saved, and we don't have a demangled name.  So,
+         you might think we could save a little space by not recording
+         this in the hash table at all.
+         
+         It turns out that it is actually important to still save such
+         an entry in the hash table, because storing this name gives
+         us better bcache hit rates for partial symbols.  */
+      if (!copy_name && linkage_name_copy == linkage_name)
+        {
+          *slot
+            = ((struct demangled_name_entry *)
+      	 obstack_alloc (&per_bfd->storage_obstack,
+      			offsetof (struct demangled_name_entry, demangled)
+      			+ demangled_len + 1));
+          (*slot)->mangled = linkage_name;
+        }
+      else
+        {
+          char *mangled_ptr;
+
+          /* If we must copy the mangled name, put it directly after
+             the demangled name so we can have a single
+             allocation.  */
+          *slot
+            = ((struct demangled_name_entry *)
+      	 obstack_alloc (&per_bfd->storage_obstack,
+      			offsetof (struct demangled_name_entry, demangled)
+      			+ len + demangled_len + 2));
+          mangled_ptr = &((*slot)->demangled[demangled_len + 1]);
+          strcpy (mangled_ptr, linkage_name_copy);
+          (*slot)->mangled = mangled_ptr;
+        }
+      (*slot)->language = gsymbol->language;
 
-	if (demangled_name != NULL)
-	  strcpy ((*slot)->demangled, demangled_name.get ());
-	else
-	  (*slot)->demangled[0] = '\0';
-      }
-    else if (gsymbol->language == language_unknown
-	     || gsymbol->language == language_auto)
-      gsymbol->language = (*slot)->language;
+      if (demangled_name != NULL)
+        strcpy ((*slot)->demangled, demangled_name.get ());
+      else
+        (*slot)->demangled[0] = '\0';
+    }
+  else if (gsymbol->language == language_unknown
+           || gsymbol->language == language_auto)
+    gsymbol->language = (*slot)->language;
 
-    found_entry = *slot;
-  }
+  found_entry = *slot;
 
   gsymbol->name = found_entry->mangled;
   if (found_entry->demangled[0] != '\0')
diff --git a/gdb/symtab.h b/gdb/symtab.h
index c3918a85af..17903df92d 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -483,6 +483,16 @@  extern void symbol_set_language (struct general_symbol_info *symbol,
                                  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