From: "Christian Biesinger via gdb-patches" <gdb-patches@sourceware.org>
To: gdb-patches@sourceware.org
Cc: Christian Biesinger <cbiesinger@google.com>
Subject: [PATCH] Don't use the mutex for each symbol_set_names call
Date: Mon, 30 Sep 2019 16:55:00 -0000 [thread overview]
Message-ID: <20190930165543.68106-1-cbiesinger@google.com> (raw)
In-Reply-To: <874l0tc1gl.fsf@tromey.com>
[Thanks Tom -- here's a rebased version, with also a small improvement
to parallel-for.h]
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>
* gdbsupport/parallel-for.h (parallel_for_each): Add a way to
run a function on each thread after the elements are processed,
to "finish up" computations.
* 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 | 12 ++-
gdb/minsyms.c | 48 +++++++---
gdb/symtab.c | 162 +++++++++++++++-------------------
gdb/symtab.h | 10 +++
4 files changed, 130 insertions(+), 102 deletions(-)
diff --git a/gdb/gdbsupport/parallel-for.h b/gdb/gdbsupport/parallel-for.h
index e8bfced475..e5b8d33989 100644
--- a/gdb/gdbsupport/parallel-for.h
+++ b/gdb/gdbsupport/parallel-for.h
@@ -36,13 +36,20 @@ namespace gdb
extern int max_threads;
+template <class It>
+void DoNothing(It a, It b)
+{
+}
+
/* A very simple "parallel for". This iterates over the elements
given by the range of iterators, which must be random access
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
+ = DoNothing<RandomIt>)
{
auto body = [&] (RandomIt start, RandomIt end)
{
@@ -60,6 +67,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 8adcff7cf2..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
--
2.23.0.444.g18eeb5a265-goog
next prev parent reply other threads:[~2019-09-30 16:55 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 7/8] Demangle minsyms in parallel 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 2/8] Remove static buffer from ada_decode 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 5/8] Introduce run_on_main_thread Tom Tromey
2019-05-18 21:00 ` [PATCH v2 4/8] Lock the demangled hash table 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 1/8] Defer minimal symbol name-setting 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 [this message]
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 ` [PATCH v2 2/2] Precompute hash value for symbol_set_names Christian Biesinger via gdb-patches
2019-10-03 18:15 ` [PATCH v2 1/2] Don't use the mutex for each symbol_set_names call Christian Biesinger via gdb-patches
2019-09-30 21:45 ` [PATCH] " 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=20190930165543.68106-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