[review,RFC] Don't block on finishing demangling msymbols
Commit Message
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463
......................................................................
[RFC] Don't block on finishing demangling msymbols
Instead, do it all on a background thread and only block when we actually
need the result.
Note, this is a speedup but not quite as much as I was expecting; still
looking into what causes the waits. However, let me know if you have thoughts
on the concept!
Change-Id: I9d871917459ece0b41d31670b3c56600757aea66
---
M gdb/minsyms.c
M gdb/objfiles.h
2 files changed, 97 insertions(+), 60 deletions(-)
Comments
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463
......................................................................
Patch Set 1:
(2 comments)
I think the overall idea looks good. This is basically one of the
things I'd like to do for psymtabs as well -- read them in the background
and then wait for the future to resolve when the psyms are actually needed.
I have some specific comments below. Also, with regards to the future thing,
I've also got a patch to let one add tasks that yield values to the thread
pool that I can push somewhere. That's useful with the memoizing future.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463/1/gdb/minsyms.c
File gdb/minsyms.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463/1/gdb/minsyms.c@344
PS1, Line 344:
311 | lookup_minimal_symbol (const char *name, const char *sfile,
| ...
339 | "lookup_minimal_symbol (%s, %s, %s)\n",
340 | name, sfile != NULL ? sfile : "NULL",
341 | objfile_debug_name (objfile));
342 | }
343 |
344 > objfile->per_bfd->wait_for_msymbols ();
345 | /* Do two passes: the first over the ordinary hash table,
346 | and the second over the demangled hash table. */
347 | lookup_minimal_symbol_mangled (name, sfile, objfile,
348 | objfile->per_bfd->msymbol_hash,
349 | mangled_hash, mangled_cmp, found);
Somewhere I have a patch to add a "memoizing future" type,
with the idea being that it is a smart pointer that wraps
both a future and a real pointer, and when requesting the value
it simply waits for the future first. I think this sort of
thing might be preferable to adding a bunch of calls like this.
I can push it to my github if you think that sounds worthwhile.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463/1/gdb/minsyms.c@1390
PS1, Line 1390:
1310 | minimal_symbol_reader::install ()
| ...
1385 | m_objfile->per_bfd->msymbols = std::move (msym_holder);
1386 |
1387 | msymbols = m_objfile->per_bfd->msymbols.get ();
1388 | objfile_per_bfd_storage* per_bfd = m_objfile->per_bfd;
1389 |
1390 > m_objfile->per_bfd->m_minsym_future
1391 > = gdb::thread_pool::g_thread_pool->post_task ([msymbols, mcount,
1392 > per_bfd] ()
1393 | {
1394 | #if CXX_STD_THREAD
1395 | /* Mutex that is used when modifying or accessing the demangled
1396 | hash table. */
1397 | std::mutex demangled_mutex;
I didn't look in detail but the main possible danger here is if
this needs any information from the minimal symbol reader, then
it will fail since that goes out of scope. (Probably not an issue
really.)
I was also considering something along these lines for psymtabs,
and wondering if we'd want to implement a form of work stealing
(by hand) so that one of the worker threads isn't essentially blocked
waiting for the other worker threads.
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463
......................................................................
Patch Set 1:
(1 comment)
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463/1/gdb/minsyms.c
File gdb/minsyms.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463/1/gdb/minsyms.c@1390
PS1, Line 1390:
1310 | minimal_symbol_reader::install ()
| ...
1385 | m_objfile->per_bfd->msymbols = std::move (msym_holder);
1386 |
1387 | msymbols = m_objfile->per_bfd->msymbols.get ();
1388 | objfile_per_bfd_storage* per_bfd = m_objfile->per_bfd;
1389 |
1390 > m_objfile->per_bfd->m_minsym_future
1391 > = gdb::thread_pool::g_thread_pool->post_task ([msymbols, mcount,
1392 > per_bfd] ()
1393 | {
1394 | #if CXX_STD_THREAD
1395 | /* Mutex that is used when modifying or accessing the demangled
1396 | hash table. */
1397 | std::mutex demangled_mutex;
> I didn't look in detail but the main possible danger here is if
> this needs any information from the minimal symbol reader, then
> it will fail since that goes out of scope.
I forgot, there's actually another danger, which is why I haven't
tried to do background psymtab reading yet.
The problem is that an objfile can possibly have a short lifetime.
So, in theory it could be deleted while the worker threads are
still running.
For psymtabs I am thinking I will convert objfiles to be managed
via shared_ptr<>. However, this patch got a bit bogged down and
so I haven't completed it yet.
Christian Biesinger has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463
......................................................................
Patch Set 1:
(2 comments)
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463/1/gdb/minsyms.c
File gdb/minsyms.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463/1/gdb/minsyms.c@344
PS1, Line 344:
311 | lookup_minimal_symbol (const char *name, const char *sfile,
| ...
339 | "lookup_minimal_symbol (%s, %s, %s)\n",
340 | name, sfile != NULL ? sfile : "NULL",
341 | objfile_debug_name (objfile));
342 | }
343 |
344 > objfile->per_bfd->wait_for_msymbols ();
345 | /* Do two passes: the first over the ordinary hash table,
346 | and the second over the demangled hash table. */
347 | lookup_minimal_symbol_mangled (name, sfile, objfile,
348 | objfile->per_bfd->msymbol_hash,
349 | mangled_hash, mangled_cmp, found);
> Somewhere I have a patch to add a "memoizing future" type, […]
Seems like a good idea. A bit inconvenient for use here since we have multiple things we're waiting for though, right? And the hashtable is an in/out parameter...
On the other hand, some kind of smart wrapper does seem like a good idea
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463/1/gdb/minsyms.c@1390
PS1, Line 1390:
1310 | minimal_symbol_reader::install ()
| ...
1385 | m_objfile->per_bfd->msymbols = std::move (msym_holder);
1386 |
1387 | msymbols = m_objfile->per_bfd->msymbols.get ();
1388 | objfile_per_bfd_storage* per_bfd = m_objfile->per_bfd;
1389 |
1390 > m_objfile->per_bfd->m_minsym_future
1391 > = gdb::thread_pool::g_thread_pool->post_task ([msymbols, mcount,
1392 > per_bfd] ()
1393 | {
1394 | #if CXX_STD_THREAD
1395 | /* Mutex that is used when modifying or accessing the demangled
1396 | hash table. */
1397 | std::mutex demangled_mutex;
> > I didn't look in detail but the main possible danger here is if […]
I purposely didn't use a default capture here to reduce the risk of that happening. I'm not sure I understand what you mean with the work stealing?
I meant to call the wait function in the destructor of the per bfd object, bug managing it via a shared ptr seems better to avoid the performance downside. I saw you sent that patch today, thanks!
By the way, in which circumstance does an objfile have a short lifetime?
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463
......................................................................
Patch Set 1:
(2 comments)
Sorry about the delay on replying to this.
| --- gdb/minsyms.c
| +++ gdb/minsyms.c
| @@ -335,18 +335,19 @@ lookup_minimal_symbol (const char *name, const char *sfile,
| {
| if (symbol_lookup_debug)
| {
| fprintf_unfiltered (gdb_stdlog,
| "lookup_minimal_symbol (%s, %s, %s)\n",
| name, sfile != NULL ? sfile : "NULL",
| objfile_debug_name (objfile));
| }
|
| + objfile->per_bfd->wait_for_msymbols ();
PS1, Line 344:
Ideally you'd shrink it down to just a single object I guess.
The memoized future is here
https://github.com/tromey/gdb/commit/ca3ff45f0d8e373f4da74504c2d48aa4c
92a44d5
The thread-pool change to help with this is here:
https://github.com/tromey/gdb/commit/3aa5f2cde3b4c2ae5a1af216bbbd8924c
182a077
Both are rough drafts since who knows if I'll end up using them
for psymtabs or not (recently I started leaning towards "not"
due to some psymtab weirdnesses).
| /* Do two passes: the first over the ordinary hash table,
| and the second over the demangled hash table. */
| lookup_minimal_symbol_mangled (name, sfile, objfile,
| objfile->per_bfd->msymbol_hash,
| mangled_hash, mangled_cmp, found);
|
| /* If not found, try the demangled hash table. */
| if (found.external_symbol.minsym == NULL)
| {
...
| @@ -1410,12 +1388,11 @@ #endif
| - hash_values[idx].minsym_demangled_hash
| - = search_name_hash (MSYMBOL_LANGUAGE (msym),
| - MSYMBOL_SEARCH_NAME (msym));
| - }
| - {
| - /* To limit how long we hold the lock, we only acquire it here
| - and not while we demangle the names above. */
| + objfile_per_bfd_storage* per_bfd = m_objfile->per_bfd;
| +
| + m_objfile->per_bfd->m_minsym_future
| + = gdb::thread_pool::g_thread_pool->post_task ([msymbols, mcount,
| + per_bfd] ()
PS1, Line 1392:
> I purposely didn't use a default capture here to reduce the risk of that happening. I'm not sure I understand what you mean with the work stealing?
I meant, if you push a job into the thread pool, and then it
pushes further jobs, ideally that outer job could be some kind
of state machine so that it doesn't take up a thread pool slot
just waiting for the child jobs to finish.
> By the way, in which circumstance does an objfile have a short lifetime?
If the inferior does a dlopen followed by a dlclose, gdb will read
the objfile and then discard it. If work is happening in the
background,
and the results aren't immediately needed, then my worry would be
the objfile being deleted while the worker is processing.
On my psymtabs-in-the-background branch, I just have the worker
keep a shared_ptr reference to the objfile to avoid this.
Maybe we could implement some kind of cancellation idea (but
not, like, async cancellation) to reduce the amount of useless
work in these cases.
| + {
| #if CXX_STD_THREAD
| - std::lock_guard<std::mutex> guard (demangled_mutex);
| + /* Mutex that is used when modifying or accessing the demangled
| + hash table. */
| + std::mutex demangled_mutex;
| #endif
| - for (minimal_symbol *msym = start; msym < end; ++msym)
| - {
Christian Biesinger has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463
......................................................................
Patch Set 1:
(1 comment)
| --- gdb/minsyms.c
| +++ gdb/minsyms.c
| @@ -335,18 +335,19 @@ lookup_minimal_symbol (const char *name, const char *sfile,
| {
| if (symbol_lookup_debug)
| {
| fprintf_unfiltered (gdb_stdlog,
| "lookup_minimal_symbol (%s, %s, %s)\n",
| name, sfile != NULL ? sfile : "NULL",
| objfile_debug_name (objfile));
| }
|
| + objfile->per_bfd->wait_for_msymbols ();
PS1, Line 344:
So the problem here is that the hash tables, at least
demangled_names_hash, isn't just an output, it's really an inout
parameter when you have multiple minsym readers installed for the same
bfd. There's also the issue that with the future you have to be
careful to std::move everywhere so as not to introduce extra copies.
I was thinking something like this:
class BlockedOnFuture<T> {
T contents;
std::future<void> future;
T& get() { if (future.valid()) future.wait(); return contents; }
T& operator->() { return get(); }
void set_future(std::future<void>& f) { future = f; }
};
And T here would be a struct with the hashtables.
That way we'll set the future when we start the symbol reading and
everything will work in a straightforward way.
WDYT?
| /* Do two passes: the first over the ordinary hash table,
| and the second over the demangled hash table. */
| lookup_minimal_symbol_mangled (name, sfile, objfile,
| objfile->per_bfd->msymbol_hash,
| mangled_hash, mangled_cmp, found);
|
| /* If not found, try the demangled hash table. */
| if (found.external_symbol.minsym == NULL)
| {
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/463
......................................................................
Patch Set 1:
(1 comment)
| --- gdb/minsyms.c
| +++ gdb/minsyms.c
| @@ -335,18 +335,19 @@ lookup_minimal_symbol (const char *name, const char *sfile,
| {
| if (symbol_lookup_debug)
| {
| fprintf_unfiltered (gdb_stdlog,
| "lookup_minimal_symbol (%s, %s, %s)\n",
| name, sfile != NULL ? sfile : "NULL",
| objfile_debug_name (objfile));
| }
|
| + objfile->per_bfd->wait_for_msymbols ();
PS1, Line 344:
> WDYT?
Yeah -- that's essentially what the "memoized future" I linked to
does.
| /* Do two passes: the first over the ordinary hash table,
| and the second over the demangled hash table. */
| lookup_minimal_symbol_mangled (name, sfile, objfile,
| objfile->per_bfd->msymbol_hash,
| mangled_hash, mangled_cmp, found);
|
| /* If not found, try the demangled hash table. */
| if (found.external_symbol.minsym == NULL)
| {
@@ -157,15 +157,15 @@
TABLE. */
static void
add_minsym_to_demangled_hash_table (struct minimal_symbol *sym,
- struct objfile *objfile,
+ struct objfile_per_bfd_storage *per_bfd,
unsigned int hash_value)
{
if (sym->demangled_hash_next == NULL)
{
- objfile->per_bfd->demangled_hash_languages.set (MSYMBOL_LANGUAGE (sym));
+ per_bfd->demangled_hash_languages.set (MSYMBOL_LANGUAGE (sym));
struct minimal_symbol **table
- = objfile->per_bfd->msymbol_demangled_hash;
+ = per_bfd->msymbol_demangled_hash;
unsigned int hash_index = hash_value % MINIMAL_SYMBOL_HASH_SIZE;
sym->demangled_hash_next = table[hash_index];
table[hash_index] = sym;
@@ -341,6 +341,7 @@
objfile_debug_name (objfile));
}
+ objfile->per_bfd->wait_for_msymbols ();
/* Do two passes: the first over the ordinary hash table,
and the second over the demangled hash table. */
lookup_minimal_symbol_mangled (name, sfile, objfile,
@@ -477,6 +478,7 @@
(struct objfile *objf, const lookup_name_info &lookup_name,
gdb::function_view<bool (struct minimal_symbol *)> callback)
{
+ objf->per_bfd->wait_for_msymbols ();
/* The first pass is over the ordinary hash table. */
{
const char *name = linkage_name_str (lookup_name);
@@ -529,6 +531,7 @@
for (objfile *objfile : objf->separate_debug_objfiles ())
{
+ objfile->per_bfd->wait_for_msymbols ();
for (minimal_symbol *msymbol = objfile->per_bfd->msymbol_hash[hash];
msymbol != NULL;
msymbol = msymbol->hash_next)
@@ -562,6 +565,7 @@
if (objf == NULL || objf == objfile
|| objf == objfile->separate_debug_objfile_backlink)
{
+ objfile->per_bfd->wait_for_msymbols ();
for (msymbol = objfile->per_bfd->msymbol_hash[hash];
msymbol != NULL && found_symbol.minsym == NULL;
msymbol = msymbol->hash_next)
@@ -609,6 +613,7 @@
if (objf == NULL || objf == objfile
|| objf == objfile->separate_debug_objfile_backlink)
{
+ objfile->per_bfd->wait_for_msymbols ();
for (msymbol = objfile->per_bfd->msymbol_hash[hash];
msymbol != NULL;
msymbol = msymbol->hash_next)
@@ -720,6 +725,7 @@
/* If this objfile has a minimal symbol table, go search it
using a binary search. */
+ objfile->per_bfd->wait_for_msymbols ();
if (objfile->per_bfd->minimal_symbol_count > 0)
{
int best_zero_sized = -1;
@@ -1270,27 +1276,27 @@
static void
build_minimal_symbol_hash_tables
- (struct objfile *objfile,
+ (struct objfile_per_bfd_storage *per_bfd,
const std::vector<computed_hash_values>& hash_values)
{
int i;
struct minimal_symbol *msym;
/* (Re)insert the actual entries. */
- int mcount = objfile->per_bfd->minimal_symbol_count;
+ int mcount = per_bfd->minimal_symbol_count;
for ((i = 0,
- msym = objfile->per_bfd->msymbols.get ());
+ msym = per_bfd->msymbols.get ());
i < mcount;
i++, msym++)
{
msym->hash_next = 0;
- add_minsym_to_hash_table (msym, objfile->per_bfd->msymbol_hash,
+ add_minsym_to_hash_table (msym, per_bfd->msymbol_hash,
hash_values[i].minsym_hash);
msym->demangled_hash_next = 0;
if (MSYMBOL_SEARCH_NAME (msym) != MSYMBOL_LINKAGE_NAME (msym))
add_minsym_to_demangled_hash_table
- (msym, objfile, hash_values[i].minsym_demangled_hash);
+ (msym, per_bfd, hash_values[i].minsym_demangled_hash);
}
}
@@ -1311,8 +1317,11 @@
if (m_objfile->per_bfd->minsyms_read)
return;
+
if (m_msym_count > 0)
{
+ m_objfile->per_bfd->wait_for_msymbols ();
+
if (symtab_create_debug)
{
fprintf_unfiltered (gdb_stdlog,
@@ -1375,63 +1384,71 @@
m_objfile->per_bfd->minimal_symbol_count = mcount;
m_objfile->per_bfd->msymbols = std::move (msym_holder);
-#if CXX_STD_THREAD
- /* Mutex that is used when modifying or accessing the demangled
- hash table. */
- std::mutex demangled_mutex;
-#endif
-
- std::vector<computed_hash_values> hash_values (mcount);
-
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)
- {
- size_t idx = msym - msymbols;
- hash_values[idx].name_length = strlen (msym->name);
- 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;
+ objfile_per_bfd_storage* per_bfd = m_objfile->per_bfd;
- hash_values[idx].mangled_name_hash
- = fast_hash (msym->name, hash_values[idx].name_length);
- }
- hash_values[idx].minsym_hash
- = msymbol_hash (MSYMBOL_LINKAGE_NAME (msym));
- hash_values[idx].minsym_demangled_hash
- = search_name_hash (MSYMBOL_LANGUAGE (msym),
- MSYMBOL_SEARCH_NAME (msym));
- }
- {
- /* To limit how long we hold the lock, we only acquire it here
- and not while we demangle the names above. */
+ m_objfile->per_bfd->m_minsym_future
+ = gdb::thread_pool::g_thread_pool->post_task ([msymbols, mcount,
+ per_bfd] ()
+ {
#if CXX_STD_THREAD
- std::lock_guard<std::mutex> guard (demangled_mutex);
+ /* Mutex that is used when modifying or accessing the demangled
+ hash table. */
+ std::mutex demangled_mutex;
#endif
- for (minimal_symbol *msym = start; msym < end; ++msym)
- {
- size_t idx = msym - msymbols;
- symbol_set_names
- (msym,
- gdb::string_view(msym->name,
- hash_values[idx].name_length),
- false,
- m_objfile->per_bfd,
- hash_values[idx].mangled_name_hash);
- }
- }
- });
- build_minimal_symbol_hash_tables (m_objfile, hash_values);
+ std::vector<computed_hash_values> hash_values (mcount);
+
+ gdb::parallel_for_each
+ (&msymbols[0], &msymbols[mcount],
+ [&] (minimal_symbol *start, minimal_symbol *end)
+ {
+ for (minimal_symbol *msym = start; msym < end; ++msym)
+ {
+ size_t idx = msym - msymbols;
+ hash_values[idx].name_length = strlen (msym->name);
+ 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,
+ &per_bfd->storage_obstack);
+ msym->name_set = 1;
+
+ hash_values[idx].mangled_name_hash
+ = fast_hash (msym->name, hash_values[idx].name_length);
+ }
+ hash_values[idx].minsym_hash
+ = msymbol_hash (MSYMBOL_LINKAGE_NAME (msym));
+ hash_values[idx].minsym_demangled_hash
+ = search_name_hash (MSYMBOL_LANGUAGE (msym),
+ MSYMBOL_SEARCH_NAME (msym));
+ }
+ {
+ /* 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)
+ {
+ size_t idx = msym - msymbols;
+ symbol_set_names
+ (msym,
+ gdb::string_view(msym->name,
+ hash_values[idx].name_length),
+ false,
+ per_bfd,
+ hash_values[idx].mangled_name_hash);
+ }
+ }
+ });
+
+ build_minimal_symbol_hash_tables (per_bfd, hash_values);
+ }
+ );
}
}
@@ -1518,6 +1535,8 @@
other sections, to find the next symbol in this section with a
different address. */
+ minsym.objfile->per_bfd->wait_for_msymbols ();
+
struct minimal_symbol *past_the_end
= (minsym.objfile->per_bfd->msymbols.get ()
+ minsym.objfile->per_bfd->minimal_symbol_count);
@@ -30,6 +30,9 @@
#include "psymtab.h"
#include <bitset>
#include <vector>
+#if CXX_STD_THREAD
+#include <future>
+#endif
#include "gdbsupport/next-iterator.h"
#include "gdbsupport/safe-iterator.h"
#include "bcache.h"
@@ -319,6 +322,19 @@
/* All the different languages of symbols found in the demangled
hash table. */
std::bitset<nr_languages> demangled_hash_languages;
+
+ void wait_for_msymbols() {
+#if CXX_STD_THREAD
+ if (m_minsym_future.valid ())
+ {
+ m_minsym_future.wait ();
+ m_minsym_future = std::future<void> ();
+ }
+#endif
+ }
+#if CXX_STD_THREAD
+ std::future<void> m_minsym_future;
+#endif
};
/* An iterator that first returns a parent objfile, and then each
@@ -439,11 +455,13 @@
minimal_symbol_iterator begin () const
{
+ m_objfile->per_bfd->wait_for_msymbols ();
return minimal_symbol_iterator (m_objfile->per_bfd->msymbols.get ());
}
minimal_symbol_iterator end () const
{
+ m_objfile->per_bfd->wait_for_msymbols ();
return minimal_symbol_iterator
(m_objfile->per_bfd->msymbols.get ()
+ m_objfile->per_bfd->minimal_symbol_count);