Don't use the mutex for each symbol_set_names call

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 50 sec -> 30
sec (32%).

See https://sourceware.org/ml/gdb-patches/2019-10/msg00087.html for some
more speed measurements.

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.

diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 84bf2bb61e206df5353f45c7f65a5674557f22f9..95ca9f6c930d3170353ed61d838c4144447c9608 100644 (file)
--- 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 8adcff7cf2b693933d35ff36bbc29c6a6de6bc64..47da5cf4e8cb01c275219e920d915fbf059c2c3e 100644 (file)
--- 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 c3918a85af7cadad228e17f36d183491e1d33db2..17903df92d69447002407969780fe7565a91b306 100644 (file)
--- 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