From: "Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>
To: gdb-patches@sourceware.org
Cc: Christian Biesinger <cbiesinger@google.com>
Subject: [PATCH v2 1/2] Don't use the mutex for each symbol_set_names call
Date: Thu, 03 Oct 2019 18:15:00 -0000 [thread overview]
Message-ID: <20191003181504.185026-1-cbiesinger@google.com> (raw)
In-Reply-To: <CAPTJ0XEVveoW5LboP0-LmAZv+kkDQqmE3dTQw2uavTqdxbBphQ@mail.gmail.com>
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 --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
--
2.23.0.444.g18eeb5a265-goog
next prev parent reply other threads:[~2019-10-03 18:15 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-18 21:00 [PATCH v2 0/8] Demangle minimal symbol names in worker threads Tom Tromey
2019-05-18 21:00 ` [PATCH v2 5/8] Introduce run_on_main_thread Tom Tromey
2019-05-18 21:00 ` [PATCH v2 6/8] Introduce thread-safe way to handle SIGSEGV Tom Tromey
2019-05-18 21:00 ` [PATCH v2 2/8] Remove static buffer from ada_decode Tom Tromey
2019-05-18 21:00 ` [PATCH v2 8/8] Add maint set/show enable-threads Tom Tromey
2019-05-22 5:01 ` Eli Zaretskii
2019-05-26 20:46 ` Tom Tromey
2019-05-27 2:32 ` Eli Zaretskii
2019-05-18 21:00 ` [PATCH v2 7/8] Demangle minsyms in parallel Tom Tromey
2019-05-18 21:00 ` [PATCH v2 1/8] Defer minimal symbol name-setting Tom Tromey
2019-05-18 21:00 ` [PATCH v2 3/8] Add configure check for std::thread Tom Tromey
2019-05-18 21:00 ` [PATCH v2 4/8] Lock the demangled hash table Tom Tromey
2019-05-19 13:59 ` [PATCH v2 0/8] Demangle minimal symbol names in worker threads Philippe Waroquiers
2019-05-19 18:55 ` Tom Tromey
2019-05-21 0:35 ` Philippe Waroquiers
2019-05-21 7:35 ` Andrew Burgess
2019-05-21 15:45 ` Tom Tromey
2019-05-21 16:21 ` Andrew Burgess
2019-05-31 2:48 ` Tom Tromey
2019-05-31 17:13 ` Philippe Waroquiers
2019-09-29 0:35 ` [PATCH] Don't use the mutex for each symbol_set_names call Christian Biesinger via gdb-patches
2019-09-30 14:18 ` Tom Tromey
2019-09-30 16:55 ` Christian Biesinger via gdb-patches
2019-10-02 17:18 ` Tom Tromey
2019-10-02 18:20 ` Christian Biesinger via gdb-patches
2019-10-02 22:02 ` Christian Biesinger via gdb-patches
2019-10-03 18:15 ` Christian Biesinger via gdb-patches [this message]
2019-10-03 18:15 ` [PATCH v2 2/2] Precompute hash value for symbol_set_names Christian Biesinger via gdb-patches
2019-09-30 21:45 ` [PATCH] Don't use the mutex for each symbol_set_names call Christian Biesinger via gdb-patches
2019-10-01 17:02 ` Tom Tromey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191003181504.185026-1-cbiesinger@google.com \
--to=gdb-patches@sourceware.org \
--cc=cbiesinger@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox