diff mbox

[v2,1/2] Don't use the mutex for each symbol_set_names call

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

Commit Message

Terekhov, Mikhail via Gdb-patches Oct. 3, 2019, 6:15 p.m. UTC 
  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/minsyms.c |  30 +++++++++-
 gdb/symtab.c  | 162 +++++++++++++++++++++++---------------------------
 gdb/symtab.h  |  10 ++++
 3 files changed, 110 insertions(+), 92 deletions(-)
diff mbox

Patch

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 84bf2bb61e..95ca9f6c93 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -56,6 +56,10 @@ 
 #include "gdbsupport/alt-stack.h"
 #include "gdbsupport/parallel-for.h"
 
+#if CXX_STD_THREAD
+#include <mutex>
+#endif
+
 /* See minsyms.h.  */
 
 bool
@@ -1333,6 +1337,11 @@  minimal_symbol_reader::install ()
       m_objfile->per_bfd->minimal_symbol_count = mcount;
       m_objfile->per_bfd->msymbols = std::move (msym_holder);
 
+#if CXX_STD_THREAD
+      /* Mutex we hold for calling symbol_set_names.  */
+      std::mutex demangled_mutex;
+#endif
+
       msymbols = m_objfile->per_bfd->msymbols.get ();
       gdb::parallel_for_each
 	(&msymbols[0], &msymbols[mcount],
@@ -1346,12 +1355,27 @@  minimal_symbol_reader::install ()
 	     {
 	       if (!msym->name_set)
 		 {
-		   symbol_set_names (msym, msym->name,
-				     strlen (msym->name), 0,
-				     m_objfile->per_bfd);
+		   /* 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 8adcff7cf2..47da5cf4e8 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