From patchwork Sun Sep 29 00:35:39 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Terekhov, Mikhail via Gdb-patches" X-Patchwork-Id: 34710 Received: (qmail 103511 invoked by alias); 29 Sep 2019 00:35:46 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 103500 invoked by uid 89); 29 Sep 2019 00:35:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.8 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_STOCKGEN, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=recording, rates, UD:binutils-gdb.git, binutilsgdbgit X-HELO: mail-yw1-f74.google.com Received: from mail-yw1-f74.google.com (HELO mail-yw1-f74.google.com) (209.85.161.74) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 29 Sep 2019 00:35:43 +0000 Received: by mail-yw1-f74.google.com with SMTP id 2so5383480ywj.16 for ; Sat, 28 Sep 2019 17:35:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:in-reply-to:message-id:mime-version:references:subject:from:to :cc; bh=YU1wZfPSlOsZTjXWh/LsASWfm/at9fx079nj7fwNAqg=; b=keVGP+t4CoDD5GJsy6lznMZDWLH7zzx5heHms925dabSwNAscu8kyXIQ5qP+A3U2Oj 3KHkOya0WxK55QpxS1nDw1kLCqbqc+Puh0dQWll5q6CWAdshxY4pcmlExjmVxuiUoWic StEMSeLRxPjRlBZG97nwOoWF2Tq4Wyy9YawPWefrq1LBW04Ftw9rIcR3PcZEa4lxzpuD JmLRI6+itu4yN5ARnGBNXnhG+apMearWhkQepAh0avMzzmoEOF1Aj9T8lUayp5D22K5k ocYJet0TqqLsPQayyoi8RnnjZ6Lga+xt/zM79tfVP+OAen47TwE5e0HR7smEeNmKi46y 19hA== Date: Sat, 28 Sep 2019 19:35:39 -0500 In-Reply-To: <1559322805.1454.45.camel@skynet.be> Message-Id: <20190929003539.58325-1-cbiesinger@google.com> Mime-Version: 1.0 References: <1559322805.1454.45.camel@skynet.be> Subject: [PATCH] Don't use the mutex for each symbol_set_names call X-Patchwork-Original-From: "Christian Biesinger via gdb-patches" From: "Terekhov, Mikhail via Gdb-patches" Reply-To: Christian Biesinger To: gdb-patches@sourceware.org Cc: Christian Biesinger X-IsSubscribed: yes [So how's this version to avoid the lock overhead? In case threading doesn't work correctly for some reason, this is in response to https://sourceware.org/ml/gdb-patches/2019-05/msg00753.html and applies on top of that patchset. Also available, rebased to trunk, on https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=shortlog;h=refs/heads/users/cbiesinger/parallel-minsyms-mutex ] This avoids the lock contention that was seen with tromey's patch (it moves it to a once- per-thread lock call from a once-per-symbol call). It speeds up "attach to Chrome's content_shell binary" from 44 sec -> 30 sec (32%) (compared to GDB trunk), or from 37 sec compared to this patchset before this patch. gdb/ChangeLog: 2019-09-28 Christian Biesinger * minsyms.c (minimal_symbol_reader::install): Only do demangling on the background thread; still call symbol_set_names on the main thread. * symtab.c (symbol_find_demangled_name): Make public, so that minsyms.c can call it. (symbol_set_names): Remove mutex. Avoid demangle call if the demangled name is already set. * symtab.h (symbol_find_demangled_name): Make public. --- gdb/gdbsupport/parallel-for.h | 5 +- gdb/minsyms.c | 48 +++++++--- gdb/symtab.c | 162 +++++++++++++++------------------- gdb/symtab.h | 10 +++ 4 files changed, 123 insertions(+), 102 deletions(-) diff --git a/gdb/gdbsupport/parallel-for.h b/gdb/gdbsupport/parallel-for.h index d63a8c4e82..9041435d7a 100644 --- a/gdb/gdbsupport/parallel-for.h +++ b/gdb/gdbsupport/parallel-for.h @@ -38,8 +38,8 @@ extern int max_threads; iterators. For each element, it calls the callback function. The work may or may not be done by separate threads. */ -template -void parallel_for_each (RandomIt first, RandomIt last, UnaryFunction f) +template +void parallel_for_each (RandomIt first, RandomIt last, UnaryFunction f, PerThreadFinishFunction per_thread_finish) { auto body = [&] (RandomIt start, RandomIt end) { @@ -53,6 +53,7 @@ void parallel_for_each (RandomIt first, RandomIt last, UnaryFunction f) /* Just ignore exceptions. Each item here must be independent. */ } + per_thread_finish (start, end); }; #if CXX_STD_THREAD diff --git a/gdb/minsyms.c b/gdb/minsyms.c index 10e3c8a548..4802b0bea0 100644 --- a/gdb/minsyms.c +++ b/gdb/minsyms.c @@ -55,6 +55,20 @@ #include "safe-ctype.h" #include "gdbsupport/parallel-for.h" +#if CXX_STD_THREAD +#include +#endif + +#if CXX_STD_THREAD +/* Mutex that is used when modifying or accessing the demangled hash + table. This is a global mutex simply because the only current + multi-threaded user of the hash table does not process multiple + objfiles in parallel. The mutex could easily live on the per-BFD + object, but this approach avoids using extra memory when it is not + needed. */ +static std::mutex demangled_mutex; +#endif + /* See minsyms.h. */ bool @@ -1340,17 +1354,29 @@ minimal_symbol_reader::install () msymbols = m_objfile->per_bfd->msymbols.get (); gdb::parallel_for_each - (&msymbols[0], &msymbols[mcount], - [&] (minimal_symbol &msym) - { - if (!msym.name_set) - { - symbol_set_names (&msym, msym.name, - strlen (msym.name), 0, - m_objfile->per_bfd); - msym.name_set = 1; - } - }); + (&msymbols[0], &msymbols[mcount], [&] (minimal_symbol &msym) + { + if (!msym.name_set) + { + if (msym.language != language_ada) + { + /* 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; + } + } + }, + [&] (minimal_symbol *first, minimal_symbol* last) { + std::lock_guard guard (demangled_mutex); + for (; first < last; ++first) { + symbol_set_names (first, first->name, + strlen (first->name), 0, + m_objfile->per_bfd); + } + }); build_minimal_symbol_hash_tables (m_objfile); } diff --git a/gdb/symtab.c b/gdb/symtab.c index 8a7ac6f036..3600e0b0ff 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -69,9 +69,6 @@ #include "arch-utils.h" #include #include "gdbsupport/pathstuff.h" -#if CXX_STD_THREAD -#include -#endif /* Forward declarations for local functions. */ @@ -713,16 +710,6 @@ symbol_set_language (struct general_symbol_info *gsymbol, /* Functions to initialize a symbol's mangled name. */ -#if CXX_STD_THREAD -/* Mutex that is used when modifying or accessing the demangled hash - table. This is a global mutex simply because the only current - multi-threaded user of the hash table does not process multiple - objfiles in parallel. The mutex could easily live on the per-BFD - object, but this approach avoids using extra memory when it is not - needed. */ -static std::mutex demangled_mutex; -#endif - /* Objects of this type are stored in the demangled name hash table. */ struct demangled_name_entry { @@ -772,13 +759,9 @@ create_demangled_names_hash (struct objfile_per_bfd_storage *per_bfd) NULL, xcalloc, xfree)); } -/* Try to determine the demangled name for a symbol, based on the - language of that symbol. If the language is set to language_auto, - it will attempt to find any demangling algorithm that works and - then set the language appropriately. The returned name is allocated - by the demangler and should be xfree'd. */ +/* See symtab.h */ -static char * +char * symbol_find_demangled_name (struct general_symbol_info *gsymbol, const char *mangled) { @@ -867,78 +850,79 @@ symbol_set_names (struct general_symbol_info *gsymbol, struct demangled_name_entry *found_entry; - { -#if CXX_STD_THREAD - std::lock_guard guard (demangled_mutex); -#endif - - if (per_bfd->demangled_names_hash == NULL) - create_demangled_names_hash (per_bfd); - - entry.mangled = linkage_name_copy; - slot = ((struct demangled_name_entry **) - htab_find_slot (per_bfd->demangled_names_hash.get (), - &entry, INSERT)); - - /* If this name is not in the hash table, add it. */ - if (*slot == NULL - /* A C version of the symbol may have already snuck into the table. - This happens to, e.g., main.init (__go_init_main). Cope. */ - || (gsymbol->language == language_go - && (*slot)->demangled[0] == '\0')) - { - char *demangled_name_ptr - = symbol_find_demangled_name (gsymbol, linkage_name_copy); - gdb::unique_xmalloc_ptr demangled_name (demangled_name_ptr); - int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0; - - /* Suppose we have demangled_name==NULL, copy_name==0, and - linkage_name_copy==linkage_name. In this case, we already have the - mangled name saved, and we don't have a demangled name. So, - you might think we could save a little space by not recording - this in the hash table at all. - - It turns out that it is actually important to still save such - an entry in the hash table, because storing this name gives - us better bcache hit rates for partial symbols. */ - if (!copy_name && linkage_name_copy == linkage_name) - { - *slot - = ((struct demangled_name_entry *) - obstack_alloc (&per_bfd->storage_obstack, - offsetof (struct demangled_name_entry, demangled) - + demangled_len + 1)); - (*slot)->mangled = linkage_name; - } - else - { - char *mangled_ptr; - - /* If we must copy the mangled name, put it directly after - the demangled name so we can have a single - allocation. */ - *slot - = ((struct demangled_name_entry *) - obstack_alloc (&per_bfd->storage_obstack, - offsetof (struct demangled_name_entry, demangled) - + len + demangled_len + 2)); - mangled_ptr = &((*slot)->demangled[demangled_len + 1]); - strcpy (mangled_ptr, linkage_name_copy); - (*slot)->mangled = mangled_ptr; - } - (*slot)->language = gsymbol->language; + if (per_bfd->demangled_names_hash == NULL) + create_demangled_names_hash (per_bfd); + + entry.mangled = linkage_name_copy; + slot = ((struct demangled_name_entry **) + htab_find_slot (per_bfd->demangled_names_hash.get (), + &entry, INSERT)); + + /* If this name is not in the hash table, add it. */ + if (*slot == NULL + /* A C version of the symbol may have already snuck into the table. + This happens to, e.g., main.init (__go_init_main). Cope. */ + || (gsymbol->language == language_go + && (*slot)->demangled[0] == '\0')) + { + /* The const_cast is safe because the only reason it is already initialized + is if we purposefully set it from a background thread to avoid doing the + work here. However, it is still allocated from the heap and needs to + be freed by us, just like if we called symbol_find_demangled_name + here. */ + char *demangled_name_ptr + = gsymbol->language_specific.demangled_name + ? const_cast(gsymbol->language_specific.demangled_name) + : symbol_find_demangled_name (gsymbol, linkage_name_copy); + gdb::unique_xmalloc_ptr demangled_name (demangled_name_ptr); + int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0; + + /* Suppose we have demangled_name==NULL, copy_name==0, and + linkage_name_copy==linkage_name. In this case, we already have the + mangled name saved, and we don't have a demangled name. So, + you might think we could save a little space by not recording + this in the hash table at all. + + It turns out that it is actually important to still save such + an entry in the hash table, because storing this name gives + us better bcache hit rates for partial symbols. */ + if (!copy_name && linkage_name_copy == linkage_name) + { + *slot + = ((struct demangled_name_entry *) + obstack_alloc (&per_bfd->storage_obstack, + offsetof (struct demangled_name_entry, demangled) + + demangled_len + 1)); + (*slot)->mangled = linkage_name; + } + else + { + char *mangled_ptr; + + /* If we must copy the mangled name, put it directly after + the demangled name so we can have a single + allocation. */ + *slot + = ((struct demangled_name_entry *) + obstack_alloc (&per_bfd->storage_obstack, + offsetof (struct demangled_name_entry, demangled) + + len + demangled_len + 2)); + mangled_ptr = &((*slot)->demangled[demangled_len + 1]); + strcpy (mangled_ptr, linkage_name_copy); + (*slot)->mangled = mangled_ptr; + } + (*slot)->language = gsymbol->language; - if (demangled_name != NULL) - strcpy ((*slot)->demangled, demangled_name.get ()); - else - (*slot)->demangled[0] = '\0'; - } - else if (gsymbol->language == language_unknown - || gsymbol->language == language_auto) - gsymbol->language = (*slot)->language; + if (demangled_name != NULL) + strcpy ((*slot)->demangled, demangled_name.get ()); + else + (*slot)->demangled[0] = '\0'; + } + else if (gsymbol->language == language_unknown + || gsymbol->language == language_auto) + gsymbol->language = (*slot)->language; - found_entry = *slot; - } + found_entry = *slot; gsymbol->name = found_entry->mangled; if (found_entry->demangled[0] != '\0') diff --git a/gdb/symtab.h b/gdb/symtab.h index c3918a85af..17903df92d 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -483,6 +483,16 @@ extern void symbol_set_language (struct general_symbol_info *symbol, enum language language, struct obstack *obstack); + +/* Try to determine the demangled name for a symbol, based on the + language of that symbol. If the language is set to language_auto, + it will attempt to find any demangling algorithm that works and + then set the language appropriately. The returned name is allocated + by the demangler and should be xfree'd. */ + +extern char *symbol_find_demangled_name (struct general_symbol_info *gsymbol, + const char *mangled); + /* Set just the linkage name of a symbol; do not try to demangle it. Used for constructs which do not have a mangled name, e.g. struct tags. Unlike SYMBOL_SET_NAMES, linkage_name must