[review] Turn off threaded minsym demangling by default
Commit Message
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/687
......................................................................
Turn off threaded minsym demangling by default
Per discussion on gdb-patches with Joel, this patch turns off multihreaded
symbol loading by default. It can be turned on using:
maint set worker-threads unlimited
To keep the behavior as close as possible to the old code, it still
calls symbol_set_names in the old place if n_worker_threads is 0.
gdb/ChangeLog:
2019-11-18 Christian Biesinger <cbiesinger@google.com>
* maint.c (n_worker_threads): Default to 0.
(worker_threads_disabled): New function.
* maint.h (worker_threads_disabled): New function.
* minsyms.c (minimal_symbol_reader::record_full): Call symbol_set_names
here if worker_threads_disabled () is true.
(minimal_symbol_reader::install): Skip all threading if
worker_threads_disabled () is true.
Change-Id: I92ba4f6bbf07363189666327cad452d6b9c8e01d
---
M gdb/maint.c
M gdb/maint.h
M gdb/minsyms.c
3 files changed, 47 insertions(+), 30 deletions(-)
Comments
Christian Biesinger has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/687
......................................................................
Patch Set 1:
tromey -- sorry I missed your IRC question; here I did more than just change the setting because I was concerned that deferring the symbol_set_names call may have a risk in case some code relies on the name being set earlier. Beyond that I skipped all the threading set up because it's not necessary with that first part. But if you think I'm overly cautious I'm fine with not doing that.
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/687
......................................................................
Patch Set 1: Code-Review+2
I finally got around to reading this.
I think this makes sense. And, I think it's fine to revert immediately
after the 9.1 branch is created.
Thanks for doing this.
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/687
......................................................................
Patch Set 2: Code-Review+2
Still looks good. Thanks for doing this.
@@ -845,7 +845,12 @@
}
#endif
-static int n_worker_threads = -1;
+static int n_worker_threads = 0;
+
+bool worker_threads_disabled ()
+{
+ return n_worker_threads == 0;
+}
/* Update the thread pool for the desired number of threads. */
static void
@@ -26,6 +26,8 @@
extern void set_per_command_space (int);
+extern bool worker_threads_disabled ();
+
/* Records a run time and space usage to be used as a base for
reporting elapsed time or change in space. */
@@ -53,6 +53,7 @@
#include "gdbsupport/symbol.h"
#include <algorithm>
#include "safe-ctype.h"
+#include "maint.h"
#include "gdbsupport/parallel-for.h"
#if CXX_STD_THREAD
@@ -1139,6 +1140,12 @@
else
msymbol->name = name.data ();
+ if (worker_threads_disabled ())
+ /* To keep our behavior as close as possible to the previous non-threaded
+ behavior for GDB 9.1, we call symbol_set_names here when threads
+ are disabled. */
+ symbol_set_names (msymbol, msymbol->name, false, m_objfile->per_bfd);
+
SET_MSYMBOL_VALUE_ADDRESS (msymbol, address);
MSYMBOL_SECTION (msymbol) = section;
@@ -1364,43 +1371,46 @@
m_objfile->per_bfd->minimal_symbol_count = mcount;
m_objfile->per_bfd->msymbols = std::move (msym_holder);
+ if (!worker_threads_disabled ())
+ {
#if CXX_STD_THREAD
- /* Mutex that is used when modifying or accessing the demangled
- hash table. */
- std::mutex demangled_mutex;
+ /* Mutex that is used when modifying or accessing the demangled
+ hash table. */
+ std::mutex demangled_mutex;
#endif
- msymbols = m_objfile->per_bfd->msymbols.get ();
- gdb::parallel_for_each
- (&msymbols[0], &msymbols[mcount],
- [&] (minimal_symbol *start, minimal_symbol *end)
- {
- for (minimal_symbol *msym = start; msym < end; ++msym)
+ msymbols = m_objfile->per_bfd->msymbols.get ();
+ gdb::parallel_for_each
+ (&msymbols[0], &msymbols[mcount],
+ [&] (minimal_symbol *start, minimal_symbol *end)
{
- if (!msym->name_set)
+ for (minimal_symbol *msym = start; msym < end; ++msym)
{
- /* 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;
+ if (!msym->name_set)
+ {
+ /* 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, false,
- m_objfile->per_bfd);
+ /* 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, false,
+ m_objfile->per_bfd);
+ }
}
- }
- });
+ });
+ }
build_minimal_symbol_hash_tables (m_objfile);
}