From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 58549 invoked by alias); 31 Oct 2019 21:24:47 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 58462 invoked by uid 89); 31 Oct 2019 21:24:40 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00 autolearn=ham version=3.3.1 spammy=1395, 344 X-HELO: mx1.osci.io Received: from polly.osci.io (HELO mx1.osci.io) (8.43.85.229) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 31 Oct 2019 21:24:29 +0000 Received: by mx1.osci.io (Postfix, from userid 994) id 9E24C204A6; Thu, 31 Oct 2019 17:24:21 -0400 (EDT) Received: from gnutoolchain-gerrit.osci.io (gnutoolchain-gerrit.osci.io [IPv6:2620:52:3:1:5054:ff:fe06:16ca]) by mx1.osci.io (Postfix) with ESMTP id 4A71220266; Thu, 31 Oct 2019 17:24:18 -0400 (EDT) Received: from localhost (localhost [127.0.0.1]) by gnutoolchain-gerrit.osci.io (Postfix) with ESMTP id 1FFC420AF6; Thu, 31 Oct 2019 17:24:18 -0400 (EDT) X-Gerrit-PatchSet: 1 Date: Thu, 31 Oct 2019 21:24:00 -0000 From: "Tom Tromey (Code Review)" To: Christian Biesinger , gdb-patches@sourceware.org Auto-Submitted: auto-generated X-Gerrit-MessageType: comment Subject: [review] [RFC] Don't block on finishing demangling msymbols X-Gerrit-Change-Id: I9d871917459ece0b41d31670b3c56600757aea66 X-Gerrit-Change-Number: 463 X-Gerrit-ChangeURL: X-Gerrit-Commit: 8c71b191dbb8b87b8942fe7c4014757e3cb37831 In-Reply-To: References: X-Gerrit-Comment-Date: Thu, 31 Oct 2019 17:24:18 -0400 Reply-To: gnutoolchain-gerrit@osci.io MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Disposition: inline User-Agent: Gerrit/3.0.3-75-g9005159e5d Content-Type: text/plain; charset=UTF-8 Message-Id: <20191031212418.1FFC420AF6@gnutoolchain-gerrit.osci.io> X-SW-Source: 2019-10/txt/msg01212.txt.bz2 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 Gerrit-CC: Tom Tromey Gerrit-Comment-Date: Thu, 31 Oct 2019 21:24:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment