From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 81230 invoked by alias); 2 Oct 2019 18:20:51 -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 81221 invoked by uid 89); 2 Oct 2019 18:20:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.0 required=5.0 tests=AWL,BAYES_00,ENV_AND_HDR_SPF_MATCH,RCVD_IN_DNSWL_NONE,SPF_PASS,USER_IN_DEF_SPF_WL autolearn=ham version=3.3.1 spammy=hesitant X-HELO: mail-ot1-f66.google.com Received: from mail-ot1-f66.google.com (HELO mail-ot1-f66.google.com) (209.85.210.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 02 Oct 2019 18:20:49 +0000 Received: by mail-ot1-f66.google.com with SMTP id o44so15514509ota.10 for ; Wed, 02 Oct 2019 11:20:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=7V2rKJJu/cw5m2O+KXCPjdn54AuDGjs23QfQ6ZDspC4=; b=TnDJuwjAAHK67f6hGEUPQQn0p0bsPTyUoGfJ2mvELWtl3qUnTx3I6kg12nltQGwfLa z2qgC3N1YNoVLFGb5wzO3NDs8gqv5hux1SDLwsYiSKjuA/B+GVf8seQ54zzaFFv7dNPN pQoK2wOZs/4sPv7OupD9qCPYm2tWEio5hCzzueYXqWt2SulQ/UXIcOO9/qqSbsLxbThA J1omlySxWAjMDBChHtkWGsa/tO1D3Pz+o63JAzvTBqGpdgbS9OFZr+R3ukGiIbc4+d8e mU7nCgnW3ksmnTuw4+fesoanb5T4X9GHjIBwrNd7HBSLDK/6dtH3QqtaABkQMs2TdV6X PlxQ== MIME-Version: 1.0 References: <874l0tc1gl.fsf@tromey.com> <20190930165543.68106-1-cbiesinger@google.com> <87r23vxdzs.fsf@tromey.com> In-Reply-To: <87r23vxdzs.fsf@tromey.com> From: "Christian Biesinger via gdb-patches" Reply-To: Christian Biesinger Date: Wed, 02 Oct 2019 18:20:00 -0000 Message-ID: Subject: Re: [PATCH] Don't use the mutex for each symbol_set_names call To: Tom Tromey Cc: Christian Biesinger via gdb-patches Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2019-10/txt/msg00079.txt.bz2 On Wed, Oct 2, 2019 at 12:18 PM Tom Tromey wrote: > > >>>>> "Christian" == Christian Biesinger via gdb-patches writes: > > Christian> It speeds up "attach to Chrome's content_shell binary" from 44 sec -> 30 > Christian> sec (32%) (compared to GDB trunk), or from 37 sec compared to this > Christian> patchset before this patch. > > Nice. I do need to redo these measurements with the latest version of the branch and patch... > Christian> + [&] (minimal_symbol *first, minimal_symbol* last) { > Christian> + std::lock_guard guard (demangled_mutex); > Christian> + for (; first < last; ++first) { > Christian> + symbol_set_names (first, first->name, > Christian> + strlen (first->name), 0, > Christian> + m_objfile->per_bfd); > Christian> + } > Christian> + }); > > IIUC the idea is to separate the demangling from updating the > demangled_names_hash. That's correct. > A couple of thoughts on that... > > Christian> + *slot > Christian> + = ((struct demangled_name_entry *) > Christian> + obstack_alloc (&per_bfd->storage_obstack, > Christian> + offsetof (struct demangled_name_entry, demangled) > Christian> + + len + demangled_len + 2)); > Christian> + mangled_ptr = &((*slot)->demangled[demangled_len + 1]); > Christian> + strcpy (mangled_ptr, linkage_name_copy); > Christian> + (*slot)->mangled = mangled_ptr; > > There's no deep reason that these things have to be stored on the > per-BFD obstack -- and requiring this means an extra copy. Instead we > could change the hash table to use ordinary heap allocation, and I think > this would be more efficient when demangling in worker threads, because > we could just reuse the existing allocation. Yes indeed. I was actually thinking of that last night -- we could change to a hash_set + reuse the alloc from gdb_demangle and avoid any allocations here. > Also, it seems fine to serialize the calls to symbol_set_names. There's > no need for a lock at all, then. True, though this way, if some threads finish faster than others it's possible to parallelize the work a bit more. > One idea I had was to parallelize build_minimal_symbol_hash_tables as > well: when possible, have it run two threads, and create the hash tables > in parallel. Hmm, yeah, that's a good idea. I hadn't thought of doing it quite that way. > Adding a third thread here to update the > demangled_names_hash might help too? Maybe this approach would > eliminate the need for your "Compute msymbol hash codes in parallel" > patch ... the issue there being that it makes the minsyms larger. > (Another way to handle that would be to keep the hashes in a local array > of some kind that is discarded once the hash tables are updated.) The local array is a bit tricky... it needs an entry for each msymbol, which is only known at runtime. So it can't be stack-allocated with a fixed size, and I'm hesitant to use alloca for this unbounded size. So it would require a heap allocation for that vector. Maybe it's still worth it... OK, let me play with some ideas. Christian