[review] [RFC] Don't block on finishing demangling msymbols

[Christian Biesinger (Code Review)]

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?



-- 
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I9d871917459ece0b41d31670b3c56600757aea66
Gerrit-Change-Number: 463
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Mon, 04 Nov 2019 05:52:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment