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

["Tom Tromey (Code Review)" <gerrit@gnutoolchain-gerrit.osci.io>]

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.



-- 
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-CC: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Thu, 31 Oct 2019 21:24:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment