From patchwork Mon Sep 30 16:55:43 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: 34732 Received: (qmail 22609 invoked by alias); 30 Sep 2019 16:55:51 -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 22486 invoked by uid 89); 30 Sep 2019 16:55:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.7 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=rates X-HELO: mail-qt1-f201.google.com Received: from mail-qt1-f201.google.com (HELO mail-qt1-f201.google.com) (209.85.160.201) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 30 Sep 2019 16:55:48 +0000 Received: by mail-qt1-f201.google.com with SMTP id i10so14512956qtq.3 for ; Mon, 30 Sep 2019 09:55:48 -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=pu8S1cwY7SirZOb8CfgnExONJ+blMM49JTE3rWm/jWw=; b=Gui3gNZgtkkhDnad6tQmrqmvvVAM5SUvafKQBF1FdewXqwAQ2xDLax2aO/eZF6rhbN R3jAWEhv3KfEs/m4bt9RWJ/dLWnjVpbMw+wB0rih4HPevca89yUQIDToSi9qerQ3ZAbh TvjnpapH/qcjfsViRF8KbOZrBxg0nfyJfiVuugFdgR6Pk+S8dODtfgp4GnolHiCUy1nX 99fL8fLcIseC8P4CDH5hIkvqTNLMSJmJdw+zm+fdqBEzPdemZ7qE1K4bd3cvQvVSzoM+ qdO+nXYFI4pq1xYUaD4bwKxdq05noE9CH6PnbfLakB5c8gYuI25+LzEtmgdAvvkeBIkx wSdQ== Date: Mon, 30 Sep 2019 11:55:43 -0500 In-Reply-To: <874l0tc1gl.fsf@tromey.com> Message-Id: <20190930165543.68106-1-cbiesinger@google.com> Mime-Version: 1.0 References: <874l0tc1gl.fsf@tromey.com> 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 [Thanks Tom -- here's a rebased version, with also a small improvement to parallel-for.h] 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 * gdbsupport/parallel-for.h (parallel_for_each): Add a way to run a function on each thread after the elements are processed, to "finish up" computations. * 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 | 12 ++- gdb/minsyms.c | 48 +++++++--- gdb/symtab.c | 162 +++++++++++++++------------------- gdb/symtab.h | 10 +++ 4 files changed, 130 insertions(+), 102 deletions(-) diff --git a/gdb/gdbsupport/parallel-for.h b/gdb/gdbsupport/parallel-for.h index e8bfced475..e5b8d33989 100644 --- a/gdb/gdbsupport/parallel-for.h +++ b/gdb/gdbsupport/parallel-for.h @@ -36,13 +36,20 @@ namespace gdb extern int max_threads; +template +void DoNothing(It a, It b) +{ +} + /* A very simple "parallel for". This iterates over the elements given by the range of iterators, which must be random access 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 + = DoNothing) { auto body = [&] (RandomIt start, RandomIt end) { @@ -60,6 +67,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 8adcff7cf2..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