From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4454 invoked by alias); 3 Jun 2013 17:27:48 -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 4436 invoked by uid 89); 3 Jun 2013 17:27:45 -0000 X-Spam-SWARE-Status: No, score=-7.8 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.1 Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Mon, 03 Jun 2013 17:27:44 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r53HRd3S016030 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Mon, 3 Jun 2013 13:27:39 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id r53HRaBd009783; Mon, 3 Jun 2013 13:27:36 -0400 Message-ID: <51ACD207.2000402@redhat.com> Date: Mon, 03 Jun 2013 17:27:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: Doug Evans CC: Keith Seitz , brobecker@adacore.com, psmith@gnu.org, gdb-patches@sourceware.org Subject: Re: [patch] Improve symbol lookup performance noted in PR 15519 References: <20903.57436.871210.593441@ruffy.mtv.corp.google.com> <51A86FF5.9090401@redhat.com> <20905.9501.800725.439795@ruffy.mtv.corp.google.com> In-Reply-To: <20905.9501.800725.439795@ruffy.mtv.corp.google.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-06/txt/msg00023.txt.bz2 On 05/31/2013 11:33 PM, Doug Evans wrote: > Pedro Alves writes: > > On 05/31/2013 12:27 AM, Doug Evans wrote: > > > Hi. > > > Here's the patch I intend to check in to "fix" > > > http://sourceware.org/bugzilla/show_bug.cgi?id=15519 > > > [It's not a complete "fix", there's still some performance gains > > > to be had, but I'm leaving that for a separate pass. This gets > > > us >95% of the way there, at least in the benchmarks I've been using, > > > including the one in the PR. Thanks Paul!] > > > > > > No regressions on amd64-linux, with/without Fission. > > > > > > I'll let it sit for a few days in case there are any more comments. > > > I'd also like to commit this to the 7.6 branch. Ok Joel? > > > [I need to rerun the testsuite in that tree before committing there.] > > > > Thanks! > > > > Absolutely no objections, but for my own education, and for the archives, > > could you describe the gist of it? E.g., what is it that is slow, and what > > is it that is being avoided to get the speedup? (where's the win?) > > It isn't super obvious to me from reading the patch. > > I think the speedup can be illustrated nicely with this example. > > Apply this patch to the before/after gdbs. > > --- cp-namespace.c~ 2013-05-31 12:23:44.000000000 -0700 > +++ cp-namespace.c 2013-05-31 12:28:26.000000000 -0700 > @@ -250,6 +250,7 @@ cp_lookup_symbol_in_namespace (const cha > const struct block *block, > const domain_enum domain, int search) > { > + printf_filtered ("cp_lookup_symbol_in_namespace (\"%s\", \"%s\", block=%p, search=%d)\n", namespace, name, block, search); > if (namespace[0] == '\0') > { > return lookup_symbol_file (name, block, domain, 0, search); > > Running gdb on one example I see this in the "before" case (cvs head): > > (gdb) p masterCatalog->traceLog->slots > cp_lookup_symbol_in_namespace ("", "PlatformRecords", block=0x2be2ab0, search=1) ... > $1 = 200 > (gdb) > > Running gdb on the same example in the "after" case: > > (gdb) p masterCatalog->traceLog->slots > cp_lookup_symbol_in_namespace ("", "PlatformRecords", block=0x2d56920, search=1) ... > $1 = 200 > (gdb) > > Now imagine scaling up the excessive duplicate searches due to > - a larger binary (bigger/more complex classes, namespaces, imports, etc.) > - or a deeper call stack (some places iterate over the block chain), > - or because it's being done in some loop in a macro. > The latter is Paul's case. > In another example (Chrome) a 5 second lookup becomes 10s of minutes! > > So the speedup comes from calling a more specific lookup routine for the task at hand. Thanks. What I guess I'm still missing to understand it, is a short blurb describing what is being skipped and why is it safe to be skipped. :-) Just to make sure I understand the change -- I see cp_lookup_symbol_namespace does: ... /* Search for name in namespaces imported to this and parent blocks. */ while (block != NULL) { sym = cp_lookup_symbol_imports (scope, name, block, domain, 0, 1); if (sym) return sym; block = BLOCK_SUPERBLOCK (block); } and a chunk of the speedup comes from skipping that, correct? That is, it is supposedly unnecessary to look symbol imports when looking up a symbol in a baseclass, right? The patch then also replaces a lookup_symbol_static with a specific block call followed by a fallback lookup_static_symbol_aux search over all objfiles, by always doing the lookup_static_symbol_aux search over all objfiles. It makes me wonder if it was there for a reason things were done that way before, like for something like the same named class/methods being implemented/hidden/private in different sos/dlls (therefore not really subject to ODR), therefore making it desirable to lookup in the same context first. I have no idea, probably not. :-) I guess I'm just after getting the analysis/conclusion that led to the change recorded for posterity. :-) > Also note that even in the "after" case we are still doing > a lot of duplicate lookups. > cp-namespace.c is a bit clumsy in some of the lookups it does. > Although it's hampered by having to work with the data structures > gdb provides it. My plan is to work on both this summer. Thanks. -- Pedro Alves